Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

init: override cmdline #12

Closed
wants to merge 1 commit into from
Closed

init: override cmdline #12

wants to merge 1 commit into from

Conversation

Snaipe
Copy link
Member

@Snaipe Snaipe commented Aug 2, 2020

This is a rather involved commit that makes the builtin init of bst
change its own cmdline to "bst-init", so that other linux tools are able
to distinguish between it and the parent bst process.

This is a rather involved commit that makes the builtin init of bst
change its own cmdline to "bst-init", so that other linux tools are able
to distinguish between it and the parent bst process.
Copy link
Contributor

@kduda kduda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is nuts. Why isn't there a simple way to setproctitle()?

Copy link
Member

@peadar peadar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked this over with Snaipe.

This is all predicated on the fact that there's value in changing what's visible when looking at /proc//commandline

The naiive thing to do is scribble on argv[0]. That has two specific issues:
1: if the new command line is bigger than the old command line, you end up scribbling on the environment. You can fix __environ to get over this from the POV of the process, but /proc/pid/environ will be broken/misleading
2: if the new command line is smaller than the old command line, then you have to pad it with null bytes, and the interpretation can be a sequence of empty args to the process.

The more sophisticated programmer will use PR_SET_MM_*
However, these (mostly) require CAP_SYS_RESOURCE, which you don't have in this process for some reason (lack of it inherited across the fork from outside the namespace, maybe).

It turns out you can do PR_SET_MM_MAP ... which is perfect...
but there's no PR_GET_MM_MAP, so you have to resort to the fprintf monster here...

The other part of our discussion ended up being "why don't you just exec a new executable bst-init, rather than try to pretend bst is bst-init"

If you choose to use the executable name to distinguish what a process does, and bst-init is doing something different to bst, iit's more than reasonable to implement init(8) like functionality in a separate executable.

Snaipe's trying that. If it doesn't work out, I, reluctantly ok with this as the next best alternative. Some comment to summarise the issues with prctl above in the code would be useful for the reader to understand why on earth this hoop has to be jumped through.


/* That's right, prctl wants the argv array to exist within the stack area.
If it's outside, it inexplicably makes /proc/pid/cmdline always empty. */
mm.arg_start = (__u64) &init_progname;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: taking the address of an array like this is redundant. Feel free to ignore for consistency with the rest of the code.

mm.arg_end = (__u64) &init_progname + sizeof (init_progname);

if (prctl(PR_SET_MM, PR_SET_MM_MAP, &mm, sizeof (mm), 0) == -1) {
warn("prctl(PR_SET_MM, PR_SET_MM_MAP)");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are relying on this change for things like pgrep/pkill/pidof, then maybe this should be fatal?

Snaipe added a commit that referenced this pull request Aug 4, 2020
This commit splits out bst-init into its own executable. This has a
bunch of interesting properties for bst:

First, it allows us to rewrite the process cmdline without doing
anything too crazy (*cough* PR_SET_MM_MAP), which helps tools
distinguish bst from its init process

Second, we don't need to explicitly mark the init as dumpable to let
inner processes with root privileges look at /proc/1/*.

Third, this lets us implement --init, which allows user to specify an
init process of their choosing, should the behaviour of *bst-init* not be
adapted to the situation they're in.

This supersedes #12.
@Snaipe Snaipe closed this Aug 5, 2020
@Snaipe Snaipe deleted the feature/init-cmdline branch August 5, 2020 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants