linux-kernel - Re: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation

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]

Date:	Tue, 30 Mar 2010 10:01:37 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	David Howells <dhowells@...hat.com>
Cc:	Eric Dumazet <eric.dumazet@...il.com>, Trond.Myklebust@...app.com,
	linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] NFS: Fix RCU warnings in
 nfs_inode_return_delegation_noreclaim() [ver #2]

On Tue, Mar 30, 2010 at 05:37:49PM +0100, David Howells wrote:
> Paul E. McKenney <paulmck@...ux.vnet.ibm.com> wrote:
> 
> > rcu: Add update-side variant of rcu_dereference()
> > 
> > Upcoming consistency-checking features are requiring that even update-side
> > accesses to RCU-protected pointers use some variant of rcu_dereference().
> > Even though rcu_dereference() is quite lightweight, it does constrain the
> > compiler, thus producing code that is worse than required.  This patch
> > therefore adds rcu_dereference_update(), which allows lockdep-style
> > checks for holding the correct update-side lock, but which does not
> > constrain the compiler.
> 
> Ummm...  I'm not so keen on the name for two reasons.  Firstly, why shouldn't
> the read side do:
> 
> 	struct foo {
> 		struct bar *b;
> 	};
> 
> 	void manage_bar(struct foo *f)
> 	{
> 		struct bar *b;
> 
> 		rcu_read_lock();
> 		b = rcu_dereference(f->b);
> 		if (b)
> 			do_something_to_bar(b);
> 		rcu_read_unlock();
> 	}
> 
> 	void manage_foo(struct foo *f)
> 	{
> 		...
> 		if (f->b)
> 			manage_bar(f);
> 		...
> 	}
> 
> Why should this be limited to the update side?

The main criterion is that you have to have done something to prevent
the data structure from changing, which normally only happens on the
update side.

> Secondly, the name rcu_dereference_update() seems to imply that this function
> itself does an update, perhaps after having done an rcu_dereference().

Indeed, that is rather ugly.

> Perhaps rcu_pointer_valid()?
> 
> 	if (rcu_pointer_valid(f->b))
> 		manage_bar(f);

Hmmm...  That sounds like something that checks the pointer for
correct format or for pointing to valid data.

> or if you really do want to limit this sort of thing to the update side:
> 
> 	if (rcu_destination_for_update(f->b)) {
> 		spin_lock(&f->lock);
> 		update_bar(f);
> 		spin_unlock(&f->lock);
> 	}

What we are doing is dereferencing an RCU-protected pointer that we
are somehow preventing from being concurrently updated, usually by
holding the update-side lock.

So maybe rcu_dereference_no_updates()?  Or rcu_dereference_locked()?

But not rcu_dereference_with_concurrent_updates_somehow_prevented().  ;-)

> Another possibility is have an 'RCU write lock' that just does the lockdep
> thing and doesn't interpolate a barrier:
> 
> 	rcu_write_lock();
> 	if (rcu_dereference_for_update(f->b)) {
> 		spin_lock(&f->lock);
> 		update_bar(f->b);
> 		spin_unlock(&f->lock);
> 	}
> 	rcu_write_unlock();

This would work for the moment, but I do not believe that sparse can
deal with tracking rcu_write_lock() across compilation units.  Besides, I
really really don't want anything that looks like it is somehow protecting
the update.  Too many newcomers to RCU somehow get the impression that
rcu_assign_pointer() provides that protection as it is.  :-/

> Or might it make sense to roll together with the lock primitive:
> 
> 	if (rcu_dereference_and_lock(f->b, &f->lock)) {
> 		update_bar(f);
> 		spin_unlock(&f->lock);
> 	}
> 
> (I'm not keen on that one because you might not want to take the lock
>  immediately, and you have a wide choice of locks).

Indeed, the API explosion might be a bit unpleasant.  ;-)

So, how about rcu_dereference_locked()?

> Sorry to be picky.

Given that I worked quite happily with RCU for quite some time without
any words for any of the underlying concepts, I cannot claim that I
don't need at least some help with name selection.  ;-)

							Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/