* [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-09 7:38 ` David Windsor 0 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-09 7:38 UTC (permalink / raw) To: linux-nfs, netdev Cc: kernel-hardening, bfields, jlayton, keescook, elena.reshetova, dwindsor 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(-) 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); -- 2.7.4 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [kernel-hardening] [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-09 7:38 ` David Windsor 0 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-09 7:38 UTC (permalink / raw) To: linux-nfs, netdev Cc: kernel-hardening, bfields, jlayton, keescook, elena.reshetova, dwindsor 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(-) 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); -- 2.7.4 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-09 7:38 ` David Windsor 0 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-09 7:38 UTC (permalink / raw) To: linux-nfs, netdev Cc: kernel-hardening, bfields, jlayton, keescook, elena.reshetova, dwindsor 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. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 2017-02-09 7:38 ` David Windsor @ 2017-02-11 6:42 ` David Windsor -1 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-11 6:42 UTC (permalink / raw) To: linux-nfs, netdev Cc: kernel-hardening, Bruce Fields, Jeff Layton, Kees Cook, Reshetova, Elena, David Windsor <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 > ^ permalink raw reply [flat|nested] 34+ messages in thread
* [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-11 6:42 ` David Windsor 0 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-11 6:42 UTC (permalink / raw) To: linux-nfs, netdev Cc: kernel-hardening, Bruce Fields, Jeff Layton, Kees Cook, Reshetova, Elena, David Windsor <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 > ^ permalink raw reply [flat|nested] 34+ messages in thread
[parent not found: <CAEXv5_jUa8Av4JABoKSAueiLHSLzibMvaE-8DrVcxZHFceckMg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 2017-02-11 6:42 ` [kernel-hardening] " David Windsor (?) @ 2017-02-11 12:31 ` Jeff Layton -1 siblings, 0 replies; 34+ messages in thread From: Jeff Layton @ 2017-02-11 12:31 UTC (permalink / raw) To: David Windsor, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA Cc: kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8, Bruce Fields, Kees Cook, Reshetova, Elena On Sat, 2017-02-11 at 01:42 -0500, David Windsor wrote: > <snip> > > > Signed-off-by: David Windsor <dwindsor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > --- > > 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-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 34+ messages in thread
* [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-11 12:31 ` Jeff Layton 0 siblings, 0 replies; 34+ messages in thread From: Jeff Layton @ 2017-02-11 12:31 UTC (permalink / raw) To: David Windsor, linux-nfs, netdev Cc: kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-11 12:31 ` Jeff Layton 0 siblings, 0 replies; 34+ messages in thread From: Jeff Layton @ 2017-02-11 12:31 UTC (permalink / raw) To: David Windsor, linux-nfs, netdev Cc: kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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> ^ permalink raw reply [flat|nested] 34+ messages in thread
[parent not found: <1486816302.4233.29.camel-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>]
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 2017-02-11 12:31 ` Jeff Layton (?) @ 2017-02-11 14:01 ` David Windsor -1 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-11 14:01 UTC (permalink / raw) To: Jeff Layton Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8, Bruce Fields, Kees Cook, Reshetova, Elena On Sat, Feb 11, 2017 at 7:31 AM, Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org> wrote: > On Sat, 2017-02-11 at 01:42 -0500, David Windsor wrote: >> <snip> >> >> > Signed-off-by: David Windsor <dwindsor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >> > --- >> > 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-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 34+ messages in thread
* [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-11 14:01 ` David Windsor 0 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-11 14:01 UTC (permalink / raw) To: Jeff Layton Cc: linux-nfs, netdev, kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-11 14:01 ` David Windsor 0 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-11 14:01 UTC (permalink / raw) To: Jeff Layton Cc: linux-nfs, netdev, kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 2017-02-11 14:01 ` David Windsor @ 2017-02-11 14:09 ` Jeff Layton -1 siblings, 0 replies; 34+ messages in thread From: Jeff Layton @ 2017-02-11 14:09 UTC (permalink / raw) To: David Windsor Cc: linux-nfs, netdev, kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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. -- Jeff Layton <jlayton@poochiereds.net> ^ permalink raw reply [flat|nested] 34+ messages in thread
* [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-11 14:09 ` Jeff Layton 0 siblings, 0 replies; 34+ messages in thread From: Jeff Layton @ 2017-02-11 14:09 UTC (permalink / raw) To: David Windsor Cc: linux-nfs, netdev, kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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. -- Jeff Layton <jlayton@poochiereds.net> ^ permalink raw reply [flat|nested] 34+ messages in thread
[parent not found: <CAEXv5_gd7F-eaazzU1BWPzH4huhEcO1Y-FWov5UP9T+6R+fv-A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 2017-02-11 14:01 ` David Windsor (?) @ 2017-02-13 10:38 ` Christoph Hellwig -1 siblings, 0 replies; 34+ messages in thread From: Christoph Hellwig @ 2017-02-13 10:38 UTC (permalink / raw) To: David Windsor Cc: Jeff Layton, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8, Bruce Fields, Kees Cook, Reshetova, Elena 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 34+ messages in thread
* [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-13 10:38 ` Christoph Hellwig 0 siblings, 0 replies; 34+ messages in thread From: Christoph Hellwig @ 2017-02-13 10:38 UTC (permalink / raw) To: David Windsor Cc: Jeff Layton, linux-nfs, netdev, kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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? ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-13 10:38 ` Christoph Hellwig 0 siblings, 0 replies; 34+ messages in thread From: Christoph Hellwig @ 2017-02-13 10:38 UTC (permalink / raw) To: David Windsor Cc: Jeff Layton, linux-nfs, netdev, kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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? ^ permalink raw reply [flat|nested] 34+ messages in thread
[parent not found: <20170213103815.GA5131-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>]
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 2017-02-13 10:38 ` Christoph Hellwig (?) @ 2017-02-13 11:42 ` David Windsor -1 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-13 11:42 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8, Bruce Fields, Kees Cook, Reshetova, Elena On Mon, Feb 13, 2017 at 5:38 AM, Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 34+ messages in thread
* [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-13 11:42 ` David Windsor 0 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-13 11:42 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, linux-nfs, netdev, kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-13 11:42 ` David Windsor 0 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-13 11:42 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, linux-nfs, netdev, kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
[parent not found: <CAEXv5_g=DS4wk0mgZuw-doVCqountb-CxZki1LOoQH-P7W1U4A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 2017-02-13 11:42 ` David Windsor (?) @ 2017-02-13 12:12 ` Christoph Hellwig -1 siblings, 0 replies; 34+ messages in thread From: Christoph Hellwig @ 2017-02-13 12:12 UTC (permalink / raw) To: David Windsor Cc: Christoph Hellwig, Jeff Layton, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8, Bruce Fields, Kees Cook, Reshetova, Elena 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 34+ messages in thread
* [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-13 12:12 ` Christoph Hellwig 0 siblings, 0 replies; 34+ messages in thread From: Christoph Hellwig @ 2017-02-13 12:12 UTC (permalink / raw) To: David Windsor Cc: Christoph Hellwig, Jeff Layton, linux-nfs, netdev, kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-13 12:12 ` Christoph Hellwig 0 siblings, 0 replies; 34+ messages in thread From: Christoph Hellwig @ 2017-02-13 12:12 UTC (permalink / raw) To: David Windsor Cc: Christoph Hellwig, Jeff Layton, linux-nfs, netdev, kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 2017-02-13 12:12 ` Christoph Hellwig @ 2017-02-14 13:48 ` David Windsor -1 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-14 13:48 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, linux-nfs, netdev, kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-14 13:48 ` David Windsor 0 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-14 13:48 UTC (permalink / raw) To: Christoph Hellwig Cc: Jeff Layton, linux-nfs, netdev, kernel-hardening, Bruce Fields, Kees Cook, Reshetova, Elena 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 2017-02-11 12:31 ` Jeff Layton (?) @ 2017-02-12 1:15 ` Bruce Fields -1 siblings, 0 replies; 34+ messages in thread From: Bruce Fields @ 2017-02-12 1:15 UTC (permalink / raw) To: Jeff Layton Cc: David Windsor, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8, Kees Cook, Reshetova, Elena 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 34+ messages in thread
* [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-12 1:15 ` Bruce Fields 0 siblings, 0 replies; 34+ messages in thread From: Bruce Fields @ 2017-02-12 1:15 UTC (permalink / raw) To: Jeff Layton Cc: David Windsor, linux-nfs, netdev, kernel-hardening, Kees Cook, Reshetova, Elena 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. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-12 1:15 ` Bruce Fields 0 siblings, 0 replies; 34+ messages in thread From: Bruce Fields @ 2017-02-12 1:15 UTC (permalink / raw) To: Jeff Layton Cc: David Windsor, linux-nfs, netdev, kernel-hardening, Kees Cook, Reshetova, Elena 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. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 2017-02-12 1:15 ` Bruce Fields @ 2017-02-12 1:42 ` David Windsor -1 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-12 1:42 UTC (permalink / raw) To: Bruce Fields Cc: Jeff Layton, linux-nfs, netdev, kernel-hardening, Kees Cook, Reshetova, Elena 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. ^ permalink raw reply [flat|nested] 34+ messages in thread
* [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-12 1:42 ` David Windsor 0 siblings, 0 replies; 34+ messages in thread From: David Windsor @ 2017-02-12 1:42 UTC (permalink / raw) To: Bruce Fields Cc: Jeff Layton, linux-nfs, netdev, kernel-hardening, Kees Cook, Reshetova, Elena 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. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 2017-02-11 6:42 ` [kernel-hardening] " David Windsor @ 2017-02-13 10:54 ` Hans Liljestrand -1 siblings, 0 replies; 34+ messages in thread From: Hans Liljestrand @ 2017-02-13 10:54 UTC (permalink / raw) To: David Windsor Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8, Bruce Fields, Jeff Layton, Kees Cook, Reshetova, Elena On Sat, Feb 11, 2017 at 01:42:53AM -0500, David Windsor wrote: ><snip> > >> Signed-off-by: David Windsor <dwindsor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >> --- >> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-13 10:54 ` Hans Liljestrand 0 siblings, 0 replies; 34+ messages in thread From: Hans Liljestrand @ 2017-02-13 10:54 UTC (permalink / raw) To: David Windsor Cc: linux-nfs, netdev, kernel-hardening, Bruce Fields, Jeff Layton, Kees Cook, Reshetova, Elena 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 >> ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 2017-02-13 10:54 ` Hans Liljestrand (?) @ 2017-02-13 11:46 ` David Windsor [not found] ` <CAEXv5_hP39k7HSLP-G_khx7MMQHnk=8Z5caa+U5n3bYvUTE1gQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> -1 siblings, 1 reply; 34+ messages in thread From: David Windsor @ 2017-02-13 11:46 UTC (permalink / raw) To: Hans Liljestrand Cc: linux-nfs, netdev, kernel-hardening, Bruce Fields, Jeff Layton, Kees Cook, Reshetova, Elena 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 >>> > ^ permalink raw reply [flat|nested] 34+ messages in thread
[parent not found: <CAEXv5_hP39k7HSLP-G_khx7MMQHnk=8Z5caa+U5n3bYvUTE1gQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 2017-02-13 11:46 ` David Windsor @ 2017-02-15 16:45 ` Bruce Fields 0 siblings, 0 replies; 34+ messages in thread From: Bruce Fields @ 2017-02-15 16:45 UTC (permalink / raw) To: David Windsor Cc: Hans Liljestrand, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8, Jeff Layton, Kees Cook, Reshetova, Elena 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote: > > On Sat, Feb 11, 2017 at 01:42:53AM -0500, David Windsor wrote: > >> > >> <snip> > >> > >>> Signed-off-by: David Windsor <dwindsor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > >>> --- > >>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kernel-hardening] Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session @ 2017-02-15 16:45 ` Bruce Fields 0 siblings, 0 replies; 34+ messages in thread From: Bruce Fields @ 2017-02-15 16:45 UTC (permalink / raw) To: David Windsor Cc: Hans Liljestrand, linux-nfs, netdev, kernel-hardening, Jeff Layton, Kees Cook, Reshetova, Elena 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. ^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2017-02-15 16:45 UTC | newest] Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-09 7:38 [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session David Windsor 2017-02-09 7:38 ` [kernel-hardening] " David Windsor 2017-02-09 7:38 ` David Windsor 2017-02-11 6:42 ` David Windsor 2017-02-11 6:42 ` [kernel-hardening] " David Windsor [not found] ` <CAEXv5_jUa8Av4JABoKSAueiLHSLzibMvaE-8DrVcxZHFceckMg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-02-11 12:31 ` Jeff Layton 2017-02-11 12:31 ` [kernel-hardening] " Jeff Layton 2017-02-11 12:31 ` Jeff Layton [not found] ` <1486816302.4233.29.camel-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org> 2017-02-11 14:01 ` David Windsor 2017-02-11 14:01 ` [kernel-hardening] " David Windsor 2017-02-11 14:01 ` David Windsor 2017-02-11 14:09 ` Jeff Layton 2017-02-11 14:09 ` [kernel-hardening] " Jeff Layton [not found] ` <CAEXv5_gd7F-eaazzU1BWPzH4huhEcO1Y-FWov5UP9T+6R+fv-A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-02-13 10:38 ` Christoph Hellwig 2017-02-13 10:38 ` [kernel-hardening] " Christoph Hellwig 2017-02-13 10:38 ` Christoph Hellwig [not found] ` <20170213103815.GA5131-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> 2017-02-13 11:42 ` David Windsor 2017-02-13 11:42 ` [kernel-hardening] " David Windsor 2017-02-13 11:42 ` David Windsor [not found] ` <CAEXv5_g=DS4wk0mgZuw-doVCqountb-CxZki1LOoQH-P7W1U4A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-02-13 12:12 ` Christoph Hellwig 2017-02-13 12:12 ` [kernel-hardening] " Christoph Hellwig 2017-02-13 12:12 ` Christoph Hellwig 2017-02-14 13:48 ` David Windsor 2017-02-14 13:48 ` [kernel-hardening] " David Windsor 2017-02-12 1:15 ` Bruce Fields 2017-02-12 1:15 ` [kernel-hardening] " Bruce Fields 2017-02-12 1:15 ` Bruce Fields 2017-02-12 1:42 ` David Windsor 2017-02-12 1:42 ` [kernel-hardening] " David Windsor 2017-02-13 10:54 ` Hans Liljestrand 2017-02-13 10:54 ` Hans Liljestrand 2017-02-13 11:46 ` David Windsor [not found] ` <CAEXv5_hP39k7HSLP-G_khx7MMQHnk=8Z5caa+U5n3bYvUTE1gQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-02-15 16:45 ` Bruce Fields 2017-02-15 16:45 ` Bruce Fields
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.