* [PATCH 2/2] fs: nfs: dir.c: Fix sparse error @ 2019-12-06 15:16 ` madhuparnabhowmik04 0 siblings, 0 replies; 20+ messages in thread From: madhuparnabhowmik04 @ 2019-12-06 15:16 UTC (permalink / raw) To: trond.myklebust, anna.schumaker, joel Cc: linux-nfs, rcu, linux-kernel-mentees, linux-kernel, Madhuparna Bhowmik 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)); cache = list_entry(lh, struct nfs_access_entry, lru); if (lh == &nfsi->access_cache_entry_lru || cred != cache->cred) -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error @ 2019-12-06 15:16 ` madhuparnabhowmik04 0 siblings, 0 replies; 20+ messages in thread From: madhuparnabhowmik04 @ 2019-12-06 15:16 UTC (permalink / raw) To: trond.myklebust, anna.schumaker, joel Cc: rcu, linux-nfs, linux-kernel-mentees, linux-kernel 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)); cache = list_entry(lh, struct nfs_access_entry, lru); if (lh == &nfsi->access_cache_entry_lru || cred != cache->cred) -- 2.17.1 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error 2019-12-06 15:16 ` [Linux-kernel-mentees] " madhuparnabhowmik04 @ 2019-12-06 16:00 ` Joel Fernandes -1 siblings, 0 replies; 20+ messages in thread From: Joel Fernandes @ 2019-12-06 16:00 UTC (permalink / raw) To: madhuparnabhowmik04 Cc: trond.myklebust, anna.schumaker, linux-nfs, rcu, linux-kernel-mentees, linux-kernel, paulmck +Paul, here is the dependent patch for the list_prev_rcu() patch Madhuparna posted. 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)); > cache = list_entry(lh, struct nfs_access_entry, lru); > if (lh == &nfsi->access_cache_entry_lru || > cred != cache->cred) > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error @ 2019-12-06 16:00 ` Joel Fernandes 0 siblings, 0 replies; 20+ messages in thread From: Joel Fernandes @ 2019-12-06 16:00 UTC (permalink / raw) To: madhuparnabhowmik04 Cc: linux-nfs, paulmck, linux-kernel, anna.schumaker, rcu, linux-kernel-mentees, trond.myklebust +Paul, here is the dependent patch for the list_prev_rcu() patch Madhuparna posted. 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)); > cache = list_entry(lh, struct nfs_access_entry, lru); > if (lh == &nfsi->access_cache_entry_lru || > cred != cache->cred) > -- > 2.17.1 > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error 2019-12-06 16:00 ` [Linux-kernel-mentees] " Joel Fernandes @ 2019-12-06 16:12 ` Paul E. McKenney -1 siblings, 0 replies; 20+ messages in thread From: Paul E. McKenney @ 2019-12-06 16:12 UTC (permalink / raw) To: Joel Fernandes Cc: madhuparnabhowmik04, trond.myklebust, anna.schumaker, linux-nfs, rcu, linux-kernel-mentees, linux-kernel On Fri, Dec 06, 2019 at 11:00:02AM -0500, Joel Fernandes wrote: > +Paul, here is the dependent patch for the list_prev_rcu() patch Madhuparna > posted. Got it, thank you! And however this turns out, it does illustrate the value of the sparse address-space checks! Thanx, Paul > 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)); > > cache = list_entry(lh, struct nfs_access_entry, lru); > > if (lh == &nfsi->access_cache_entry_lru || > > cred != cache->cred) > > -- > > 2.17.1 > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error @ 2019-12-06 16:12 ` Paul E. McKenney 0 siblings, 0 replies; 20+ messages in thread From: Paul E. McKenney @ 2019-12-06 16:12 UTC (permalink / raw) To: Joel Fernandes Cc: linux-nfs, linux-kernel, anna.schumaker, rcu, linux-kernel-mentees, trond.myklebust On Fri, Dec 06, 2019 at 11:00:02AM -0500, Joel Fernandes wrote: > +Paul, here is the dependent patch for the list_prev_rcu() patch Madhuparna > posted. Got it, thank you! And however this turns out, it does illustrate the value of the sparse address-space checks! Thanx, Paul > 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)); > > cache = list_entry(lh, struct nfs_access_entry, lru); > > if (lh == &nfsi->access_cache_entry_lru || > > cred != cache->cred) > > -- > > 2.17.1 > > _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error 2019-12-06 15:16 ` [Linux-kernel-mentees] " madhuparnabhowmik04 @ 2019-12-06 16:02 ` Paul E. McKenney -1 siblings, 0 replies; 20+ messages in thread From: Paul E. McKenney @ 2019-12-06 16:02 UTC (permalink / raw) To: madhuparnabhowmik04 Cc: trond.myklebust, anna.schumaker, joel, linux-nfs, rcu, linux-kernel-mentees, linux-kernel 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error @ 2019-12-06 16:02 ` Paul E. McKenney 0 siblings, 0 replies; 20+ messages in thread From: Paul E. McKenney @ 2019-12-06 16:02 UTC (permalink / raw) To: madhuparnabhowmik04 Cc: linux-nfs, linux-kernel, anna.schumaker, rcu, joel, linux-kernel-mentees, trond.myklebust 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error 2019-12-06 16:02 ` [Linux-kernel-mentees] " Paul E. McKenney @ 2019-12-06 17:52 ` Trond Myklebust -1 siblings, 0 replies; 20+ messages in thread From: Trond Myklebust @ 2019-12-06 17:52 UTC (permalink / raw) To: paulmck, madhuparnabhowmik04 Cc: linux-kernel-mentees, linux-nfs, rcu, anna.schumaker, joel, linux-kernel Hi Paul, On Fri, 2019-12-06 at 08:02 -0800, Paul E. McKenney wrote: > 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(). The pointer nfsi->access_cache_entry_lru is the head of the list, so it won't get poisoned. Furthermore, the objects it points to are freed using kfree_rcu(), so they will survive as long as we hold the rcu read lock. The object's cred pointers also points to something that is freed in an rcu-safe manner. The problem here is rather that a racing list_del() can cause nfsi- >access_cache_entry_lru to be empty, which is presumably why Neil added that check plus the empty cred pointer check in the following line. The barrier semantics may be suspect, although the spin unlock after list_del() should presumably guarantee release semantics? -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error @ 2019-12-06 17:52 ` Trond Myklebust 0 siblings, 0 replies; 20+ messages in thread From: Trond Myklebust @ 2019-12-06 17:52 UTC (permalink / raw) To: paulmck, madhuparnabhowmik04 Cc: linux-nfs, linux-kernel, rcu, joel, linux-kernel-mentees, anna.schumaker Hi Paul, On Fri, 2019-12-06 at 08:02 -0800, Paul E. McKenney wrote: > 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(). The pointer nfsi->access_cache_entry_lru is the head of the list, so it won't get poisoned. Furthermore, the objects it points to are freed using kfree_rcu(), so they will survive as long as we hold the rcu read lock. The object's cred pointers also points to something that is freed in an rcu-safe manner. The problem here is rather that a racing list_del() can cause nfsi- >access_cache_entry_lru to be empty, which is presumably why Neil added that check plus the empty cred pointer check in the following line. The barrier semantics may be suspect, although the spin unlock after list_del() should presumably guarantee release semantics? -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error 2019-12-06 17:52 ` [Linux-kernel-mentees] " Trond Myklebust @ 2019-12-06 18:24 ` Paul E. McKenney -1 siblings, 0 replies; 20+ messages in thread From: Paul E. McKenney @ 2019-12-06 18:24 UTC (permalink / raw) To: Trond Myklebust Cc: madhuparnabhowmik04, linux-kernel-mentees, linux-nfs, rcu, anna.schumaker, joel, linux-kernel On Fri, Dec 06, 2019 at 05:52:10PM +0000, Trond Myklebust wrote: > Hi Paul, > > On Fri, 2019-12-06 at 08:02 -0800, Paul E. McKenney wrote: > > 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(). > > The pointer nfsi->access_cache_entry_lru is the head of the list, so it > won't get poisoned. Furthermore, the objects it points to are freed > using kfree_rcu(), so they will survive as long as we hold the rcu read > lock. The object's cred pointers also points to something that is freed > in an rcu-safe manner. > > The problem here is rather that a racing list_del() can cause nfsi- > >access_cache_entry_lru to be empty, which is presumably why Neil added > that check plus the empty cred pointer check in the following line. > > The barrier semantics may be suspect, although the spin unlock after > list_del() should presumably guarantee release semantics? Ah, OK, so you are only ever using ->prev only from the head of the list, and presumably never do list_del() on the head itself. (Don't laugh, this does really happen as a way to remove the entire list, though perhaps with list_del_init() rather than list_del().) Maybe we should have a list_tail_rcu() that is only expected to work on the head of the list? Thanx, Paul ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error @ 2019-12-06 18:24 ` Paul E. McKenney 0 siblings, 0 replies; 20+ messages in thread From: Paul E. McKenney @ 2019-12-06 18:24 UTC (permalink / raw) To: Trond Myklebust Cc: linux-nfs, linux-kernel, rcu, joel, linux-kernel-mentees, anna.schumaker On Fri, Dec 06, 2019 at 05:52:10PM +0000, Trond Myklebust wrote: > Hi Paul, > > On Fri, 2019-12-06 at 08:02 -0800, Paul E. McKenney wrote: > > 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(). > > The pointer nfsi->access_cache_entry_lru is the head of the list, so it > won't get poisoned. Furthermore, the objects it points to are freed > using kfree_rcu(), so they will survive as long as we hold the rcu read > lock. The object's cred pointers also points to something that is freed > in an rcu-safe manner. > > The problem here is rather that a racing list_del() can cause nfsi- > >access_cache_entry_lru to be empty, which is presumably why Neil added > that check plus the empty cred pointer check in the following line. > > The barrier semantics may be suspect, although the spin unlock after > list_del() should presumably guarantee release semantics? Ah, OK, so you are only ever using ->prev only from the head of the list, and presumably never do list_del() on the head itself. (Don't laugh, this does really happen as a way to remove the entire list, though perhaps with list_del_init() rather than list_del().) Maybe we should have a list_tail_rcu() that is only expected to work on the head of the list? Thanx, Paul _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error 2019-12-06 18:24 ` [Linux-kernel-mentees] " Paul E. McKenney @ 2019-12-06 18:28 ` Trond Myklebust -1 siblings, 0 replies; 20+ messages in thread From: Trond Myklebust @ 2019-12-06 18:28 UTC (permalink / raw) To: paulmck Cc: linux-kernel-mentees, madhuparnabhowmik04, linux-nfs, rcu, anna.schumaker, linux-kernel, joel On Fri, 2019-12-06 at 10:24 -0800, Paul E. McKenney wrote: > On Fri, Dec 06, 2019 at 05:52:10PM +0000, Trond Myklebust wrote: > > Hi Paul, > > > > On Fri, 2019-12-06 at 08:02 -0800, Paul E. McKenney wrote: > > > 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(). > > > > The pointer nfsi->access_cache_entry_lru is the head of the list, > > so it > > won't get poisoned. Furthermore, the objects it points to are freed > > using kfree_rcu(), so they will survive as long as we hold the rcu > > read > > lock. The object's cred pointers also points to something that is > > freed > > in an rcu-safe manner. > > > > The problem here is rather that a racing list_del() can cause nfsi- > > > access_cache_entry_lru to be empty, which is presumably why Neil > > > added > > that check plus the empty cred pointer check in the following line. > > > > The barrier semantics may be suspect, although the spin unlock > > after > > list_del() should presumably guarantee release semantics? > > Ah, OK, so you are only ever using ->prev only from the head of the > list, > and presumably never do list_del() on the head itself. (Don't laugh, > this does really happen as a way to remove the entire list, though > perhaps with list_del_init() rather than list_del().) Correct. > Maybe we should have a list_tail_rcu() that is only expected to work > on the head of the list? > That might be the best way to resolve this, yes. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error @ 2019-12-06 18:28 ` Trond Myklebust 0 siblings, 0 replies; 20+ messages in thread From: Trond Myklebust @ 2019-12-06 18:28 UTC (permalink / raw) To: paulmck Cc: linux-nfs, linux-kernel, rcu, joel, linux-kernel-mentees, anna.schumaker On Fri, 2019-12-06 at 10:24 -0800, Paul E. McKenney wrote: > On Fri, Dec 06, 2019 at 05:52:10PM +0000, Trond Myklebust wrote: > > Hi Paul, > > > > On Fri, 2019-12-06 at 08:02 -0800, Paul E. McKenney wrote: > > > 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(). > > > > The pointer nfsi->access_cache_entry_lru is the head of the list, > > so it > > won't get poisoned. Furthermore, the objects it points to are freed > > using kfree_rcu(), so they will survive as long as we hold the rcu > > read > > lock. The object's cred pointers also points to something that is > > freed > > in an rcu-safe manner. > > > > The problem here is rather that a racing list_del() can cause nfsi- > > > access_cache_entry_lru to be empty, which is presumably why Neil > > > added > > that check plus the empty cred pointer check in the following line. > > > > The barrier semantics may be suspect, although the spin unlock > > after > > list_del() should presumably guarantee release semantics? > > Ah, OK, so you are only ever using ->prev only from the head of the > list, > and presumably never do list_del() on the head itself. (Don't laugh, > this does really happen as a way to remove the entire list, though > perhaps with list_del_init() rather than list_del().) Correct. > Maybe we should have a list_tail_rcu() that is only expected to work > on the head of the list? > That might be the best way to resolve this, yes. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error 2019-12-06 18:28 ` [Linux-kernel-mentees] " Trond Myklebust @ 2019-12-06 18:45 ` Paul E. McKenney -1 siblings, 0 replies; 20+ messages in thread From: Paul E. McKenney @ 2019-12-06 18:45 UTC (permalink / raw) To: Trond Myklebust Cc: linux-kernel-mentees, madhuparnabhowmik04, linux-nfs, rcu, anna.schumaker, linux-kernel, joel On Fri, Dec 06, 2019 at 06:28:14PM +0000, Trond Myklebust wrote: > On Fri, 2019-12-06 at 10:24 -0800, Paul E. McKenney wrote: > > On Fri, Dec 06, 2019 at 05:52:10PM +0000, Trond Myklebust wrote: > > > Hi Paul, > > > > > > On Fri, 2019-12-06 at 08:02 -0800, Paul E. McKenney wrote: > > > > 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(). > > > > > > The pointer nfsi->access_cache_entry_lru is the head of the list, > > > so it > > > won't get poisoned. Furthermore, the objects it points to are freed > > > using kfree_rcu(), so they will survive as long as we hold the rcu > > > read > > > lock. The object's cred pointers also points to something that is > > > freed > > > in an rcu-safe manner. > > > > > > The problem here is rather that a racing list_del() can cause nfsi- > > > > access_cache_entry_lru to be empty, which is presumably why Neil > > > > added > > > that check plus the empty cred pointer check in the following line. > > > > > > The barrier semantics may be suspect, although the spin unlock > > > after > > > list_del() should presumably guarantee release semantics? > > > > Ah, OK, so you are only ever using ->prev only from the head of the > > list, > > and presumably never do list_del() on the head itself. (Don't laugh, > > this does really happen as a way to remove the entire list, though > > perhaps with list_del_init() rather than list_del().) > > Correct. > > > Maybe we should have a list_tail_rcu() that is only expected to work > > on the head of the list? > > That might be the best way to resolve this, yes. Madhuparna, would you be willing to do a patch series along these lines? Thanx, Paul ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error @ 2019-12-06 18:45 ` Paul E. McKenney 0 siblings, 0 replies; 20+ messages in thread From: Paul E. McKenney @ 2019-12-06 18:45 UTC (permalink / raw) To: Trond Myklebust Cc: linux-nfs, linux-kernel, rcu, joel, linux-kernel-mentees, anna.schumaker On Fri, Dec 06, 2019 at 06:28:14PM +0000, Trond Myklebust wrote: > On Fri, 2019-12-06 at 10:24 -0800, Paul E. McKenney wrote: > > On Fri, Dec 06, 2019 at 05:52:10PM +0000, Trond Myklebust wrote: > > > Hi Paul, > > > > > > On Fri, 2019-12-06 at 08:02 -0800, Paul E. McKenney wrote: > > > > 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(). > > > > > > The pointer nfsi->access_cache_entry_lru is the head of the list, > > > so it > > > won't get poisoned. Furthermore, the objects it points to are freed > > > using kfree_rcu(), so they will survive as long as we hold the rcu > > > read > > > lock. The object's cred pointers also points to something that is > > > freed > > > in an rcu-safe manner. > > > > > > The problem here is rather that a racing list_del() can cause nfsi- > > > > access_cache_entry_lru to be empty, which is presumably why Neil > > > > added > > > that check plus the empty cred pointer check in the following line. > > > > > > The barrier semantics may be suspect, although the spin unlock > > > after > > > list_del() should presumably guarantee release semantics? > > > > Ah, OK, so you are only ever using ->prev only from the head of the > > list, > > and presumably never do list_del() on the head itself. (Don't laugh, > > this does really happen as a way to remove the entire list, though > > perhaps with list_del_init() rather than list_del().) > > Correct. > > > Maybe we should have a list_tail_rcu() that is only expected to work > > on the head of the list? > > That might be the best way to resolve this, yes. Madhuparna, would you be willing to do a patch series along these lines? Thanx, Paul _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error 2019-12-06 16:02 ` [Linux-kernel-mentees] " Paul E. McKenney @ 2019-12-12 21:55 ` Joel Fernandes -1 siblings, 0 replies; 20+ messages in thread From: Joel Fernandes @ 2019-12-12 21:55 UTC (permalink / raw) To: Paul E. McKenney Cc: madhuparnabhowmik04, trond.myklebust, anna.schumaker, linux-nfs, rcu, linux-kernel-mentees, linux-kernel On Fri, Dec 06, 2019 at 08:02:38AM -0800, Paul E. McKenney wrote: Thanks for fixing these issues and I caught up with all the patches. > > 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: On a related topic, I was trying to reason about how one could come up with bidirectional traversal without ever getting rid of poisoning. As you noted in another post, if during traversal, the node is deleted and poisoned, then the traverser can access a poisoned pointer. If the list is being traversed in reverse (by following prev), then poisioning could hurt it. Even with the below modifications, poisoning would still hurt it. No? Were you suggesting to remove poisoning for such bidirectional RCU list? Sorry if I missed something. thanks, - Joel > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error @ 2019-12-12 21:55 ` Joel Fernandes 0 siblings, 0 replies; 20+ messages in thread From: Joel Fernandes @ 2019-12-12 21:55 UTC (permalink / raw) To: Paul E. McKenney Cc: linux-nfs, linux-kernel, anna.schumaker, rcu, linux-kernel-mentees, trond.myklebust On Fri, Dec 06, 2019 at 08:02:38AM -0800, Paul E. McKenney wrote: Thanks for fixing these issues and I caught up with all the patches. > > 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: On a related topic, I was trying to reason about how one could come up with bidirectional traversal without ever getting rid of poisoning. As you noted in another post, if during traversal, the node is deleted and poisoned, then the traverser can access a poisoned pointer. If the list is being traversed in reverse (by following prev), then poisioning could hurt it. Even with the below modifications, poisoning would still hurt it. No? Were you suggesting to remove poisoning for such bidirectional RCU list? Sorry if I missed something. thanks, - Joel > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error 2019-12-12 21:55 ` [Linux-kernel-mentees] " Joel Fernandes @ 2019-12-13 1:16 ` Paul E. McKenney -1 siblings, 0 replies; 20+ messages in thread From: Paul E. McKenney @ 2019-12-13 1:16 UTC (permalink / raw) To: Joel Fernandes Cc: madhuparnabhowmik04, trond.myklebust, anna.schumaker, linux-nfs, rcu, linux-kernel-mentees, linux-kernel On Thu, Dec 12, 2019 at 04:55:34PM -0500, Joel Fernandes wrote: > On Fri, Dec 06, 2019 at 08:02:38AM -0800, Paul E. McKenney wrote: > > Thanks for fixing these issues and I caught up with all the patches. > > > > > 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: > > On a related topic, I was trying to reason about how one could come up with > bidirectional traversal without ever getting rid of poisoning. > > As you noted in another post, if during traversal, the node is deleted and > poisoned, then the traverser can access a poisoned pointer. If the list is > being traversed in reverse (by following prev), then poisioning could hurt > it. > > Even with the below modifications, poisoning would still hurt it. No? Were > you suggesting to remove poisoning for such bidirectional RCU list? Yes. We removed forward poisoning from list_del_rcu(), and a list_del_rcuprev() or whatever name would need to avoid poisoning both pointers. Thanx, Paul > Sorry if I missed something. > thanks, > > - Joel > > > > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error @ 2019-12-13 1:16 ` Paul E. McKenney 0 siblings, 0 replies; 20+ messages in thread From: Paul E. McKenney @ 2019-12-13 1:16 UTC (permalink / raw) To: Joel Fernandes Cc: linux-nfs, linux-kernel, anna.schumaker, rcu, linux-kernel-mentees, trond.myklebust On Thu, Dec 12, 2019 at 04:55:34PM -0500, Joel Fernandes wrote: > On Fri, Dec 06, 2019 at 08:02:38AM -0800, Paul E. McKenney wrote: > > Thanks for fixing these issues and I caught up with all the patches. > > > > > 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: > > On a related topic, I was trying to reason about how one could come up with > bidirectional traversal without ever getting rid of poisoning. > > As you noted in another post, if during traversal, the node is deleted and > poisoned, then the traverser can access a poisoned pointer. If the list is > being traversed in reverse (by following prev), then poisioning could hurt > it. > > Even with the below modifications, poisoning would still hurt it. No? Were > you suggesting to remove poisoning for such bidirectional RCU list? Yes. We removed forward poisoning from list_del_rcu(), and a list_del_rcuprev() or whatever name would need to avoid poisoning both pointers. Thanx, Paul > Sorry if I missed something. > thanks, > > - Joel > > > > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2019-12-13 1:16 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 2019-12-06 16:02 ` [Linux-kernel-mentees] " 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
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.