Re: [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@xxxxxxxxx wrote:
> > From: Madhuparna Bhowmik <madhuparnabhowmik04@xxxxxxxxx>
> > 
> > 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@xxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxx