[Toybox] [PATCH] nsenter: A tool to use setns(2)

[Andy Lutomirski]

On Sun, Oct 19, 2014 at 2:48 PM, Rob Landley <rob at landley.net> wrote:
> I imported the first one, but applied the second as a patch because new
> commands go in the "pending" directory so I don't lose track of what
> I've fully reviewed yet.
>
> On 10/17/14 22:01, Andy Lutomirski wrote:
>> nsenter: A tool to use setns(2)
>
> I don't have this command on my host system, and it's not even in the
> python "install this package if you want this command" thing.

It's in util-linux.  I bet you're using Ubuntu or Debian :)  Except
for very new Debians (IIRC), they're both quite a few years behind on
util-linux updates.

>
>> This implements all of the namespace parts of nsenter, but UID and GID
>> switching are missing, as are -r and -w (both because they're not strictly
>> necessary and because the nsenter manpage has an insufficient
>> description of how they work).
>>
>> diff -r 5330d3f88fa3 -r 5520248c82b2 toys/other/nsenter.c
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/toys/other/nsenter.c    Fri Oct 17 20:00:58 2014 -0700
>> @@ -0,0 +1,104 @@
>> +/* nsenter.c - Enter existing namespaces
>> + *
>> + * Copyright 2014 andy Lutomirski <luto at amacapital.net>
>> +
>> +USE_NSENTER(NEWTOY(nsenter, "<1F(no-fork)t#(target)i:(ipc);m:(mount);n:(net);p:(pid);u:(uts);U:(user);", TOYFLAG_USR|TOYFLAG_BIN))
>
> All the ";" characters mean you can only supply the option to a longopt
> with =, as in "--ipc=abc" but "--ipc abc" are considered two separate
> arguments (as in "--ipc= abc").
>
> (Because ls --color works that way. Because gnu, apparently.)
>
> I'm guessing this is _not_ the behavior you want? I don't even remember
> what I made the short options do in that case, I designed it to apply to
> a bare longopt that needed it.

I think this is the behavior I want.  I haven't checked exactly for
longopts, but, for the short form, the behavior seems to match
util-linux's.

>
>> +config NSENTER
>> +  bool "nsenter"
>> +  default n
>> +  help
>> +    usage: nsenter [-t pid] [-F] [-i] [-m] [-n] [-p] [-u] [-U] COMMAND...
>> +
>> +    Run COMMAND in a different set of namespaces.
>> +
>> +    -T  PID to take namespaces from
>
> Lower case above, capital here.

Whoops.  Lowercase is correct; the "-T" is wrong.

>
>> +    -F  don't fork, even if -p is set
>> +
>> +    The namespaces to switch are:
>> +
>> +    -i       SysV IPC (message queues, semaphores, shared memory)
>> +    -m       Mount/unmount tree
>> +    -n       Network address, sockets, routing, iptables
>> +    -p       Process IDs and init (will fork unless -F is used)
>> +    -u       Host and domain names
>> +    -U       UIDs, GIDs, capabilities
>> +
>> +    Each of those options takes an optional argument giving the path of
>> +    the namespace file (usually in /proc).  This optional argument is
>> +    mandatory unless -t is used.
>> +*/
>
> Lower case again. and I note that the lib/args.c option parsing
> infrastructure isn't really designed to work that way. (I can probably
> extend it, just... What command are you copying the behavior of?)
>
>> +#define FOR_nsenter
>> +#define _GNU_SOURCE
>
> I never do that. This is not a GNU project, it's a Linux project. (If
> anything it stands in opposition to gnu.)
>
> The headers have #define _ALL_SOURCE for musl, for the rest I do fixups
> in portabilty.h as necessary (and include linux/*.h headers from the
> local command, since that doesn't belong in the global headers either).
>
>> +#include "toys.h"
>> +#include <errno.h>
>> +#include <sched.h>
>> +#include <linux/sched.h>
>
> I note toys.h already includes errno.h and sched.h in the standard
> headers, and I already did the sched.h namespace dance for this stuff in
> unshare.c. I'll probably put this command in that file so they can share
> the infrastructure.
>
>> +#define NUM_NSTYPES 6
>
> We have an ARRAY_LEN() macro.
>
>> +struct nstype {
>> +  int type;
>> +  const char *name;
>> +};
>> +
>> +struct nstype nstypes[NUM_NSTYPES] = {
>> +  {CLONE_NEWUSER, "user"}, /* must be first to allow non-root operation */
>> +  {CLONE_NEWUTS,  "uts"},
>> +  {CLONE_NEWPID,  "pid"},
>> +  {CLONE_NEWNET,  "net"},
>> +  {CLONE_NEWNS,   "mnt"},
>> +  {CLONE_NEWIPC,  "ipc"},
>> +};
>
> If you leave the array's [] empty it'll automatically allocate space for
> the number of entries you provide, and then you can ARRAY_LEN to see the
> length.
>
> That said, if unhare already has an array of these symbols we can reuse
> that and then ahve the names just be a "user\0uts\0pid\0net\0mnt\ipc"
> string.
>
> Also, I try not to define global data outside the GLOBALS block. If it
> needs to be initialized, I do that in the function's main(). (There are
> times when having static global data is sufficiently cheaper to be worth
> it, but it's not my first choice. Finding a way to avoid needing it is
> my first choice.)

That should have been const, in which case I think it makes sense to
have it outside GLOBALS.  But I'll defer to your proposed tinier
version.

>
> (P.S. I can do this cleanup, just explaining conceptually how I'd
> simplify it.)
>
>> +GLOBALS(
>> +  char *nsnames[6];
>> +  long targetpid;
>> +)
>
> You have a macro above, and a hardwired 6 here. (I know the macro
> doesn't apply to globals, but if you're willing to hardwire 6 here
> having the macro at all is not a net win.)

At least it reduces the number of them. :-/

>
>> +static void enter_by_name(int idx)
>> +{
>
> This function has exactly one caller, and can be inlined at that
> callsite. Doing so often reveals opportunities for further simplification.

I wrote it this way because I might want to split the loop if I ever
implement the setuid stuff.  But, on second thought, that might not be
necessary.

>
>> +  int fd, rc;
>> +  char buf[64];
>
> We have "toybuf", a static 4k global char array, available to all
> commands. (And there's another one "libbuf" for library code.)
>
>> +  char *filename = TT.nsnames[idx];
>> +
>> +  if (!(toys.optflags & (1<<idx))) return;
>> +
>> +  if (!filename || !*filename) {
>
> Ok, I'll bite: where did TT.nsnames[] get set to anything other than
> NULL, so that filename would not be NULL here? (The GLOBALS block starts
> zeroed.)

I think that happened when I did toybox nsenter -t PID -U command.
This should be interpreted as -U with no argument followed by a
command, and filename ended up being the empty string.

--Andy

 1413760381.0