On Tue, Jan 20, 2009 at 06:23:04PM +0300, Evgeniy Polyakov wrote: > On Tue, Jan 20, 2009 at 10:11:25AM -0500, J. Bruce Fields (bfields@xxxxxxxxxxxx) wrote: > > This doesn't look familiar, no; thanks for the report. I guess we > > should take a careful look at the recent changes to fs/nfsd/auth.c? > > If creds are allocated in nfsd_setuser() and never freed? groups_alloc() > can also explain size-256 slab grew, so this may be the place where > things are allocated, but why they are not freed? > This may also explain why I did not see this for the large sequential > IO, since number of requests to the server was noticebly smaller, than > in random IO test. 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. But that's unrelated. For the cred reference counting: - revert_creds(get_cred(current->real_cred)) modifies current->cred, putting the old value and getting the new. OK. - new = prepare_creds() creates a new object with count 1. All subsequent exits from the function go through the oom or error labels, which put new. OK. - Each of the three ALLSQUASH/ROOTSQUASH/else paths create a new reference to a groups_info struct in gi, which is unconditionally assigned to new by set_groups(). set_groups() takes its own reference on gi, then we put the original reference in the following put_group_info(). OK. - 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? --b. diff --git a/fs/nfsd/auth.c b/fs/nfsd/auth.c index c903e04..9966e9e 100644 --- a/fs/nfsd/auth.c +++ b/fs/nfsd/auth.c @@ -85,6 +85,7 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp) new->cap_effective = cap_raise_nfsd_set(new->cap_effective, new->cap_permitted); put_cred(override_creds(new)); + put_cred(new); return 0; oom: -- 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
