| Message ID | 1486625901-10094-1-git-send-email-dwindsor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
<snip> > Signed-off-by: David Windsor <dwindsor@gmail.com> > --- > fs/nfsd/nfs4state.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index a0dee8a..b0f3010 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct nfsd4_session *ses) > > lockdep_assert_held(&nn->client_lock); > > - if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses)) > + if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_des(ses)) This should read: if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses)) > free_session(ses); > put_client_renew_locked(clp); > } > @@ -1645,7 +1645,7 @@ static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, stru > new->se_flags = cses->flags; > new->se_cb_prog = cses->callback_prog; > new->se_cb_sec = cses->cb_sec; > - atomic_set(&new->se_ref, 0); > + atomic_set(&new->se_ref, 1); > idx = hash_sessionid(&new->se_sessionid); > list_add(&new->se_hash, &nn->sessionid_hashtbl[idx]); > spin_lock(&clp->cl_lock); > @@ -1792,7 +1792,7 @@ free_client(struct nfs4_client *clp) > ses = list_entry(clp->cl_sessions.next, struct nfsd4_session, > se_perclnt); > list_del(&ses->se_perclnt); > - WARN_ON_ONCE(atomic_read(&ses->se_ref)); > + WARN_ON_ONCE((atomic_read(&ses->se_ref) > 1)); > free_session(ses); > } > rpc_destroy_wait_queue(&clp->cl_cb_waitq); > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2017-02-11 at 01:42 -0500, David Windsor wrote: > <snip> > > > Signed-off-by: David Windsor <dwindsor@gmail.com> > > --- > > fs/nfsd/nfs4state.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index a0dee8a..b0f3010 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct nfsd4_session *ses) > > > > lockdep_assert_held(&nn->client_lock); > > > > - if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses)) > > + if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_des(ses)) > > This should read: > if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses)) > > > free_session(ses); > > put_client_renew_locked(clp); > > } > > @@ -1645,7 +1645,7 @@ static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, stru > > new->se_flags = cses->flags; > > new->se_cb_prog = cses->callback_prog; > > new->se_cb_sec = cses->cb_sec; > > - atomic_set(&new->se_ref, 0); > > + atomic_set(&new->se_ref, 1); > > idx = hash_sessionid(&new->se_sessionid); > > list_add(&new->se_hash, &nn->sessionid_hashtbl[idx]); > > spin_lock(&clp->cl_lock); > > @@ -1792,7 +1792,7 @@ free_client(struct nfs4_client *clp) > > ses = list_entry(clp->cl_sessions.next, struct nfsd4_session, > > se_perclnt); > > list_del(&ses->se_perclnt); > > - WARN_ON_ONCE(atomic_read(&ses->se_ref)); > > + WARN_ON_ONCE((atomic_read(&ses->se_ref) > 1)); > > free_session(ses); > > } > > rpc_destroy_wait_queue(&clp->cl_cb_waitq); > > -- > > 2.7.4 > > The basic idea here is that nfsv4 sessions have a "resting state" of 0. We want to keep them around, but if they go "dead" then we we'll tear them down if they aren't actively in use at the time. So, we still free the thing when the refcount goes to zero, but we have an extra condition before we free it on the put -- that the session is also "dead" (meaning that the client asked us to destroy it). Your patch doesn't look like it'll break anything, but I personally find it harder to follow that way. The freeable reference state will be 1 instead of the normal 0. -- Jeff Layton <jlayton@poochiereds.net> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 11, 2017 at 7:31 AM, Jeff Layton <jlayton@poochiereds.net> wrote: > On Sat, 2017-02-11 at 01:42 -0500, David Windsor wrote: >> <snip> >> >> > Signed-off-by: David Windsor <dwindsor@gmail.com> >> > --- >> > fs/nfsd/nfs4state.c | 6 +++--- >> > 1 file changed, 3 insertions(+), 3 deletions(-) >> > >> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >> > index a0dee8a..b0f3010 100644 >> > --- a/fs/nfsd/nfs4state.c >> > +++ b/fs/nfsd/nfs4state.c >> > @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct nfsd4_session *ses) >> > >> > lockdep_assert_held(&nn->client_lock); >> > >> > - if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses)) >> > + if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_des(ses)) >> >> This should read: >> if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses)) >> >> > free_session(ses); >> > put_client_renew_locked(clp); >> > } >> > @@ -1645,7 +1645,7 @@ static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, stru >> > new->se_flags = cses->flags; >> > new->se_cb_prog = cses->callback_prog; >> > new->se_cb_sec = cses->cb_sec; >> > - atomic_set(&new->se_ref, 0); >> > + atomic_set(&new->se_ref, 1); >> > idx = hash_sessionid(&new->se_sessionid); >> > list_add(&new->se_hash, &nn->sessionid_hashtbl[idx]); >> > spin_lock(&clp->cl_lock); >> > @@ -1792,7 +1792,7 @@ free_client(struct nfs4_client *clp) >> > ses = list_entry(clp->cl_sessions.next, struct nfsd4_session, >> > se_perclnt); >> > list_del(&ses->se_perclnt); >> > - WARN_ON_ONCE(atomic_read(&ses->se_ref)); >> > + WARN_ON_ONCE((atomic_read(&ses->se_ref) > 1)); >> > free_session(ses); >> > } >> > rpc_destroy_wait_queue(&clp->cl_cb_waitq); >> > -- >> > 2.7.4 >> > > > The basic idea here is that nfsv4 sessions have a "resting state" of 0. > We want to keep them around, but if they go "dead" then we we'll tear > them down if they aren't actively in use at the time. So, we still free > the thing when the refcount goes to zero, but we have an extra condition > before we free it on the put -- that the session is also "dead" (meaning > that the client asked us to destroy it). > > Your patch doesn't look like it'll break anything, but I personally find > it harder to follow that way. The freeable reference state will be 1 > instead of the normal 0. > I'm not sure there's another way to accomplish what we need (initializing struct nfsd4_session objects with refcount=1) without also modifying the freeable reference state. After migrating to the refcount_t API, if we leave init_session() as is, the first call to nfsd4_get_session_locked() will fail: static __be32 nfsd4_get_session_locked(struct nfsd4_session *ses) { __be32 status; if (is_session_dead(ses)) return nfserr_badsession; status = get_client_locked(ses->se_client); if (status) return status; refcount_inc(&ses->se_ref); /* This fails and WARNS when ses->se_ref is 0. */ return nfs_ok; } The refcount_t API patches aren't yet merged, so this discussion is a bit limited in that respect, but refcount_inc() WARNS when called with a refcount_t object whose value is 0, as this may represent a use-after-free attempt. Given this, I'm unsure how it's possible to achieve initialization of struct nfsd4_session objects with refcount=1 while still maintaining these objects' "rest state" at refcount=0. > -- > Jeff Layton <jlayton@poochiereds.net> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2017-02-11 at 09:01 -0500, David Windsor wrote: > On Sat, Feb 11, 2017 at 7:31 AM, Jeff Layton <jlayton@poochiereds.net> wrote: > > On Sat, 2017-02-11 at 01:42 -0500, David Windsor wrote: > > > <snip> > > > > > > > Signed-off-by: David Windsor <dwindsor@gmail.com> > > > > --- > > > > fs/nfsd/nfs4state.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > > > index a0dee8a..b0f3010 100644 > > > > --- a/fs/nfsd/nfs4state.c > > > > +++ b/fs/nfsd/nfs4state.c > > > > @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct nfsd4_session *ses) > > > > > > > > lockdep_assert_held(&nn->client_lock); > > > > > > > > - if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses)) > > > > + if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_des(ses)) > > > > > > This should read: > > > if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses)) > > > > > > > free_session(ses); > > > > put_client_renew_locked(clp); > > > > } > > > > @@ -1645,7 +1645,7 @@ static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, stru > > > > new->se_flags = cses->flags; > > > > new->se_cb_prog = cses->callback_prog; > > > > new->se_cb_sec = cses->cb_sec; > > > > - atomic_set(&new->se_ref, 0); > > > > + atomic_set(&new->se_ref, 1); > > > > idx = hash_sessionid(&new->se_sessionid); > > > > list_add(&new->se_hash, &nn->sessionid_hashtbl[idx]); > > > > spin_lock(&clp->cl_lock); > > > > @@ -1792,7 +1792,7 @@ free_client(struct nfs4_client *clp) > > > > ses = list_entry(clp->cl_sessions.next, struct nfsd4_session, > > > > se_perclnt); > > > > list_del(&ses->se_perclnt); > > > > - WARN_ON_ONCE(atomic_read(&ses->se_ref)); > > > > + WARN_ON_ONCE((atomic_read(&ses->se_ref) > 1)); > > > > free_session(ses); > > > > } > > > > rpc_destroy_wait_queue(&clp->cl_cb_waitq); > > > > -- > > > > 2.7.4 > > > > > > > > The basic idea here is that nfsv4 sessions have a "resting state" of 0. > > We want to keep them around, but if they go "dead" then we we'll tear > > them down if they aren't actively in use at the time. So, we still free > > the thing when the refcount goes to zero, but we have an extra condition > > before we free it on the put -- that the session is also "dead" (meaning > > that the client asked us to destroy it). > > > > Your patch doesn't look like it'll break anything, but I personally find > > it harder to follow that way. The freeable reference state will be 1 > > instead of the normal 0. > > > > I'm not sure there's another way to accomplish what we need > (initializing struct nfsd4_session objects with refcount=1) without > also modifying the freeable reference state. After migrating to the > refcount_t API, if we leave init_session() as is, the first call to > nfsd4_get_session_locked() will fail: > > static __be32 nfsd4_get_session_locked(struct nfsd4_session *ses) > { > __be32 status; > > if (is_session_dead(ses)) > return nfserr_badsession; > status = get_client_locked(ses->se_client); > if (status) > return status; > refcount_inc(&ses->se_ref); /* This fails and WARNS when > ses->se_ref is 0. */ > return nfs_ok; > } > > > The refcount_t API patches aren't yet merged, so this discussion is a > bit limited in that respect, but refcount_inc() WARNS when called with > a refcount_t object whose value is 0, as this may represent a > use-after-free attempt. > > Given this, I'm unsure how it's possible to achieve initialization of > struct nfsd4_session objects with refcount=1 while still maintaining > these objects' "rest state" at refcount=0. > One idea might be to take an extra reference on the thing when creating it, and then drop that reference when the thing is marked DEAD. The extra reference would be superfluous, but it might make it look a little more natural.
On Sat, Feb 11, 2017 at 07:31:42AM -0500, Jeff Layton wrote: > The basic idea here is that nfsv4 sessions have a "resting state" of 0. > We want to keep them around, but if they go "dead" then we we'll tear > them down if they aren't actively in use at the time. So, we still free > the thing when the refcount goes to zero, but we have an extra condition > before we free it on the put -- that the session is also "dead" (meaning > that the client asked us to destroy it). > > Your patch doesn't look like it'll break anything, but I personally find > it harder to follow that way. The freeable reference state will be 1 > instead of the normal 0. Alas, I don't have any examples in mind, but doesn't this pattern happen all over? You have objects that live in some data structure. They're freed only when they're removed from the data structure. You want removal to fail whenever they're in use. So it's natural to use an atomic counter to track the number of external users and some other lock to serialize lookup and destruction. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 11, 2017 at 8:15 PM, Bruce Fields <bfields@fieldses.org> wrote: > On Sat, Feb 11, 2017 at 07:31:42AM -0500, Jeff Layton wrote: >> The basic idea here is that nfsv4 sessions have a "resting state" of 0. >> We want to keep them around, but if they go "dead" then we we'll tear >> them down if they aren't actively in use at the time. So, we still free >> the thing when the refcount goes to zero, but we have an extra condition >> before we free it on the put -- that the session is also "dead" (meaning >> that the client asked us to destroy it). >> >> Your patch doesn't look like it'll break anything, but I personally find >> it harder to follow that way. The freeable reference state will be 1 >> instead of the normal 0. > > Alas, I don't have any examples in mind, but doesn't this pattern happen > all over? > The majority of refcounted objects are allocated with refcount=1: the very fact that they're being allocated means that they already have a user. The issue with struct nfsd4_session is that, like struct nfs4_client, references to it are only taken when the server is actively working on it. Its default "resting state" is with refcount=0. I would like to make its default resting state with refcount=1. In other cases similar to this, we've gotten around it by doing a semantic +1 to the object's overall refcounting scheme. Jeff suggested taking an additional reference in init_session(), and dropping it in is_session_dead(), after determining, in fact, that the object is DEAD. > You have objects that live in some data structure. They're freed only > when they're removed from the data structure. You want removal to fail > whenever they're in use. > When they're in use, these objects' refcount should be > 0. > So it's natural to use an atomic counter to track the number of external > users and some other lock to serialize lookup and destruction. > When considering refcounted objects, the most "natural" interpretation of refcount=0 means that the object no longer has any users and can be freed. Increments on objects with refcount=0 shouldn't be allowed, as this may indicate a use-after-free condition. This discussion is difficult because the refcount_t API hasn't yet been introduced. The purpose of that API is to eliminate use-after-free bugs. > --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 11, 2017 at 09:01:15AM -0500, David Windsor wrote: > I'm not sure there's another way to accomplish what we need > (initializing struct nfsd4_session objects with refcount=1) without > also modifying the freeable reference state. After migrating to the > refcount_t API, if we leave init_session() as is, the first call to > nfsd4_get_session_locked() will fail: Which is a pretty clear indicator that this code should simply not migrate to the recount_t API. Why was it even considered if the conversion is obviously broken? -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 11, 2017 at 01:42:53AM -0500, David Windsor wrote: ><snip> > >> Signed-off-by: David Windsor <dwindsor@gmail.com> >> --- >> fs/nfsd/nfs4state.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >> index a0dee8a..b0f3010 100644 >> --- a/fs/nfsd/nfs4state.c >> +++ b/fs/nfsd/nfs4state.c >> @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct nfsd4_session *ses) >> >> lockdep_assert_held(&nn->client_lock); >> >> - if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses)) >> + if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_des(ses)) > >This should read: >if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses)) > >> free_session(ses); Hi, I'm not sure if I have this correctly; But both before and after the patch free_session gets called when se_ref count was 1, shouldn't this have changed with the +1 scheme? Also, since the !atomic_add_unless doesn't actually decrement when at 1, doesn't this leave the se_ref as 1 when it's destroyed? The function seems to always be locked, so perhaps this doesn't matter, but still seems a bit risky. Thanks, -hans >> put_client_renew_locked(clp); >> } >> @@ -1645,7 +1645,7 @@ static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, stru >> new->se_flags = cses->flags; >> new->se_cb_prog = cses->callback_prog; >> new->se_cb_sec = cses->cb_sec; >> - atomic_set(&new->se_ref, 0); >> + atomic_set(&new->se_ref, 1); >> idx = hash_sessionid(&new->se_sessionid); >> list_add(&new->se_hash, &nn->sessionid_hashtbl[idx]); >> spin_lock(&clp->cl_lock); >> @@ -1792,7 +1792,7 @@ free_client(struct nfs4_client *clp) >> ses = list_entry(clp->cl_sessions.next, struct nfsd4_session, >> se_perclnt); >> list_del(&ses->se_perclnt); >> - WARN_ON_ONCE(atomic_read(&ses->se_ref)); >> + WARN_ON_ONCE((atomic_read(&ses->se_ref) > 1)); >> free_session(ses); >> } >> rpc_destroy_wait_queue(&clp->cl_cb_waitq); >> -- >> 2.7.4 >> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 13, 2017 at 5:38 AM, Christoph Hellwig <hch@infradead.org> wrote: > On Sat, Feb 11, 2017 at 09:01:15AM -0500, David Windsor wrote: >> I'm not sure there's another way to accomplish what we need >> (initializing struct nfsd4_session objects with refcount=1) without >> also modifying the freeable reference state. After migrating to the >> refcount_t API, if we leave init_session() as is, the first call to >> nfsd4_get_session_locked() will fail: > > Which is a pretty clear indicator that this code should simply not > migrate to the recount_t API. Why was it even considered if the > conversion is obviously broken? I'm not sure this is a sound argument for not converting to refcount_t. In other locations in which refcounting schemes are "unnatural," i.e. freeing refcounted objects when their refcount is -1 (rather than 0), conversion to refcount_t is accomplished by performing a logical +1 to the overall refcounting scheme. We're auditing all refcounting corner cases, such as these, to see if similar solutions can be found. Thanks, David -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 13, 2017 at 5:54 AM, Hans Liljestrand <ishkamiel@gmail.com> wrote: > On Sat, Feb 11, 2017 at 01:42:53AM -0500, David Windsor wrote: >> >> <snip> >> >>> Signed-off-by: David Windsor <dwindsor@gmail.com> >>> --- >>> fs/nfsd/nfs4state.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >>> index a0dee8a..b0f3010 100644 >>> --- a/fs/nfsd/nfs4state.c >>> +++ b/fs/nfsd/nfs4state.c >>> @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct >>> nfsd4_session *ses) >>> >>> lockdep_assert_held(&nn->client_lock); >>> >>> - if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses)) >>> + if (!atomic_add_unless(&ses->se_ref, -1, 1) && >>> is_session_des(ses)) >> >> >> This should read: >> if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses)) >> >>> free_session(ses); > > > Hi, > I'm not sure if I have this correctly; But both before and after the patch > free_session gets called when se_ref count was 1, shouldn't this have > changed with the +1 scheme? > > Also, since the !atomic_add_unless doesn't actually decrement when at 1, > doesn't this leave the se_ref as 1 when it's destroyed? The function seems > to always be locked, so perhaps this doesn't matter, but still seems a bit > risky. > Yes; I forgot the additional call to atomic_dec_and_test() before free_session(). Thanks! I'll resubmit this after seeing how the rest of this discussion goes. We may end up abandoning this refcounting case. > Thanks, > -hans > > >>> put_client_renew_locked(clp); >>> } >>> @@ -1645,7 +1645,7 @@ static void init_session(struct svc_rqst *rqstp, >>> struct nfsd4_session *new, stru >>> new->se_flags = cses->flags; >>> new->se_cb_prog = cses->callback_prog; >>> new->se_cb_sec = cses->cb_sec; >>> - atomic_set(&new->se_ref, 0); >>> + atomic_set(&new->se_ref, 1); >>> idx = hash_sessionid(&new->se_sessionid); >>> list_add(&new->se_hash, &nn->sessionid_hashtbl[idx]); >>> spin_lock(&clp->cl_lock); >>> @@ -1792,7 +1792,7 @@ free_client(struct nfs4_client *clp) >>> ses = list_entry(clp->cl_sessions.next, struct >>> nfsd4_session, >>> se_perclnt); >>> list_del(&ses->se_perclnt); >>> - WARN_ON_ONCE(atomic_read(&ses->se_ref)); >>> + WARN_ON_ONCE((atomic_read(&ses->se_ref) > 1)); >>> free_session(ses); >>> } >>> rpc_destroy_wait_queue(&clp->cl_cb_waitq); >>> -- >>> 2.7.4 >>> > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 13, 2017 at 06:42:56AM -0500, David Windsor wrote: > I'm not sure this is a sound argument for not converting to > refcount_t. It's an argument again the way how this patch was sent. Please take care of all the trivial conversions first, and then do anything non-trivial on a case by case basis. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 13, 2017 at 7:12 AM, Christoph Hellwig <hch@infradead.org> wrote: > On Mon, Feb 13, 2017 at 06:42:56AM -0500, David Windsor wrote: >> I'm not sure this is a sound argument for not converting to >> refcount_t. > > It's an argument again the way how this patch was sent. Please take care > of all the trivial conversions first, and then do anything non-trivial > on a case by case basis. This is actually what we're currently doing. The vast majority of atomic_t to refcount_t conversions are trivial, as they most always follow the pattern: 1. Initialize the atomic_t refcounter to 1. 2. Perform shared object get/put operations. 3. Free the shared object when its refcount becomes 0. We've identified a handful of instances in which steps 1 and 3 are different from above (i.e. initializing refcounts to 0, freeing objects when refcount is non-zero, etc.). The above patch addresses one of these instances in NFS. Thanks, David -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 13, 2017 at 06:46:19AM -0500, David Windsor wrote: > On Mon, Feb 13, 2017 at 5:54 AM, Hans Liljestrand <ishkamiel@gmail.com> wrote: > > On Sat, Feb 11, 2017 at 01:42:53AM -0500, David Windsor wrote: > >> > >> <snip> > >> > >>> Signed-off-by: David Windsor <dwindsor@gmail.com> > >>> --- > >>> fs/nfsd/nfs4state.c | 6 +++--- > >>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > >>> index a0dee8a..b0f3010 100644 > >>> --- a/fs/nfsd/nfs4state.c > >>> +++ b/fs/nfsd/nfs4state.c > >>> @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct > >>> nfsd4_session *ses) > >>> > >>> lockdep_assert_held(&nn->client_lock); > >>> > >>> - if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses)) > >>> + if (!atomic_add_unless(&ses->se_ref, -1, 1) && > >>> is_session_des(ses)) > >> > >> > >> This should read: > >> if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_dead(ses)) > >> > >>> free_session(ses); > > > > > > Hi, > > I'm not sure if I have this correctly; But both before and after the patch > > free_session gets called when se_ref count was 1, shouldn't this have > > changed with the +1 scheme? > > > > Also, since the !atomic_add_unless doesn't actually decrement when at 1, > > doesn't this leave the se_ref as 1 when it's destroyed? The function seems > > to always be locked, so perhaps this doesn't matter, but still seems a bit > > risky. > > > > Yes; I forgot the additional call to atomic_dec_and_test() before > free_session(). Thanks! > > I'll resubmit this after seeing how the rest of this discussion goes. > We may end up abandoning this refcounting case. I could live with it. My knee jerk reaction is like Jeff's--it just seems more natural to me for reference count 0 to mean "not in use, OK to free" in cases like this--but maybe I just need to get used to the idea. It'd be interesting to see what the final result looks like after conversion to refcount_t. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index a0dee8a..b0f3010 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -196,7 +196,7 @@ static void nfsd4_put_session_locked(struct nfsd4_session *ses) lockdep_assert_held(&nn->client_lock); - if (atomic_dec_and_test(&ses->se_ref) && is_session_dead(ses)) + if (!atomic_add_unless(&ses->se_ref, -1, 1) && is_session_des(ses)) free_session(ses); put_client_renew_locked(clp); } @@ -1645,7 +1645,7 @@ static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, stru new->se_flags = cses->flags; new->se_cb_prog = cses->callback_prog; new->se_cb_sec = cses->cb_sec; - atomic_set(&new->se_ref, 0); + atomic_set(&new->se_ref, 1); idx = hash_sessionid(&new->se_sessionid); list_add(&new->se_hash, &nn->sessionid_hashtbl[idx]); spin_lock(&clp->cl_lock); @@ -1792,7 +1792,7 @@ free_client(struct nfs4_client *clp) ses = list_entry(clp->cl_sessions.next, struct nfsd4_session, se_perclnt); list_del(&ses->se_perclnt); - WARN_ON_ONCE(atomic_read(&ses->se_ref)); + WARN_ON_ONCE((atomic_read(&ses->se_ref) > 1)); free_session(ses); } rpc_destroy_wait_queue(&clp->cl_cb_waitq);
In furtherance of the KSPP effort to add overflow protection to kernel reference counters, a new type (refcount_t) and API have been created. Part of the refcount_t API is refcount_inc(), which will not increment a refcount_t variable if its value is 0 (as this would indicate a possible use-after-free condition). In auditing the kernel for refcounting corner cases, we've come across the case of struct nfsd4_session. From fs/nfsd/state.h: /* * Representation of a v4.1+ session. These are refcounted in a similar * fashion to the nfs4_client. References are only taken when the server * is actively working on the object (primarily during the processing of * compounds). */ struct nfsd4_session { atomic_t se_ref; ... }; From fs/nfsd/nfs4state.c: static void init_session(..., struct nfsd4_session *new, ...) { ... atomic_set(&new->se_ref, 0); ... } Since nfsd4_session objects are initialized with refcount = 0, subsequent increments will fail using the new refcount_t API. Being largely unfamiliar with this subsystem's garbage collection mechanism, I'm unsure how to best fix this. Attached is a patch that performs a logical +1 on struct nfsd4_session's reference counting scheme. If this is the correct route to take, I will resubmit this patch with updated comments for how struct nfsd4_session is refcounted (see the above comment from fs/nsfd/state.h). This is in preparation for the previously mentioned refcount_t API series. Thanks, David Windsor Signed-off-by: David Windsor <dwindsor@gmail.com> --- fs/nfsd/nfs4state.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)