Skip to content

Commit

Permalink
Make sure altstack_size_ is a multiple of the system page size (#1883)
Browse files Browse the repository at this point in the history
Summary: This PR modifies the logic that sets the value of
`altstack_size_`. When deploying Pixie on a 7th gen AMI running bottle
rocket, the value of `altstack_size_` was set to `MINSIGSTKSZ`. This
value was 39616 and was not a multiple of the system page size.

In this [line of
code](https://github.com/pixie-io/pixie/blob/0888c38df2dc414a36cc84198c7b159c39eea0e0/src/common/signal/signal_action.cc#L149)
`mprotect` was then applied on a non page aligned region since its
address was set as `altstack_ + guard_size_ + altstack_size_` leading to
an error. This region in `altstack_` was also not the tailing
`guard_size_` area as
[mmap](https://github.com/pixie-io/pixie/blob/0888c38df2dc414a36cc84198c7b159c39eea0e0/src/common/signal/signal_action.cc#L145)
would have added extra bytes to `altstack_` in order to page align it.

As per the docs, the address passed to `mprotect` needs to be aligned to
a page boundary. The function in this PR makes sure that the value of
`altstack_size_` is a multiple of the page size so that the address
calculated/passed to `mprotect` is aligned to a page boundary and that
it protects the tailing `guard_size_` region of `altstack_`

Relevant Issues: Fixes #1882

Type of change: /kind bug

Test Plan: Skaffolded pixie on EKS clusters with m7i.large, r7i.large
instances and on a GKE cluster with e2-standard-4 nodes and saw the PEM
start up

---------

Signed-off-by: Kartik Pattaswamy <kpattaswamy@pixielabs.ai>
  • Loading branch information
kpattaswamy committed May 1, 2024
1 parent c399d12 commit f30a32e
Showing 1 changed file with 13 additions and 1 deletion.
14 changes: 13 additions & 1 deletion src/common/signal/signal_action.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class SignalAction : public NotCopyable {
public:
SignalAction()
: guard_size_(px::system::Config::GetInstance().PageSizeBytes()),
altstack_size_(std::max(guard_size_ * 4, static_cast<size_t>(MINSIGSTKSZ))) {
altstack_size_(DetermineAltStackSize()) {
MapAndProtectStackMemory();
InstallSigHandlers();
}
Expand Down Expand Up @@ -71,6 +71,18 @@ class SignalAction : public NotCopyable {
* Additionally, two guard pages will be allocated to bookend the usable area.
*/
const size_t altstack_size_;
/**
* Determine the number of bytes to allocate to altstack_size_.
*/
size_t DetermineAltStackSize() const {
// The size of altstack_size_ should be at least 4 * guard size or size MINSIGSTKSZ if it is
// greater. This size needs to be a multiple of the system page size.
const size_t min_altstack_size = 4 * guard_size_;
const size_t sig_page_count = IntRoundUpDivide(static_cast<size_t>(MINSIGSTKSZ), guard_size_);
const size_t sig_stack_size = sig_page_count * guard_size_;

return std::max(min_altstack_size, sig_stack_size);
}
/**
* Signal handlers will be installed for these signals which have a fatal outcome.
*/
Expand Down

0 comments on commit f30a32e

Please sign in to comment.