Re: NFS/credentials leak in 2.6.29-rc1

[David Howells <dhowells@redhat.com>]

J. Bruce Fields <bfields@fieldses.org> wrote:

> If the only difference is just whether it takes a reference on the
> passed-in cred it might be clearest just to write
> 
> 	set_creds(new);
> 
> or
> 	set_creds(get_creds(new));
> 
> depending on which you want?

The former would be preferable, if it transfers the reference on the creds to
the task_struct, thus eliminating the need for a put_cred().

> In any case, yes, the revert_creds()/override_creds() names don't tell
> me much.
> 
> > > Looking through nfsd_setuser(), one obvious bug: in the (flags &
> > > NFSEXP_ALLSQUASH) case, we never check the return value from the
> > > groups_alloc(0).  If it returns NULL, we dereference it anyway.
> > 
> > Since a zero-length groups list must be copied before writing, can I recommend
> > that we make groups_alloc(0) a special case that returns pointer to a
> > statically allocated groups list (after inc'ing the refcount) that represents
> > a zero-length list, thus meaning groups_alloc(0) will never fail?
> 
> Is there a really big advantage to that?  On the face of it it strikes
> me as a weird corner case that I'll trip over every time I look at this
> code.

It'll remove a potential OOM condition.  It's a minor optimisation, I think.

David