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,
> [email protected] wrote:
> > From: Madhuparna Bhowmik <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
[email protected]
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,
> > [email protected] wrote:
> > > From: Madhuparna Bhowmik <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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
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,
> > > [email protected] wrote:
> > > > From: Madhuparna Bhowmik <[email protected]>
> > > >
> > > > 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 <
> > > > [email protected]>
> > > > ---
> > > > 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
[email protected]
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,
> > > > [email protected] wrote:
> > > > > From: Madhuparna Bhowmik <[email protected]>
> > > > >
> > > > > 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 <
> > > > > [email protected]>
> > > > > ---
> > > > > 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