Re: [PATCH RFC tip/core/rcu 23/23] vhost: add __rcu annotations

[Paul E. McKenney]

On Mon, May 17, 2010 at 09:35:28PM -0400, Mathieu Desnoyers wrote:
> * Paul E. McKenney (paulmck@xxxxxxxxxxxxxxxxxx) wrote:
> > On Mon, May 17, 2010 at 07:40:25PM -0400, Mathieu Desnoyers wrote:
> > > * Paul E. McKenney (paulmck@xxxxxxxxxxxxxxxxxx) wrote:
> > > > On Mon, May 17, 2010 at 06:00:25PM -0400, Mathieu Desnoyers wrote:

[ . . . ]

> > > > But perhaps we should be simply treating this as a use-after-free
> > > > problem, so that RCU is not directly involved. Isn't that the standard
> > > > use of debugobjects anyway?
> > > 
> > > OK so we could tie "rcu_dereference" do debugobjects, and free would be
> > > a standard free. Yes, I think it could be done. It looks a bit like the
> > > memory allocation debugging code. If we know that a certain
> > > rcu_dereference always access dynamically allocated memory, we could
> > > probably add some checks there based on the memory allocator debug
> > > objects.
> > 
> > We probably need vhost to add code at the end of the relevant RCU
> > read-side critical section checking that the pointers returned by
> > any rcu_dereference() calls still point to valid memory.  Don't get
> > me wrong, your approach could find bugs in which someone forgot to
> > remove the RCU-protected structure from a public list, but it could
> > not detect failure to wait a grace period between the time of removal
> > and the time of freeing.
> 
> Good point too. So something like a new rcu_unreference() (or feel free
> to find any better name) ;) that would be compiled out normally, but
> would call into debugobjects might do the trick. We would have to add
> these annotations to match every rcu_dereference() though, might means a
> lot of new lines of code. On the plus side, that looks like a good audit
> of RCU read-side use. ;)

My first thought is that we have added quite a bit of RCU consistency
check code in the past few months, so we should see what bugs they find
and what bugs escape.  It is all too easy to create consistency check
code that is more trouble than it is worth.

But in the meantime, let's see what would be required to check for
failures to insert grace-period delays:

o	There would need to be something like rcu_unreference(),
	rcu_no_more_readers() or some such after the grace period.
	The update side would then become something like the following:

		oldp = rcu_dereference_protected(gp, &mylock);
		rcu_assign_pointer(gp, newp);
		synchronize_rcu();
		rcu_no_more_readers(oldp);
		kfree(oldp);

o	There would need to be something to check all of the pointers
	traversed in the read-side critical sections:

		rcu_read_lock();
		...
		p1 = rcu_dereference(gp1->field1);
		...
		p2 = rcu_dereference(gp2->field2);
		...

		rcu_validate(p1);
		rcu_validate(p2);
		rcu_read_unlock();

One thing that bothers me about this is that we are forcing the developer
to do a lot of extra typing.  For example, rcu_no_more_readers() is in
a truth-and-beauty sense redundant with kfree() -- why type both?  The
same could be said about rcu_validate() and rcu_read_unlock(), but nested
RCU read-side critical sections make this difficult.

Or am I misunderstanding what you are suggesting?

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