PATCH: report NGROUPS_MAX via a sysctl (read-only)

PATCH: report NGROUPS_MAX via a sysctl (read-only)

[Tim Hockin]

[-- Attachment #1: Type: text/plain, Size: 404 bytes --]

Attached is a simple patch to expose NGROUPS_MAX via sysctl.  Nothing fancy,
just a read-only variable.  glibc can use this to sysconf() the value
properly, so apps will stop relying on NGROUPS_MAX as a real constant.

Is this the right path?  Or should there be a sysconf-specific mechanism?
-- 
Tim Hockin
Sun Microsystems, Linux Software Engineering
thockin@sun.com
All opinions are my own, not Sun's

[-- Attachment #2: ngroups-sysctl-1.diff --]
[-- Type: text/plain, Size: 1277 bytes --]

===== include/linux/sysctl.h 1.64 vs edited =====
--- 1.64/include/linux/sysctl.h	Wed Feb 18 19:43:21 2004
+++ edited/include/linux/sysctl.h	Thu Feb 19 17:00:34 2004
@@ -129,6 +129,7 @@
 	KERN_HPPA_UNALIGNED=59,	/* int: hppa unaligned-trap enable */
 	KERN_PRINTK_RATELIMIT=60, /* int: tune printk ratelimiting */
 	KERN_PRINTK_RATELIMIT_BURST=61,	/* int: tune printk ratelimiting */
+	KERN_NGROUPS_MAX=62,	/* int: NGROUPS_MAX */
 };
 
 
===== kernel/sysctl.c 1.61 vs edited =====
--- 1.61/kernel/sysctl.c	Wed Feb 18 19:43:21 2004
+++ edited/kernel/sysctl.c	Thu Feb 19 17:05:47 2004
@@ -38,6 +38,7 @@
 #include <linux/security.h>
 #include <linux/initrd.h>
 #include <linux/times.h>
+#include <linux/limits.h>
 #include <asm/uaccess.h>
 
 #ifdef CONFIG_ROOT_NFS
@@ -68,6 +69,8 @@
 static int maxolduid = 65535;
 static int minolduid;
 
+static int ngroups_max = NGROUPS_MAX;
+
 #ifdef CONFIG_KMOD
 extern char modprobe_path[];
 #endif
@@ -591,6 +594,14 @@
 		.data		= &printk_ratelimit_burst,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
+		.proc_handler	= &proc_dointvec,
+	},
+	{
+		.ctl_name	= KERN_NGROUPS_MAX,
+		.procname	= "ngroups_max",
+		.data		= &ngroups_max,
+		.maxlen		= sizeof (int),
+		.mode		= 0444,
 		.proc_handler	= &proc_dointvec,
 	},
 	{ .ctl_name = 0 }

Re: PATCH: report NGROUPS_MAX via a sysctl (read-only)

[Andrew Morton]

Tim Hockin <thockin@sun.com> wrote:
>
>  Attached is a simple patch to expose NGROUPS_MAX via sysctl.

Why does userspace actually care?  You try to do an oversized setgroups(),
so you get an error?

And why does NGROUPS_MAX still exist, come to that?  AFAICT the only thing
it does is to prevent users from being able to allocate too much kernel
memory??

Re: PATCH: report NGROUPS_MAX via a sysctl (read-only)

[Tim Hockin]

On Thu, Feb 19, 2004 at 09:30:28PM -0800, Andrew Morton wrote:
> >  Attached is a simple patch to expose NGROUPS_MAX via sysctl.
> 
> Why does userspace actually care?  You try to do an oversized setgroups(),
> so you get an error?

I am systematically tracking down apps that use it.  glibc is almost free of
it.  sysconf() still uses it, but as long as the value compiled into glibc
as NGROUPS_MAX is less-than-or-equal-to the current kernel's idea, it meets
POSIX, right?  If any one goes into their kernel source and lowers
NGROUPS_MAX they might break things, but I guess that isn't too big of a
worry.  Some apps are still assuming that the value they get from sysconf()
is the absolute max number of groups.  Anyone with libc compiled against an
older kernel will see 32, when they could have 64k.

> And why does NGROUPS_MAX still exist, come to that?  AFAICT the only thing

Because Linus would not let me set it to INT_MAX. Something about
"insanity" ;)

As long as we can convince userspace apps to not trust sysconf() or
NGROUPS_MAX constants, I guess we don't *need* it exposed.  I guess that's a
veto, then? 

-- 
Tim Hockin
Sun Microsystems, Linux Software Engineering
thockin@sun.com
All opinions are my own, not Sun's

Re: PATCH: report NGROUPS_MAX via a sysctl (read-only)

[Andrew Morton]

Tim Hockin <thockin@sun.com> wrote:
>
> On Thu, Feb 19, 2004 at 09:30:28PM -0800, Andrew Morton wrote:
> > >  Attached is a simple patch to expose NGROUPS_MAX via sysctl.
> > 
> > Why does userspace actually care?  You try to do an oversized setgroups(),
> > so you get an error?
> 
> I am systematically tracking down apps that use it.  glibc is almost free of
> it.  sysconf() still uses it, but as long as the value compiled into glibc
> as NGROUPS_MAX is less-than-or-equal-to the current kernel's idea, it meets
> POSIX, right?  If any one goes into their kernel source and lowers
> NGROUPS_MAX they might break things, but I guess that isn't too big of a
> worry.  Some apps are still assuming that the value they get from sysconf()
> is the absolute max number of groups.  Anyone with libc compiled against an
> older kernel will see 32, when they could have 64k.

