Yay.
> You can go and fetch the patch at:
> http://www.sdf.se/~eldmgr/dynamic_groups.patch.2.3.12
OK, here's my review:
1. In alloc_groups_struct() you make an unchecked memory allocation
(either a kmalloc, __get_free_page, or vmalloc). You do
correctly check for a NULL return in sys_setgroups, but not
before alloc_groups_struct() has already stepped on the NULL.
2. In free_groups_struct() you compute "size" based on:
size = sizeof(struct groups_struct) + gs->ngroups * sizeof(gid_t);
I think you mean gs->max_ngroups there. The way you have it now
the memory could be freed using a different method than it was
allocated from if a process setgroups() a huge list and then
later shrank the list to just a few entries.
3. When you allocate superhuge lists of groups (i.e. with vmalloc)
you might as well round up max_ngroups to just fit into that
many PAGE_SIZE's. This would greatly benefit the case of
incrementally adding more groups to a large list.
4. Why allow ->sec_groups to be NULL? Instead why not make a
special groups_struct to reflect that:
struct groups_struct empty_groups_struct = { 1, 0, 0 };
This would remove a lot of checks against NULL in your code and
may speed up in_groups (it adds a cache-hit memory read in the
case of sec_groups being empty and needing to be consulted, but
that shouldn't be common case; it saves a branch in the more
common case)
The only two concrens I can see are:
* you might need to do something fancier to initialize the atomic_t -
I'm sure there's some macro for initializing atomic's in
static initializers.
* the atomic increment and decrement of empty_groups_stuct.count may
cause that cache line to bounce between processors on SMP boxes
under very heavy process creation load. If that's a concern then
just add a check for gs==&free_groups_struct in fork and
free_groups_struct() and don't do the increment/decrement.
Note that in this case empty_groups_struct.count should be
initialized to something other than 1 so that sys_setgroups()
doesn't try to grab it.
> 1) I use atomic for count, but it will fail if the process calling
> fork is able to die before the fork is finished.
Basically, you need to make sure that by the time you increment ->count
that we are sure to go through release() and vice versa. I'm not
100% sure you have that right, but I'm not convinced it's definately
wrong either. I don't have a copy of .12 handy to check at the moment.
-Mitch
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/