From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751993AbdASCnU (ORCPT ); Wed, 18 Jan 2017 21:43:20 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:58198 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751867AbdASCnS (ORCPT ); Wed, 18 Jan 2017 21:43:18 -0500 Date: Wed, 18 Jan 2017 14:15:26 -0800 From: "Paul E. McKenney" To: Paolo Bonzini Cc: Dmitry Vyukov , Steve Rutherford , syzkaller , Radim =?utf-8?B?S3LEjW3DocWZ?= , KVM list , LKML Subject: Re: kvm: use-after-free in process_srcu Reply-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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <84cdf3bd-e2b2-0a42-05d9-2163d3535a2f@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 17011822-0020-0000-0000-00000B1DEFD7 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006458; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000199; SDB=6.00809511; UDB=6.00394369; IPR=6.00586869; BA=6.00005072; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00013969; XFM=3.00000011; UTC=2017-01-18 22:29:22 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17011822-0021-0000-0000-000059544189 Message-Id: <20170118221526.GO5238@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-01-18_13:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=2 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1612050000 definitions=main-1701180273 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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