From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.4 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7E13FC433DF for ; Thu, 15 Oct 2020 12:07:27 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 01ED0218AC for ; Thu, 15 Oct 2020 12:07:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=suse.com header.i=@suse.com header.b="CZGbDe0d" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 01ED0218AC Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from list by lists.xenproject.org with outflank-mailman.7289.19006 (Exim 4.92) (envelope-from ) id 1kT22H-0006J0-Jz; Thu, 15 Oct 2020 12:07:09 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 7289.19006; Thu, 15 Oct 2020 12:07:09 +0000 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kT22H-0006It-Gc; Thu, 15 Oct 2020 12:07:09 +0000 Received: by outflank-mailman (input) for mailman id 7289; Thu, 15 Oct 2020 12:07:08 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kT22G-0006Io-KM for xen-devel@lists.xenproject.org; Thu, 15 Oct 2020 12:07:08 +0000 Received: from mx2.suse.de (unknown [195.135.220.15]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id fe2917c4-aa1d-48ac-8716-8309ff8f335a; Thu, 15 Oct 2020 12:07:07 +0000 (UTC) Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 30612ACB5; Thu, 15 Oct 2020 12:07:06 +0000 (UTC) Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kT22G-0006Io-KM for xen-devel@lists.xenproject.org; Thu, 15 Oct 2020 12:07:08 +0000 X-Inumbo-ID: fe2917c4-aa1d-48ac-8716-8309ff8f335a Received: from mx2.suse.de (unknown [195.135.220.15]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id fe2917c4-aa1d-48ac-8716-8309ff8f335a; Thu, 15 Oct 2020 12:07:07 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1602763626; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=JXafp5VLA0P0xzAG6FhQCxBZCd4G7t26VpzSvuztxtQ=; b=CZGbDe0d3TwDZcXFGygdSsa2SAr/svia9/Y/aN7DLRVccq20haXg9uTXEYkKWE1R6By1LB oeoLBp2hDtFuTSk82g9Xqrtkb40spXWuQ/2hteWg3ODiAPy3Dhqh2JIultKv6rxFVoDT6u jWK/RLdoeVFJ7CF8qdPlSTRpurNX/u8= Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 30612ACB5; Thu, 15 Oct 2020 12:07:06 +0000 (UTC) Subject: Re: [PATCH v2 1/2] xen/events: access last_priority and last_vcpu_id together To: Julien Grall Cc: =?UTF-8?B?SsO8cmdlbiBHcm/Dnw==?= , xen-devel@lists.xenproject.org, Andrew Cooper , George Dunlap , Ian Jackson , Stefano Stabellini , Wei Liu References: <20201012092740.1617-1-jgross@suse.com> <20201012092740.1617-2-jgross@suse.com> <9485004c-b739-5590-202b-c8e6f84e5e54@suse.com> <821a77d3-7e37-d1d2-d904-94db0177893a@suse.com> <350a5738-b239-e36b-59aa-05b8f86648b8@suse.com> <548f80a9-0fa3-cd9e-ec44-5cd37d98eadc@xen.org> From: Jan Beulich Message-ID: <4f4ecc8d-f5d2-81e9-1615-0f2925b928ba@suse.com> Date: Thu, 15 Oct 2020 14:07:06 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.1 MIME-Version: 1.0 In-Reply-To: <548f80a9-0fa3-cd9e-ec44-5cd37d98eadc@xen.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 14.10.2020 13:40, Julien Grall wrote: > Hi Jan, > > On 13/10/2020 15:26, Jan Beulich wrote: >> On 13.10.2020 16:20, Jürgen Groß wrote: >>> On 13.10.20 15:58, Jan Beulich wrote: >>>> On 12.10.2020 11:27, Juergen Gross wrote: >>>>> The queue for a fifo event is depending on the vcpu_id and the >>>>> priority of the event. When sending an event it might happen the >>>>> event needs to change queues and the old queue needs to be kept for >>>>> keeping the links between queue elements intact. For this purpose >>>>> the event channel contains last_priority and last_vcpu_id values >>>>> elements for being able to identify the old queue. >>>>> >>>>> In order to avoid races always access last_priority and last_vcpu_id >>>>> with a single atomic operation avoiding any inconsistencies. >>>>> >>>>> Signed-off-by: Juergen Gross >>>> >>>> I seem to vaguely recall that at the time this seemingly racy >>>> access was done on purpose by David. Did you go look at the >>>> old commits to understand whether there really is a race which >>>> can't be tolerated within the spec? >>> >>> At least the comments in the code tell us that the race regarding >>> the writing of priority (not last_priority) is acceptable. >> >> Ah, then it was comments. I knew I read something to this effect >> somewhere, recently. >> >>> Especially Julien was rather worried by the current situation. In >>> case you can convince him the current handling is fine, we can >>> easily drop this patch. >> >> Julien, in the light of the above - can you clarify the specific >> concerns you (still) have? > > Let me start with that the assumption if evtchn->lock is not held when > evtchn_fifo_set_pending() is called. If it is held, then my comment is moot. But this isn't interesting - we know there are paths where it is held, and ones (interdomain sending) where it's the remote port's lock instead which is held. What's important here is that a _consistent_ lock be held (but it doesn't need to be evtchn's). > From my understanding, the goal of lock_old_queue() is to return the > old queue used. > > last_priority and last_vcpu_id may be updated separately and I could not > convince myself that it would not be possible to return a queue that is > neither the current one nor the old one. > > The following could happen if evtchn->priority and > evtchn->notify_vcpu_id keeps changing between calls. > > pCPU0 | pCPU1 > | > evtchn_fifo_set_pending(v0,...) | > | evtchn_fifo_set_pending(v1, ...) > [...] | > /* Queue has changed */ | > evtchn->last_vcpu_id = v0 | > | -> evtchn_old_queue() > | v = d->vcpu[evtchn->last_vcpu_id]; > | old_q = ... > | spin_lock(old_q->...) > | v = ... > | q = ... > | /* q and old_q would be the same */ > | > evtchn->las_priority = priority| > > If my diagram is correct, then pCPU1 would return a queue that is > neither the current nor old one. I think I agree. > In which case, I think it would at least be possible to corrupt the > queue. From evtchn_fifo_set_pending(): > > /* > * If this event was a tail, the old queue is now empty and > * its tail must be invalidated to prevent adding an event to > * the old queue from corrupting the new queue. > */ > if ( old_q->tail == port ) > old_q->tail = 0; > > Did I miss anything? I don't think you did. The important point though is that a consistent lock is being held whenever we come here, so two racing set_pending() aren't possible for one and the same evtchn. As a result I don't think the patch here is actually needed. If I take this further, then I think I can reason why it wasn't necessary to add further locking to send_guest_{global,vcpu}_virq(): The virq_lock is the "consistent lock" protecting ECS_VIRQ ports. The spin_barrier() while closing the port guards that side against the port changing to a different ECS_* behind the sending functions' backs. And binding such ports sets ->virq_to_evtchn[] last, with a suitable barrier (the unlock). Which leaves send_guest_pirq() before we can drop the IRQ-safe locking again. I guess we would need to work towards using the underlying irq_desc's lock as consistent lock here, but this certainly isn't the case just yet, and I'm not really certain this can be achieved. Jan