J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> - Finally, we put_cred(override_creds(new)). That modifies
> current->cred again, putting the old value and getting the
> new.
>
> Hm. But that last part's not OK; aren't we still holding our own
> reference to new, in addition to the one that override_creds() just
> took? So I think we need the following?
Yes, you're right. override_creds() takes an extra ref on the argument it is
passed, thus leaving the caller with their original reference intact.
So really, you don't want to call override_creds() as that will cost you an
extra atomic_inc() and atomic_dec_and_test(). I recommend you replace:
put_cred(override_creds(new));
with:
revert_creds(new);
I think that should do the right thing. It may look a bit odd, but it'll be
quicker. If you object to using revert_creds)( because of the name, we can
come up with an alternative name.
> 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?
David
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
