From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753119AbdASWDd (ORCPT ); Thu, 19 Jan 2017 17:03:33 -0500 Received: from mail-yw0-f195.google.com ([209.85.161.195]:33113 "EHLO mail-yw0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753045AbdASWDa (ORCPT ); Thu, 19 Jan 2017 17:03:30 -0500 MIME-Version: 1.0 In-Reply-To: References: <754246063.9562871.1484603305281.JavaMail.zimbra@redhat.com> <20170117203436.GC5238@linux.vnet.ibm.com> <84cdf3bd-e2b2-0a42-05d9-2163d3535a2f@redhat.com> <20170118221526.GO5238@linux.vnet.ibm.com> From: Paul McKenney Date: Thu, 19 Jan 2017 13:52:25 -0800 Message-ID: Subject: Re: kvm: use-after-free in process_srcu To: Paolo Bonzini Cc: Paul McKenney , Dmitry Vyukov , Steve Rutherford , syzkaller , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , KVM list , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org (Trouble with VPN, so replying from gmail.) On Thu, Jan 19, 2017 at 1:27 AM, Paolo Bonzini wrote: > > > On 18/01/2017 23:15, Paul E. McKenney wrote: >> 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(). > > It certainly should! Or at least that would be our bug. > >> 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(). > > Ok, good. > >>> 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. > > Yes, that would work for KVM and anyone else who doesn't use call_srcu > (and order synchronize_srcu correctly against destruction). > > On the other hand, users of call_srcu, such as rcutorture, _do_ need to > place an srcu_barrier before cleanup_srcu_struct, or they need two > flush_delayed_work() calls back to back in cleanup_srcu_struct. Do you > agree? The reason I am resisting the notion of placing an srcu_barrier() in the cleanup_srcu_struct path is that most users don't use call_srcu(), and I don't feel that we should be inflicting the srcu_barrier() performance penalty on them. So I agree with the user invoking call_srcu() after preventing future calls to call_srcu(), and with there being a flush_delayed_work() in cleanup_srcu_struct(). As in the untested (and probably whitespace-mangled) patch below. Thoughts? Thanx, Paul diff --git a/kernel/rcu/srcu.c b/kernel/rcu/srcu.c index f2abfbae258c..5813ed848821 100644 --- a/kernel/rcu/srcu.c +++ b/kernel/rcu/srcu.c @@ -65,6 +65,17 @@ static inline bool rcu_batch_empty(struct rcu_batch *b) } /* + * Are all batches empty for the specified srcu_struct? + */ +static inline bool rcu_all_batches_empty(struct srcu_struct *sp) +{ + return rcu_batch_empty(&sp->batch_done) && + rcu_batch_empty(&sp->batch_check1) && + rcu_batch_empty(&sp->batch_check0) && + rcu_batch_empty(&sp->batch_queue); +} + +/* * Remove the callback at the head of the specified rcu_batch structure * and return a pointer to it, or return NULL if the structure is empty. */ @@ -251,6 +262,11 @@ void cleanup_srcu_struct(struct srcu_struct *sp) { if (WARN_ON(srcu_readers_active(sp))) return; /* Leakage unless caller handles error. */ + if (WARN_ON(!rcu_all_batches_empty(sp))) + return; /* Leakage unless caller handles error. */ + flush_delayed_work(&sp->work); + if (WARN_ON(sp->running)) + return; /* Caller forgot to stop doing call_srcu()? */ free_percpu(sp->per_cpu_ref); sp->per_cpu_ref = NULL; } @@ -610,15 +626,9 @@ static void srcu_reschedule(struct srcu_struct *sp) { bool pending = true; - if (rcu_batch_empty(&sp->batch_done) && - rcu_batch_empty(&sp->batch_check1) && - rcu_batch_empty(&sp->batch_check0) && - rcu_batch_empty(&sp->batch_queue)) { + if (rcu_all_batches_empty(sp)) { spin_lock_irq(&sp->queue_lock); - if (rcu_batch_empty(&sp->batch_done) && - rcu_batch_empty(&sp->batch_check1) && - rcu_batch_empty(&sp->batch_check0) && - rcu_batch_empty(&sp->batch_queue)) { + if (rcu_all_batches_empty(sp)) { sp->running = false; pending = false; }