On Mon, Dec 04 2017, Thiago Rafael Becker wrote: > On Mon, 4 Dec 2017, NeilBrown wrote: > >> I think you need to add groups_sort() in a few more places. >> Almost anywhere that calls groups_alloc() should be considered. >> net/sunrpc/svcauth_unix.c, net/sunrpc/auth_gss/svcauth_gss.c, >> fs/nfsd/auth.c definitely need it. > > So are any other functions that modify group_info. OK, I think I'll > implement the type detection below as it helps detecting where these > situations are located. > > This may take some time to make sane. I wonder if we shouldn't > accept the first change suggested to fix the corruption detected in > auth.unix.gid while I work on a new set of patches. As we don't seem to be pursuing this possibility is probably isn't very important, but I'd like to point out that the original fix isn't a true fix. It just sorts a shared group_info early. This does not stop corruption. Every time a thread calls set_groups() on that group_info it will be sorted again. The sort algorithm used is the heap sort, and a heap sort always moves elements in the array around - it does not leave a sorted array untouched (unlike e.g. the quick sort which doesn't move anything in a sorted array). So it is still possible for two calls to groups_sort() to race. We *need* to move groups_sort() out of set_groups(). Thanks, NeilBrown > Also, that patch > doesn't change behavior of set_groups, and is easier to backport if > distros relying on older kernels need to do so and change behavior. The > first suggestion is undergoing tests, and so far we didn't detect any > new corruptions on auth.unix.gid. > >> Maybe it could be done with types. > > I changed the interfaces on groups_{alloc,sort} to check. There are some > extra changes needed in groups_from_user and others to make this viable, > but I like it and I'll try to make it happen. > >> Thanks, >> NeilBrown >> > > Thanks, > trbecker
Attachment:
signature.asc
Description: PGP signature