From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751506AbdASJ11 (ORCPT ); Thu, 19 Jan 2017 04:27:27 -0500 Received: from mail-lf0-f65.google.com ([209.85.215.65]:34706 "EHLO mail-lf0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751323AbdASJ1N (ORCPT ); Thu, 19 Jan 2017 04:27:13 -0500 Subject: Re: kvm: use-after-free in process_srcu To: paulmck@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> <20170118221526.GO5238@linux.vnet.ibm.com> Cc: Dmitry Vyukov , Steve Rutherford , syzkaller , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , KVM list , LKML From: Paolo Bonzini Message-ID: Date: Thu, 19 Jan 2017 10:27:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20170118221526.GO5238@linux.vnet.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? Thanks, Paolo