Messages in this thread | | | Date | Wed, 4 Aug 1999 20:51:51 -0700 | From | Mitchell Blank Jr <> | Subject | Re: PATCH for dynamic secondary-groups limit |
| |
Mattias.Gronlund wrote: > The patch does the following: > It removes the "ngroups" and "groups" fields from the task_struct > and adds a new field "sec_groups". This new field is a pointer to > a groups_struct as:
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/
| |