OK, well certainly fishing the number out of the currently-running kernel
is the one sure way of getting it right.

> > And why does NGROUPS_MAX still exist, come to that?  AFAICT the only thing
> 
> Because Linus would not let me set it to INT_MAX. Something about
> "insanity" ;)

Is 64k enough?

Re: PATCH: report NGROUPS_MAX via a sysctl (read-only)

[Tim Hockin]

On Thu, Feb 19, 2004 at 10:47:52PM -0800, Andrew Morton wrote:
> > > Why does userspace actually care?  You try to do an oversized setgroups(),
> > > so you get an error?
> > 
> > I am systematically tracking down apps that use it.  glibc is almost free of
> > it.  sysconf() still uses it, but as long as the value compiled into glibc
> > as NGROUPS_MAX is less-than-or-equal-to the current kernel's idea, it meets
> > POSIX, right?  If any one goes into their kernel source and lowers
> > NGROUPS_MAX they might break things, but I guess that isn't too big of a
> > worry.  Some apps are still assuming that the value they get from sysconf()
> > is the absolute max number of groups.  Anyone with libc compiled against an
> > older kernel will see 32, when they could have 64k.
> 
> OK, well certainly fishing the number out of the currently-running kernel
> is the one sure way of getting it right.

Well, really I don't see how apps would want to use it in any way that was
correct.  You can't use it as the size of an array.  If it WERE defined as
INT_MAX...  :)

On the other hand, some obsessive-compulsive part of me says that a constant
like that SHOULD be exposed.  Which is why I asked about doing either a
sys_sysconf() call to expose those, or adding something to
sysfs/procfs/somethingfs to gather all the sysconf-mandated constants and
maybe other kernel constants.

> > > And why does NGROUPS_MAX still exist, come to that?  AFAICT the only thing
> > 
> > Because Linus would not let me set it to INT_MAX. Something about
> > "insanity" ;)
> 
> Is 64k enough?

64k makes our users happy.  It was a concession that had to be made.  Now
that the infrastructure for allocating, sorting, and searching is in place,
any user can change NGROUPS_MAX to 128k or 256k or INT_MAX and recompile -
no other work needed.  That's far better than where we were.

Of course, if we DON'T expose things like NGROUPS_MAX, then userspace won't
know if the user changes it to 256k.  But then again, what would they be
using it for that would not be wrong?  I'm not sure it is for the kernel to
say "you don't need to know this".

Re: PATCH: report NGROUPS_MAX via a sysctl (read-only)

[Glen Turner]

On Fri, 2004-02-20 at 17:40, Tim Hockin wrote:

> Well, really I don't see how apps would want to use it in any way that was
> correct.

The mere existence of the value means it can be used correctly
in application code for sanity checking.

eg:
   assert(list_length(group_list) < ngroups_max());
   list_append(&group_list, group);

An application might also use it to automatically
size data structure details, such as the parameters of
a hash function.

  h = hash_create(ngroups_max());

Returning INT_MAX for NGROUPS_MAX isn't wrong, but you
then can't blame user space for making inefficient choices
if the kernel limit is actually smaller.

Cheers,
Glen

Re: PATCH: report NGROUPS_MAX via a sysctl (read-only)

[Tim Hockin]

On Wed, Feb 25, 2004 at 11:29:27AM +1030, Glen Turner wrote:
> The mere existence of the value means it can be used correctly
> in application code for sanity checking.

Andrew seems to have picked it up for -mm.  I sent a patch to glibc-bugs (is
that the right place?  any libc folk?) to use the new sysctl in
sysconf(_SC_NGROUPS_MAX).  So if/when all those releases converge, you
should be able to get the kernel's idea of NGROUPS_MAX from
sysconf(_SC_NGROUPS_MAX);

> Returning INT_MAX for NGROUPS_MAX isn't wrong, but you
> then can't blame user space for making inefficient choices
> if the kernel limit is actually smaller.

Well, if the kernel HAS no limit, then NGROUPS_MAX really is INT_MAX.  There
are not many legit uses I can think of for userspace to actually care about
NGROUPS_MAX.  Just the current number of groups, which is easily gotten via
setgroups().

Now that the sysctl is in, it's a very tiny patch to make ngroups_max
actually raisable/ by sysctl, though it is debatable whether it is useful AT
ALL, and dubious whether it is safe to lower that value after any apps are
running.

-- 
Tim Hockin
thockin@hockin.org
Soon anyone who's not on the World Wide Web will qualify for a government 
subsidy for the home-pageless.

Re: PATCH: report NGROUPS_MAX via a sysctl (read-only)

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Tim Hockin wrote:
> I sent a patch to glibc-bugs (is that the right place?

No. libc-alpha (at) sources.redhat.com.

- -- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3 (GNU/Linux)

iD8DBQFAPE+k2ijCOnn/RHQRAsNoAJ9rzW4nh8e5t1b8pDu/Ad7coTl71QCgiLhT
qYs6dwTrImTJOZc0JK88z/Q=
=8CR9
-----END PGP SIGNATURE-----