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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2ec21ec537144bb3c0d5fbdaf88ea022d07b7ff8.camel@hammerspace.com>
Date:   Fri, 6 Dec 2019 17:52:10 +0000
From:   Trond Myklebust <trondmy@...merspace.com>
To:     "paulmck@...nel.org" <paulmck@...nel.org>,
        "madhuparnabhowmik04@...il.com" <madhuparnabhowmik04@...il.com>
CC:     "linux-kernel-mentees@...ts.linuxfoundation.org" 
        <linux-kernel-mentees@...ts.linuxfoundation.org>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "rcu@...r.kernel.org" <rcu@...r.kernel.org>,
        "anna.schumaker@...app.com" <anna.schumaker@...app.com>,
        "joel@...lfernandes.org" <joel@...lfernandes.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

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@...il.com wrote:
> > From: Madhuparna Bhowmik <madhuparnabhowmik04@...il.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@...il.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@...merspace.com