[Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

[Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

[madhuparnabhowmik04]

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

Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

[Joel Fernandes]

+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

Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

[Paul E. McKenney]

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

Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

[Paul E. McKenney]

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

Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

[Trond Myklebust]

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

Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

[Paul E. McKenney]

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

Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

[Trond Myklebust]

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

Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

[Paul E. McKenney]

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

Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

[Joel Fernandes]

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

Re: [Linux-kernel-mentees] [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

[Paul E. McKenney]

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