Date: Wed, 15 Feb 2017 11:45:05 -0500 From: Bruce Fields <bfields@...ldses.org> To: David Windsor <dwindsor@...il.com> Cc: Hans Liljestrand <ishkamiel@...il.com>, linux-nfs@...r.kernel.org, netdev@...r.kernel.org, kernel-hardening@...ts.openwall.com, Jeff Layton <jlayton@...chiereds.net>, Kees Cook <keescook@...omium.org>, "Reshetova, Elena" <elena.reshetova@...el.com> Subject: Re: Re: [RFC][PATCH] nfsd: add +1 to reference counting scheme for struct nfsd4_session 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@...il.com> wrote: > > On Sat, Feb 11, 2017 at 01:42:53AM -0500, David Windsor wrote: > >> > >> <snip> > >> > >>> Signed-off-by: David Windsor <dwindsor@...il.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.
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.