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] [day] [month] [year] [list]

Message-ID: <20191213011610.GC2889@paulmck-ThinkPad-P72>
Date:   Thu, 12 Dec 2019 17:16:10 -0800
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     madhuparnabhowmik04@...il.com, trond.myklebust@...merspace.com,
        anna.schumaker@...app.com, linux-nfs@...r.kernel.org,
        rcu@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] fs: nfs: dir.c: Fix sparse error

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