Re: [PATCH 2/3, V2] kernel: Move groups_sort to the caller of set_groups.

[NeilBrown]

On Tue, Dec 19 2017, J. Bruce Fields wrote:

> On Tue, Dec 05, 2017 at 07:11:00AM +1100, NeilBrown wrote:
>> 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().
>
> By the way,
>
> 	https://bugzilla.kernel.org/show_bug.cgi?id=197887
>
> looks like it might be this bug.  They report it started to happen on
> upgrade from a 4.10-ish kernel to a 4.13-ish kernel, which would include
> the commit (b7b2562f725) that converted groups_sort to a function that
> is no longer a no-op in the already-sorted case.
>
> Looks like rpc.mountd just uses getgrouplist(), and I don't think that
> guarantees any particular oder.  I wonder if it's the case that many
> common configurations always pass down an already-sorted list.  In that
> case this may show up as a 4.13 regression for some users.

I think the 4.13 patch makes the problem worse, but isn't the only
cause.  We had this reported against SLE12 which is based on 3.12 and
doesn't have that patch backported.

Before 4.13 the problem only occurs if getgrouplist() returns an
unsorted list, and if two threads both try to sort this list at the same
time they can corrupt it.
If you are using /etc/passwd and /etc/group, then the order of groups
returned by getgrouplist is the order that the groups were added to
/etc/group (or at least, the order they appear in the file).
This will often be numerical order, but site-local policies could easily
result in a different order.

4.13-stable is EOL and the patch has already been queued for 4.14.8.
Greg tried 4.9 and 4.4, but the patch didn't apply.  Is anyone
volunteering to do the backport???

Thanks,
NeilBrown

Attachment: signature.asc
Description: PGP signature