From: "Paul E. McKenney" <paulmck@kernel.org> To: madhuparnabhowmik04@gmail.com Cc: trond.myklebust@hammerspace.com, anna.schumaker@netapp.com, joel@joelfernandes.org, linux-nfs@vger.kernel.org, rcu@vger.kernel.org, linux-kernel-mentees@lists.linuxfoundation.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error Date: Fri, 6 Dec 2019 08:02:38 -0800 [thread overview] Message-ID: <20191206160238.GE2889@paulmck-ThinkPad-P72> (raw) In-Reply-To: <20191206151640.10966-1-madhuparnabhowmik04@gmail.com> On Fri, Dec 06, 2019 at 08:46:40PM +0530, madhuparnabhowmik04@gmail.com wrote: > From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> > > This patch fixes the following errors: > fs/nfs/dir.c:2353:14: error: incompatible types in comparison expression (different address spaces): > fs/nfs/dir.c:2353:14: struct list_head [noderef] <asn:4> * > fs/nfs/dir.c:2353:14: struct list_head * > > caused due to directly accessing the prev pointer of > a RCU protected list. > Accessing the pointer using the macro list_prev_rcu() fixes this error. > > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> > --- > fs/nfs/dir.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > index e180033e35cf..2035254cc283 100644 > --- a/fs/nfs/dir.c > +++ b/fs/nfs/dir.c > @@ -2350,7 +2350,7 @@ static int nfs_access_get_cached_rcu(struct inode *inode, const struct cred *cre > rcu_read_lock(); > if (nfsi->cache_validity & NFS_INO_INVALID_ACCESS) > goto out; > - lh = rcu_dereference(nfsi->access_cache_entry_lru.prev); > + lh = rcu_dereference(list_prev_rcu(&nfsi->access_cache_entry_lru)); And as noted in the earlier email, what is preventing concurrent insertions into and deletions from this list? o This use of list_move_tail() is OK because it does not poison. Though it isn't being all that friendly to lockless access to ->prev -- no WRITE_ONCE() in list_move_tail(). o The use of list_add_tail() is not safe with RCU readers, though they do at least partially compensate via use of smp_wmb() in nfs_access_add_cache() before calling nfs_access_add_rbtree(). o The list_del() near the end of nfs_access_add_rbtree() will poison the ->prev pointer. I don't see how this is safe given the possibility of a concurrent call to nfs_access_get_cached_rcu(). > cache = list_entry(lh, struct nfs_access_entry, lru); > if (lh == &nfsi->access_cache_entry_lru || > cred != cache->cred) And a few lines below here, it really does dereference the pointer obtained from ->prev! So how to really fix this? Here is one possibility, but we of course need to get the NFS developers' and maintainers' thoughts: o Create a list that is safe for bidirectional RCU traversal. This can use list_head, and would need these functions, give or take the exact names: list_add_tail_rcuprev(): This is like list_add_tail_rcu(), but also has smp_store_release() for ->prev. (As in there is also a __list_add_rcuprev() helper that actually contains the additional smp_store_release().) list_del_rcuprev(): This can be exactly __list_del_entry(), but with the assignment to ->prev in __list_del() becoming WRITE_ONCE(). And it looks like callers to __list_del_entry() and __list_del() might need some attention! And these might result in additional users of *_rcuprev(). list_prev_rcu() as in your first patch, but with READ_ONCE(). Otherwise DEC Alpha can fail. And more subtle compiler issues can appear on other architectures. Note that list_move_tail() will be OK give or take *_ONCE(). It might be better to define a list_move_tail_rcuprev(), given the large number of users of list_move_tail() -- some of these users might not like even the possibility of added overhead due to volatile accesses. ;-) Or am I missing something subtle here? Thanx, Paul
WARNING: multiple messages have this Message-ID (diff)
From: "Paul E. McKenney" <paulmck@kernel.org> To: madhuparnabhowmik04@gmail.com Cc: linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org, anna.schumaker@netapp.com, rcu@vger.kernel.org, joel@joelfernandes.org, linux-kernel-mentees@lists.linuxfoundation.org, trond.myklebust@hammerspace.com Subject: Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error Date: Fri, 6 Dec 2019 08:02:38 -0800 [thread overview] Message-ID: <20191206160238.GE2889@paulmck-ThinkPad-P72> (raw) In-Reply-To: <20191206151640.10966-1-madhuparnabhowmik04@gmail.com> On Fri, Dec 06, 2019 at 08:46:40PM +0530, madhuparnabhowmik04@gmail.com wrote: > From: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> > > This patch fixes the following errors: > fs/nfs/dir.c:2353:14: error: incompatible types in comparison expression (different address spaces): > fs/nfs/dir.c:2353:14: struct list_head [noderef] <asn:4> * > fs/nfs/dir.c:2353:14: struct list_head * > > caused due to directly accessing the prev pointer of > a RCU protected list. > Accessing the pointer using the macro list_prev_rcu() fixes this error. > > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik04@gmail.com> > --- > fs/nfs/dir.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > index e180033e35cf..2035254cc283 100644 > --- a/fs/nfs/dir.c > +++ b/fs/nfs/dir.c > @@ -2350,7 +2350,7 @@ static int nfs_access_get_cached_rcu(struct inode *inode, const struct cred *cre > rcu_read_lock(); > if (nfsi->cache_validity & NFS_INO_INVALID_ACCESS) > goto out; > - lh = rcu_dereference(nfsi->access_cache_entry_lru.prev); > + lh = rcu_dereference(list_prev_rcu(&nfsi->access_cache_entry_lru)); And as noted in the earlier email, what is preventing concurrent insertions into and deletions from this list? o This use of list_move_tail() is OK because it does not poison. Though it isn't being all that friendly to lockless access to ->prev -- no WRITE_ONCE() in list_move_tail(). o The use of list_add_tail() is not safe with RCU readers, though they do at least partially compensate via use of smp_wmb() in nfs_access_add_cache() before calling nfs_access_add_rbtree(). o The list_del() near the end of nfs_access_add_rbtree() will poison the ->prev pointer. I don't see how this is safe given the possibility of a concurrent call to nfs_access_get_cached_rcu(). > cache = list_entry(lh, struct nfs_access_entry, lru); > if (lh == &nfsi->access_cache_entry_lru || > cred != cache->cred) And a few lines below here, it really does dereference the pointer obtained from ->prev! So how to really fix this? Here is one possibility, but we of course need to get the NFS developers' and maintainers' thoughts: o Create a list that is safe for bidirectional RCU traversal. This can use list_head, and would need these functions, give or take the exact names: list_add_tail_rcuprev(): This is like list_add_tail_rcu(), but also has smp_store_release() for ->prev. (As in there is also a __list_add_rcuprev() helper that actually contains the additional smp_store_release().) list_del_rcuprev(): This can be exactly __list_del_entry(), but with the assignment to ->prev in __list_del() becoming WRITE_ONCE(). And it looks like callers to __list_del_entry() and __list_del() might need some attention! And these might result in additional users of *_rcuprev(). list_prev_rcu() as in your first patch, but with READ_ONCE(). Otherwise DEC Alpha can fail. And more subtle compiler issues can appear on other architectures. Note that list_move_tail() will be OK give or take *_ONCE(). It might be better to define a list_move_tail_rcuprev(), given the large number of users of list_move_tail() -- some of these users might not like even the possibility of added overhead due to volatile accesses. ;-) Or am I missing something subtle here? Thanx, Paul _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent reply other threads:[~2019-12-06 16:02 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-06 15:16 [PATCH 2/2] fs: nfs: dir.c: Fix sparse error madhuparnabhowmik04 2019-12-06 15:16 ` [Linux-kernel-mentees] " madhuparnabhowmik04 2019-12-06 16:00 ` Joel Fernandes 2019-12-06 16:00 ` [Linux-kernel-mentees] " Joel Fernandes 2019-12-06 16:12 ` Paul E. McKenney 2019-12-06 16:12 ` [Linux-kernel-mentees] " Paul E. McKenney 2019-12-06 16:02 ` Paul E. McKenney [this message] 2019-12-06 16:02 ` Paul E. McKenney 2019-12-06 17:52 ` Trond Myklebust 2019-12-06 17:52 ` [Linux-kernel-mentees] " Trond Myklebust 2019-12-06 18:24 ` Paul E. McKenney 2019-12-06 18:24 ` [Linux-kernel-mentees] " Paul E. McKenney 2019-12-06 18:28 ` Trond Myklebust 2019-12-06 18:28 ` [Linux-kernel-mentees] " Trond Myklebust 2019-12-06 18:45 ` Paul E. McKenney 2019-12-06 18:45 ` [Linux-kernel-mentees] " Paul E. McKenney 2019-12-12 21:55 ` Joel Fernandes 2019-12-12 21:55 ` [Linux-kernel-mentees] " Joel Fernandes 2019-12-13 1:16 ` Paul E. McKenney 2019-12-13 1:16 ` [Linux-kernel-mentees] " Paul E. McKenney
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191206160238.GE2889@paulmck-ThinkPad-P72 \ --to=paulmck@kernel.org \ --cc=anna.schumaker@netapp.com \ --cc=joel@joelfernandes.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nfs@vger.kernel.org \ --cc=madhuparnabhowmik04@gmail.com \ --cc=rcu@vger.kernel.org \ --cc=trond.myklebust@hammerspace.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.