[lkml]   [2017]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 2/3, V2] kernel: Move groups_sort to the caller of set_groups.
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,

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.


 \ /
  Last update: 2017-12-19 17:30    [W:0.165 / U:1.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site