Re: [PATCH v2] kernel: Conditionally support non-root users, groups and capabilities

[josh@joshtriplett.org]

On Thu, Jan 29, 2015 at 07:59:09PM +0100, Geert Uytterhoeven wrote:
> Hi Iulia,
> 
> On Thu, Jan 29, 2015 at 7:43 PM, Iulia Manda <iulia.manda21@gmail.com> wrote:
> > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> > index 68b68d7..b2d2116 100644
> > --- a/arch/s390/Kconfig
> > +++ b/arch/s390/Kconfig
> > @@ -324,6 +324,7 @@ config COMPAT
> >         select COMPAT_BINFMT_ELF if BINFMT_ELF
> >         select ARCH_WANT_OLD_COMPAT_IPC
> >         select COMPAT_OLD_SIGACTION
> > +       select NON_ROOT
> 
> > @@ -10,6 +10,7 @@ config LUSTRE_FS
> 
> > +       select NON_ROOT
> 
> > @@ -6,6 +6,7 @@ config NFSD
> 
> > +       select NON_ROOT
> 
> >  config BSD_PROCESS_ACCT
> >         bool "BSD Process Accounting"
> > +       select NON_ROOT
> 
> >  config TASKSTATS
> 
> > +       select NON_ROOT
> 
> Is there a specific reason why you chose to use "select NON_ROOT"
> instead of "depends on NON_ROOT" for all these options?
> As configuring NON_ROOT=n is quite a drastic decision, I don't
> think you should let that be revertable such easily by all those selects.

In the past, there's been quite a bit of negative feedback about
"depends on", because that makes various options invisible and
un-enableable.  (Kconfig can be awkward that way.)  However, I think
it'd be perfectly reasonable to make all of these "depends on NON_ROOT"
instead, if there aren't any objections to doing so.

> > @@ -1140,6 +1142,7 @@ config CHECKPOINT_RESTORE
> >
> >  menuconfig NAMESPACES
> >         bool "Namespaces support" if EXPERT
> > +       depends on NON_ROOT
> 
> > @@ -1352,11 +1355,25 @@ menuconfig EXPERT
> >
> >  config UID16
> >         bool "Enable 16-bit UID system calls" if EXPERT
> > -       depends on HAVE_UID16
> > +       depends on HAVE_UID16 && NON_ROOT
> 
> Ah, finally a few "depends on".
> 
> > +config NON_ROOT
> > +       bool "Multiple users, groups and capabilities support" if EXPERT
> > +       default y
> > +       help
> > +         This option enables support for non-root users, groups and
> > +         capabilities.
> > +
> > +         If you say N here, all processes will run with UID 0, GID 0, and all
> > +         possible capabilities.  Saying N here also compiles out support for
> > +         system calls related to UIDs, GIDs, and capabilities, such as setuid,
> > +         setgid, and capset.
> > +
> > +         If unsure, say Y here.
> 
> I think it would be clearer to use positive instead of negative logic.
> What about calling the option "MULTIUSER" instead of "NON_ROOT"?

Nice name idea; reminiscent of Multics versus UNIX.

The original motivation for CONFIG_NON_ROOT was to ensure that 'y' was
the option that added code to the kernel, so that "allnoconfig" does the
right thing.  As long as the logic stays that way around, changing the
name of the option seems perfectly fine.

(As long as we're bikeshedding: CONFIG_MULTIUSER or CONFIG_MULTI_USER?)

- Josh Triplett