(sorry if this dups, screwup in my aliases file)
Linus,
Attached is a patch to remove the NGROUPS limit (again). This patch is
against linux-2.6.2rc2. If it is OK, I can make it bitkeeper accessible.
This does not use vmalloc, but instead uses an array of pages. It also will
require a glibc patch which I will pop off as soon as I get the OK on this
(I've been reworking this every few months for about 2 years now).
It does duplicate some code in the compat code for some architectures. I'll
follow this up with a cleanup patch for that, I promise.
This patch changes the security interface for task_setgroups().
This throughly breaks intermezzo, though I don't know if anyone will notice.
What think? Can we get rid of this limit at long last? :)
Tim
On Mon, Sep 29, 2003 at 04:10:23PM -0700, Linus Torvalds wrote:
> I'm definitely happier about this one._
>_
> Not that I'm any more thrilled about users using thousands of groups. But_
> this looks a bit saner.
--
Tim Hockin
Sun Microsystems, Linux Software Engineering
[email protected]
All opinions are my own, not Sun's
On Tue, 2004-01-27 at 15:53, Tim Hockin wrote:
> What think? Can we get rid of this limit at long last? :)
Aren't the Samba folks asking for this as well? People using Samba v3
to replace NT4 domains where commonly users belong to many more than 32
groups.
Dax
* Tim Hockin ([email protected]) wrote:
> This patch changes the security interface for task_setgroups().
Minor fixup would be needed for SELinux.
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
===== security/selinux/hooks.c 1.20 vs edited =====
--- 1.20/security/selinux/hooks.c Tue Jan 20 17:58:48 2004
+++ edited/security/selinux/hooks.c Tue Jan 27 15:42:41 2004
@@ -2265,7 +2265,7 @@
return task_has_perm(current, p, PROCESS__GETSESSION);
}
-static int selinux_task_setgroups(int gidsetsize, gid_t *grouplist)
+static int selinux_task_setgroups(struct group_info *group_info)
{
/* See the comment for setuid above. */
return 0;
* Tim Hockin ([email protected]) wrote:
> +/* validate and set current->group_info */
> +int set_current_groups(struct group_info *info)
> +{
> + int retval;
> + struct group_info *old_info;
> +
> + retval = security_task_setgroups(info);
> + if (retval)
> + return retval;
> +
> + groups_sort(info);
> + old_info = current->group_info;
> + current->group_info = info;
> + put_group_info(old_info);
> +
> + return 0;
> +}
<snip>
> ===== fs/proc/array.c 1.55 vs edited =====
> --- 1.55/fs/proc/array.c Tue Oct 14 14:00:09 2003
> +++ edited/fs/proc/array.c Tue Jan 27 12:40:02 2004
> @@ -176,8 +176,8 @@
> p->files ? p->files->max_fds : 0);
> task_unlock(p);
>
> - for (g = 0; g < p->ngroups; g++)
> - buffer += sprintf(buffer, "%d ", p->groups[g]);
> + for (g = 0; g < min(p->group_info->ngroups,NGROUPS_SMALL); g++)
> + buffer += sprintf(buffer, "%d ", GROUP_AT(p->group_info,g));
>
> buffer += sprintf(buffer, "\n");
> return buffer;
this seems racy with no get/put?
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
On Tue, Jan 27, 2004 at 04:46:15PM -0800, Andrew Morton wrote:
> rant. We have soooo many syscalls declared in .c files. We had a bug due
> to this a while back. Problem is, we have no anointed header in which to
> place them. include/linux/syscalls.h would suit. And unistd.h for
> arch-specific syscalls. But that's not appropriate to this patch.
I did that in the linux-abi patch for 2.4 and even submitted it a few
times before 2.5 forked, but Linus didn't seem to like it.
Tim Hockin <[email protected]> wrote:
>
> Attached is a patch to remove the NGROUPS limit (again).
+/* export the group_info to a user-space array */
+static int groups_to_user(gid_t *grouplist, struct group_info __user *info)
+{
+ int i;
+ int count = info->ngroups;
+
+ for (i = 0; i < info->nblocks; i++) {
+ int cp_count = min(NGROUPS_BLOCK, count);
+ int off = i * NGROUPS_BLOCK;
+ int len = cp_count * sizeof(*grouplist);
+
+ if (copy_to_user(grouplist+off, info->blocks[i], len))
+ return -EFAULT;
+
This had me thorougly confused for a while ;) The __user tag here should
apply to grouplist, not to info.
+static int groups16_to_user(old_gid_t __user *grouplist,
+ struct group_info *info)
+{
+ int i;
+ old_gid_t group;
+
+ if (info->ngroups > TASK_SIZE/sizeof(group))
+ return -EFAULT;
+ if (!access_ok(VERIFY_WRITE, grouplist, info->ngroups * sizeof(group)))
+ return -EFAULT;
Why are many functions playing with TASK_SIZE?
--- 1.2/fs/nfsd/auth.c Tue Jun 17 16:31:29 2003
+++ edited/fs/nfsd/auth.c Tue Jan 27 12:40:02 2004
@@ -10,12 +10,15 @@
#include <linux/sunrpc/svcauth.h>
#include <linux/nfsd/nfsd.h>
+extern asmlinkage long sys_setgroups(int gidsetsize, gid_t *grouplist);
+
rant. We have soooo many syscalls declared in .c files. We had a bug due
to this a while back. Problem is, we have no anointed header in which to
place them. include/linux/syscalls.h would suit. And unistd.h for
arch-specific syscalls. But that's not appropriate to this patch.
On Tue, Jan 27, 2004 at 04:46:15PM -0800, Andrew Morton wrote:
> > Attached is a patch to remove the NGROUPS limit (again).
> +/* export the group_info to a user-space array */
> +static int groups_to_user(gid_t *grouplist, struct group_info __user *info)
> This had me thorougly confused for a while ;) The __user tag here should
> apply to grouplist, not to info.
indeed.
> +static int groups16_to_user(old_gid_t __user *grouplist,
> + struct group_info *info)
> +{
> + int i;
> + old_gid_t group;
> +
> + if (info->ngroups > TASK_SIZE/sizeof(group))
> + return -EFAULT;
> + if (!access_ok(VERIFY_WRITE, grouplist, info->ngroups * sizeof(group)))
> + return -EFAULT;
>
> Why are many functions playing with TASK_SIZE?
Not sure - I thought it was maybe a paranoid check, Rusty included it in his
version of a similar patch a while ago.
> +extern asmlinkage long sys_setgroups(int gidsetsize, gid_t *grouplist);
>
> rant. We have soooo many syscalls declared in .c files. We had a bug due
> to this a while back. Problem is, we have no anointed header in which to
> place them. include/linux/syscalls.h would suit. And unistd.h for
> arch-specific syscalls. But that's not appropriate to this patch.
Agreed.
In message <[email protected]> you write:
> (sorry if this dups, screwup in my aliases file)
>
> Linus,
You should probably send to Andrew Morton, too (or instead).
For the record; it's overkill for what I need (hundreds, not thousands
of groups, and I don't really mind if accessing them is slow), but at
this point I'd just like *some* solution.
Thanks Tim!
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
On Tue, Jan 27, 2004 at 02:53:11PM -0800, Tim Hockin wrote:
For the every declaration of struct group_info *info, is there a
more descriptive name than info available. This will make
searchs simpler in the long run.
+#define NGROUPS_SMALL 32
+#define NGROUPS_BLOCK ((int)(EXEC_PAGESIZE / sizeof(gid_t)))
Can this be changed to NGROUPS_PER_EPAGE. NGROUPS_BLOCK seems to
indicate a location of a block or the block or some block given an offset.
NGROUPS_PER_EPAGE is very explicit.
+void groups_free(struct group_info *info)
{
+ if (info->ngroups > NGROUPS_SMALL) {
Why not use info->nblocks > 0. These are more clearly tied to each
other than ngroups and NGROUPS_SMALL.
+ while (left >= 0 && GROUP_AT(info, left) > tmp) {
+ GROUP_AT(info, right) = GROUP_AT(info, left);
+ left -= stride;
+ right = left + stride;
For the above in groups_sort, why not just have
right = stride;
left -= stride;
@@ -1102,54 +1256,43 @@
if (gidsetsize < 0)
return -EINVAL;
- i = current->ngroups;
+ i = current->group_info->ngroups;
if (gidsetsize) {
if (i > gidsetsize)
return -EINVAL;
- if (copy_to_user(grouplist, current->groups, sizeof(gid_t)*i))
+ if (groups_to_user(grouplist, current->group_info))
return -EFAULT;
}
Shouldn't there be a get/put pair around this. Especially with the
copy_to_user call buried in groups_to_user, we could spend quite some time
waiting for the page fault and another thread could free our structure.
Or maybe I am missing something...
+ if (info->ngroups > TASK_SIZE/sizeof(group))
+ return -EFAULT;
I don't understand the TASK_SIZE usage. Maybe this is a difference
between ia64 and i386, but these checks don't make any sense to
me. Consider them not looked at.
asmlinkage long sys_getgroups16(int gidsetsize, old_gid_t __user *grouplist)
{
- old_gid_t groups[NGROUPS];
- int i,j;
+ int i = 0;
if (gidsetsize < 0)
return -EINVAL;
- i = current->ngroups;
+ i = current->group_info->ngroups;
if (gidsetsize) {
if (i > gidsetsize)
return -EINVAL;
- for(j=0;j<i;j++)
- groups[j] = current->groups[j];
- if (copy_to_user(grouplist, groups, sizeof(old_gid_t)*i))
+ if (groups16_to_user(grouplist, current->group_info))
Again with the get/put.
===== fs/proc/array.c 1.55 vs edited =====
--- 1.55/fs/proc/array.c Tue Oct 14 14:00:09 2003
+++ edited/fs/proc/array.c Tue Jan 27 12:40:02 2004
@@ -176,8 +176,8 @@
p->files ? p->files->max_fds : 0);
task_unlock(p);
- for (g = 0; g < p->ngroups; g++)
- buffer += sprintf(buffer, "%d ", p->groups[g]);
+ for (g = 0; g < min(p->group_info->ngroups,NGROUPS_SMALL); g++)
+ buffer += sprintf(buffer, "%d ", GROUP_AT(p->group_info,g));
buffer += sprintf(buffer, "\n");
return buffer;
Again with the get/put.
All of the sunrpc mods appear to need get/put.
On Tue, 27 Jan 2004, Tim Hockin wrote:
> On Tue, Jan 27, 2004 at 04:46:15PM -0800, Andrew Morton wrote:
> > +
> > + if (info->ngroups > TASK_SIZE/sizeof(group))
> > + return -EFAULT;
> > + if (!access_ok(VERIFY_WRITE, grouplist, info->ngroups * sizeof(group)))
> > + return -EFAULT;
> >
> > Why are many functions playing with TASK_SIZE?
>
> Not sure - I thought it was maybe a paranoid check, Rusty included it in his
> version of a similar patch a while ago.
Yes, a necessary paranoid check: without it, info->ngroups * sizeof(group)
can easily wrap to something small, and access_ok pass when it should fail.
Hugh
On Wed, 28 Jan 2004, Hugh Dickins wrote:
>
> Sorry, I should have looked further. info->ngroups is an "int", so
> if this check is needed, a check for negativity (or unsigned cast)
> would also be needed.
Nope - there's an implied cast by virtue of the right side being unsigned
in the comparison already.
Although I do believe that it would be better written as
#define MAXGROUPS (1000) /* Arbitrary, but we have to limit it somehere */
if ((unsigned) info->ngroups > MAXGROUPS)
return -ETOOEFFINGLARGE;
as I absolutely _despise_ code that tries to be too generic.
What is it with CS classes that have removed "common sense" from the
equation?
Linus
On Wed, 28 Jan 2004, Hugh Dickins wrote:
> On Tue, 27 Jan 2004, Tim Hockin wrote:
> > On Tue, Jan 27, 2004 at 04:46:15PM -0800, Andrew Morton wrote:
> > > +
> > > + if (info->ngroups > TASK_SIZE/sizeof(group))
> > > + return -EFAULT;
> > > + if (!access_ok(VERIFY_WRITE, grouplist, info->ngroups * sizeof(group)))
> > > + return -EFAULT;
> > >
> > > Why are many functions playing with TASK_SIZE?
> >
> > Not sure - I thought it was maybe a paranoid check, Rusty included it in his
> > version of a similar patch a while ago.
>
> Yes, a necessary paranoid check: without it, info->ngroups * sizeof(group)
> can easily wrap to something small, and access_ok pass when it should fail.
Sorry, I should have looked further. info->ngroups is an "int", so
if this check is needed, a check for negativity (or unsigned cast)
would also be needed. But it shouldn't be needed in the copy to user
cases, and in the copy from user cases gidsetsize should be checked
much earlier, in or before groups_alloc.
Hugh
On Wed, Jan 28, 2004 at 10:10:02AM -0800, Linus Torvalds wrote:
> Although I do believe that it would be better written as
>
> #define MAXGROUPS (1000) /* Arbitrary, but we have to limit it somehere */
>
> if ((unsigned) info->ngroups > MAXGROUPS)
> return -ETOOEFFINGLARGE;
>
> as I absolutely _despise_ code that tries to be too generic.
>
> What is it with CS classes that have removed "common sense" from the
> equation?
OK, there are two easy answers to this. I can re-work it with a simple 32k
limit that needs to be recompiled to change, or I can add a sysctl to
control it (it appeared in an early version of this patch).
Preference?
--
Tim Hockin
Sun Microsystems, Linux Software Engineering
[email protected]
All opinions are my own, not Sun's