From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Paul E. McKenney" Subject: Re: kvm: use-after-free in process_srcu Date: Wed, 18 Jan 2017 14:15:26 -0800 Message-ID: <20170118221526.GO5238@linux.vnet.ibm.com> References: <754246063.9562871.1484603305281.JavaMail.zimbra@redhat.com> <20170117203436.GC5238@linux.vnet.ibm.com> <84cdf3bd-e2b2-0a42-05d9-2163d3535a2f@redhat.com> Reply-To: paulmck@linux.vnet.ibm.com Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Dmitry Vyukov , Steve Rutherford , syzkaller , Radim =?utf-8?B?S3LEjW3DocWZ?= , KVM list , LKML To: Paolo Bonzini Return-path: Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:39648 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751342AbdASB4u (ORCPT ); Wed, 18 Jan 2017 20:56:50 -0500 Received: from pps.filterd (m0098414.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v0IMScaG078973 for ; Wed, 18 Jan 2017 17:29:24 -0500 Received: from e36.co.us.ibm.com (e36.co.us.ibm.com [32.97.110.154]) by mx0b-001b2d01.pphosted.com with ESMTP id 282h4ag50c-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 18 Jan 2017 17:29:24 -0500 Received: from localhost by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 18 Jan 2017 15:29:23 -0700 Content-Disposition: inline In-Reply-To: <84cdf3bd-e2b2-0a42-05d9-2163d3535a2f@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 > >>>> 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