From patchwork Thu Jan 19 21:52:25 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul McKenney X-Patchwork-Id: 9526969 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 99C876020B for ; Thu, 19 Jan 2017 22:03:52 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 89E1928662 for ; Thu, 19 Jan 2017 22:03:52 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7D01E28665; Thu, 19 Jan 2017 22:03:52 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.3 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 13C4228662 for ; Thu, 19 Jan 2017 22:03:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753062AbdASWDb (ORCPT ); Thu, 19 Jan 2017 17:03:31 -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 Received: by mail-yw0-f195.google.com with SMTP id u68so6445485ywg.0; Thu, 19 Jan 2017 14:03:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=fmiX2SW/4cOdF5X3aETrSkEvpbO0DZAdyi7QxnaeB5E=; b=Bkg1ZQzifHDr5xzR6/jRvAmyzX2Va2VgzDB+miYX8AMoONzUtz2PCZzoXFOdX7odHk Z6pNrd77nfgdoO0hHp3W0l9pHjxkqEc1oIE1H0VyoCzNGZrLLdqWAZUNdToZLeLKczDi Gn1SsZjZxtwGc1XJrmex/wFJa2YDL+9JVsz2H8EEAcJqpcr0I/wcG0+7xeUNDOuRtzEn HF80PmgeF4UCxL1hmuZA6Ggt+Nz/FcMS8F2IHB8PgBXqtjcAiKBxzQ9n8w0UJUn2VA14 8hPFB+UYrw8LDYKO7X6S/UzVBVk3OzFa8WLzYokK1fPQ33AenSfrMSZgOGvPn73TUowq KVeQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=fmiX2SW/4cOdF5X3aETrSkEvpbO0DZAdyi7QxnaeB5E=; b=hA1vxyobvwubs3Fhm39wpSqNz/hl4sogCqFNCYgyAqJe2fz6DFKPWdHb3yiF2ps7Qo GEHJx76ooZPcjtxWeWCKVHW+OTNGFLXzw3QA85RBc04mzEtHB/WNRsgCI/1oOwpCShPB ya74m9qavverDb5zV31TfGVi+GeMT5PMnlMG+UCO9f8Gt2Bp+ue/Q+932Gi9mwlcD3Aj p6LghW0NdX7aAoJ7zm0pLffckLxwSWvHP6Ow4ndvOF95zUwpdm1uuWQRzrXFjES5QCIf 09jRV4zZnOKEmmX63IEhMg11th0mYoJG27ftYY9FE7Ncx/TvXpxZ/BVM/SrADY+l0NtG fKzQ== X-Gm-Message-State: AIkVDXIrn1+iFGd6VvbOrpV4EhYpSpDX6z7g7uaWLf5svtdfkNENTjeWfKdUf3Xkb6zpWLl4CVcOnOFNyZmMDw== X-Received: by 10.55.174.195 with SMTP id x186mr9947476qke.87.1484862746135; Thu, 19 Jan 2017 13:52:26 -0800 (PST) MIME-Version: 1.0 Received: by 10.12.176.15 with HTTP; Thu, 19 Jan 2017 13:52:25 -0800 (PST) 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 Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP (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 --- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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; }