On Thu, Jan 08, 2015 at 11:17:44PM +0100, Lennart Poettering wrote: > On Thu, 08.01.15 14:27, Stéphane Graber (stgra...@ubuntu.com) wrote: > > > This adds a new detect_userns function in virt.c which will check > > whether systemd is running in the host user namespace (single map of all > > available uids and gids) or is using a uid/gid map. > > > > The check makes sure that uid_map and gid_map are both exactly equal to > > the default host map (assuming 32bit uid_t) for a process running in the > > host namespace. > > Hmm, so I think detecting userns is useful, and we probably should add > that as one of the checks to detect_continer(), as a baseline > container implementation check or so. > > However, for the OOM adjust setting I'd really keep things simple, and > just gracefully skip over things on any kind of EPERM, not just those > related to userns. I mean, one day selinux or appormor or some other > MAC might want to prohibit access to that too, and we shouldn't have > to whitelist every single one of them, given how minor and relatively > irrelevant the oom adjustment stuff is. > > Since the fix for that is trivial I now made the check for it, see git > d5243d628624038567c576e9b69c1d775eb05a05, please test. > > I'd still be interested in getting the detect_userns() stuff into > detect_container() as a fallback in some form, hence some comments on > that:
Looks good, I'll give it a try in a tiny bit (with your subsequent fix).
I sent a V2 of my 1/2 patch that should fix most concerns and be a
re-usable for anyone who wants to expand detect_container.
>
> > ---
> > src/shared/virt.c | 22 ++++++++++++++++++++++
> > src/shared/virt.h | 1 +
> > 2 files changed, 23 insertions(+)
> >
> > diff --git a/src/shared/virt.c b/src/shared/virt.c
> > index f10baab..3d94e1f 100644
> > --- a/src/shared/virt.c
> > +++ b/src/shared/virt.c
> > @@ -363,3 +363,25 @@ int detect_virtualization(const char **id) {
> >
> > return VIRTUALIZATION_NONE;
> > }
> > +
> > +/* Detect whether we run in a uid/gid shifted namespace */
> > +int detect_userns(void) {
> > + int r;
> > + static const char host_id_map[] = " 0 0
> > 4294967295";
>
> Not a fan of "static const char[]" where a #define would do too. Also,
> would prefer parsing the values with sscanf() into numbers rather than
> checking the byte-by-byte string...
V2 fixes that.
>
> > + char *uid_map = NULL;
> > + char *gid_map = NULL;
>
> Something needs to free these, consider using "_cleanup_free_" for this.
And that.
I actually had them as _cleanup_free_ originally but that got dropped
when I was testing the code outside of systemd (I forgot to re-introduce
before submitting...).
>
> > +
> > + /* Check if we are uid-shifted */
> > + r = read_one_line_file("/proc/self/uid_map", &uid_map);
> > + if (r == 0 && !streq(uid_map, host_id_map))
> > + return 1;
> > +
> > + /* Check if we are gid-shifted */
> > + r = read_one_line_file("/proc/self/gid_map", &gid_map);
> > + if (r == 0 && !streq(gid_map, host_id_map))
> > + return 1;
> > +
> > + /* If both uid_map and gid_map don't exist or if they both match
> > + * the full uid/gid range, then we're not inside a user namespace
> > */
> > + return 0;
> > +}
>
> Also, I think the code should return errors when read_one_line_file()
> fails, except when it gets ENOENT, since that is an indication that
> userns is simply not enabled in the kernel.
V2 doesn't have that, but that's a fair point and should indeed be done
to catch any weirdness (LSMs or severe kernel bug are the only two I can
think of for that case).
>
> Lennart
>
> --
> Lennart Poettering, Red Hat
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
signature.asc
Description: Digital signature
_______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
