Mitchell Blank Jr wrote:
>
> 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.
Yes, fixing.
> 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.
Yes, fixing.
> 3. When you allocate superhuge lists of groups (i.e. with
> 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.
I can't think of any application that would do that, but it seems as
vmalloc() starts off with size = PAGE_ALIGN(size), so I could as well
do it myself to and calculate max_ngroups out of that size insted.
> 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 };
Because I didn't think of a static struct declaration, thanks.
> 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.
Looks like ATOMIC_INIT should do...
> * 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_structsss 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.
I have implemented this in my new patch, there is a few, but vital
processes that has never been allocated any groups. But I do think that
I
can let count start at one, as sys_setgroups will just reuse it if
max_ngroups is large enough and the initial-groups-struct will have
max_ngroups == 0.
> > 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.
Right, I was more into the problem with mutal-exclution, I think that
the
count is right, it follows the p->user->count so if that one is right
this one should also be right.
I have been thinking about the mutal-exclution problem for a while, and
I
see it this way:
There might be a raise, it could be possible that sys_setgroups would be
called and that it checks the reference-count and decide to reuse the
groups struct and that frok get to be called before the "change" has
taken place, and it may render in that the child process will "change"
groups along with its mother. If it is possible to call fork when
setgroups is called with kernel-threads, then this might happen, but
as I see it it is undefined in the current case to, as it depends on
when the task_struct is copied. sys_setgroup sets ngroup at the end
so there will never be any time when there is undefined data in the
array inside the ngroups range.
There still is one bug in this, if a process joins a enough groups and
then reads /proc/self/status it will print all group-ids even if there
isn't space left in the buffer. What to do in this case? The best way
I can see at the moment would be to rewrite the proc-code for
/proc/<pid> to support unlimited length for the "file". What do other
people think?
A new version of the patch can be found at:
http://www.sdf.se/~eldmgr/dynamic_groups_a2.patch.2.3.12
/Mattias
-
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/