Re: kvm: use-after-free in process_srcu

[Paul E. McKenney]

On Wed, Jan 18, 2017 at 09:53:19AM +0100, Paolo Bonzini wrote:
> 
> 
> On 17/01/2017 21:34, Paul E. McKenney wrote:
> > Do any of your callback functions invoke call_srcu()?  (Hey, I have to ask!)
> 
> No, we only use synchronize_srcu and synchronize_srcu_expedited, so our
> only callback comes from there.

OK, so the next question is whether your code makes sure that all of its
synchronize_srcu() and synchronize_srcu_expedited() calls return before
the call to cleanup_srcu_struct().

> >>>> From: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> >>>> Subject: [PATCH] srcu: wait for all callbacks before deeming SRCU "cleaned up"
> >>>>
> >>>> Even though there are no concurrent readers, it is possible that the
> >>>> work item is queued for delayed processing when cleanup_srcu_struct is
> >>>> called.  The work item needs to be flushed before returning, or a
> >>>> use-after-free can ensue.
> >>>>
> >>>> Furthermore, because of SRCU's two-phase algorithm it may take up to
> >>>> two executions of srcu_advance_batches before all callbacks are invoked.
> >>>> This can happen if the first flush_delayed_work happens as follows
> >>>>
> >>>>                                                           srcu_read_lock
> >>>>     process_srcu
> >>>>         srcu_advance_batches
> >>>>             ...
> >>>>             if (!try_check_zero(sp, idx^1, trycount))
> >>>>                 // there is a reader
> >>>>                 return;
> >>>>         srcu_invoke_callbacks
> >>>>             ...
> >>>>                                                           srcu_read_unlock
> >>>>                                                           cleanup_srcu_struct
> >>>>                                                               flush_delayed_work
> >>>>         srcu_reschedule
> >>>>             queue_delayed_work
> >>>>
> >>>> Now flush_delayed_work returns but srcu_reschedule will *not* have cleared
> >>>> sp->running to false.
> >
> > But srcu_reschedule() sets sp->running to false if there are no callbacks.
> > And at that point, there had better be no callbacks.
> 
> There must be no callbacks in batch_queue and in batch_check0, and of
> course srcu_invoke_callbacks will have emptied batch_done as well.
> 
> However, I'm not sure that batch_check1 is always empty after the first
> flush_delayed_work *if there's no srcu_barrier* in the caller.
> srcu_advance_batches's second call to try_check_zero could have failed,
> and then srcu_reschedule will requeue the work item to advance
> batch_check1 into batch_done.

You should only need srcu_barrier() if there were calls to call_srcu().
Given that you only have synchronize_srcu() and synchronize_srcu_expedited(),
you -don't- need srcu_barrier().  What you need instead is to make sure
that all synchronize_srcu() and synchronize_srcu_expedited() have
returned before the call to cleanup_srcu_struct().

> If this is incorrect, then one flush_delayed_work is enough.  If it is
> correct, the possible alternatives are:
> 
> * srcu_barrier in the caller, flush_delayed_work+WARN_ON(sp->running) in
> cleanup_srcu_struct.  I strongly dislike this one---because we don't use
> call_srcu at all, there should be no reason to use srcu_barrier in KVM
> code.  Plus I think all other users have the same issue.
> 
> * srcu_barrier+flush_delayed_work+WARN_ON(sp->running) in
> cleanup_srcu_struct
> 
> * flush_delayed_work+flush_delayed_work+WARN_ON(sp->running) in
> cleanup_srcu_struct
> 
> * while(flush_delayed_work) in cleanup_srcu_struct
> 
> * "while(sp->running) flush_delayed_work" in cleanup_srcu_struct

My current thought is flush_delayed_work() followed by a warning if
there are any callbacks still posted, and also as you say sp->running.

Thoughts?

							Thanx, Paul