Received: by 2002:a25:ab43:0:0:0:0:0 with SMTP id u61csp375490ybi; Fri, 31 May 2019 02:58:40 -0700 (PDT) X-Google-Smtp-Source: APXvYqx45vHQuWRCa3mSnLbWg1vgDa+fqZWp81NlaySqk5NVR5a1W0B+CZQzptRTBd5LNVDYbROZ X-Received: by 2002:a17:90a:480a:: with SMTP id a10mr332178pjh.57.1559296720591; Fri, 31 May 2019 02:58:40 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1559296720; cv=none; d=google.com; s=arc-20160816; b=uXpfMTx0ZyYGAnBkph0H5rtEKXwyxNR5N13QgcQXjgrsdlJ8kOFPrLxtlUiFADWmo4 i3JRtOcUimjFkiGDWUZa+OffcJGHRDzzTC1TWY5oQYjipyLvXNCOu92tP86+if/coKio J/x0BeL1n4aA1LyPUk7Pid9BdwWWRxQ2AukHtlGl/9ls1EVQ9CGoG0z1sj7U2GXDfl8S MIV5jZ6vDQ1OWWPNsYkRXOR9kERU1k2RxZgYzQOPNfyZZ/thQicvhByLkDoJ5pKosrGw p7p8++QJeEa9OAHfyPkIU54iZUQ1EXPR8HgM8pQQwL4ay4dUSGwczmDyAx/GFWzzGaAN aedA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-language :content-transfer-encoding:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject; bh=RZWYMyG3XxdDOU6uzR8kqsec4MIInI3eRGgZVq5MS6A=; b=TnF947kpMbXkr1Lx+bzY9GxsA9IGzD783kvUhOSjO0Mv27p6XY53whz4HbZers4pgT rxWIeCxP5eVx9PFbLBRbwem4zG3Qw8d/8TNpgzYaDPmEwMPkV5oA2QrVJbJ/lelbk2vB PhPglMRbc2YDpY+0zLzUAmCZTin1HlG3plYJnqcYjI9ZZWY6uKdd7MpSt3Vpj7CKfjZe P3Ux3qoiM3DcEYvCFXxq0kZl/pV47EJBAiajFbG4SuZn/2n1SaAoRcYNhoZBtS5VNxQw imptGKUiaiGZ0iZbp3XtEZ5P/2wxqLLSg1lUMLdlggNPYxN+6yk8X3zvNxlKyq56oqEp w2lQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id p24si5308964plr.269.2019.05.31.02.58.24; Fri, 31 May 2019 02:58:40 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727231AbfEaJ4u (ORCPT + 99 others); Fri, 31 May 2019 05:56:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41582 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726002AbfEaJ4t (ORCPT ); Fri, 31 May 2019 05:56:49 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C24539FFDA; Fri, 31 May 2019 09:56:48 +0000 (UTC) Received: from [10.72.12.182] (ovpn-12-182.pek2.redhat.com [10.72.12.182]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2743A5DA34; Fri, 31 May 2019 09:56:40 +0000 (UTC) Subject: Re: [PATCH 3/4] vsock/virtio: fix flush of works during the .remove() To: Stefano Garzarella Cc: "Michael S . Tsirkin" , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, Stefan Hajnoczi , "David S. Miller" References: <20190528105623.27983-1-sgarzare@redhat.com> <20190528105623.27983-4-sgarzare@redhat.com> <9ac9fc4b-5c39-2503-dfbb-660a7bdcfbfd@redhat.com> <20190529105832.oz3sagbne5teq3nt@steredhat> <8c9998c8-1b9c-aac6-42eb-135fcb966187@redhat.com> <20190530101036.wnjphmajrz6nz6zc@steredhat.homenet.telecomitalia.it> <4c881585-8fee-0a53-865c-05d41ffb8ed1@redhat.com> <20190531081824.p6ylsgvkrbckhqpx@steredhat> From: Jason Wang Message-ID: Date: Fri, 31 May 2019 17:56:39 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: <20190531081824.p6ylsgvkrbckhqpx@steredhat> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Fri, 31 May 2019 09:56:49 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2019/5/31 下午4:18, Stefano Garzarella wrote: > On Thu, May 30, 2019 at 07:59:14PM +0800, Jason Wang wrote: >> On 2019/5/30 下午6:10, Stefano Garzarella wrote: >>> On Thu, May 30, 2019 at 05:46:18PM +0800, Jason Wang wrote: >>>> On 2019/5/29 下午6:58, Stefano Garzarella wrote: >>>>> On Wed, May 29, 2019 at 11:22:40AM +0800, Jason Wang wrote: >>>>>> On 2019/5/28 下午6:56, Stefano Garzarella wrote: >>>>>>> @@ -690,6 +693,9 @@ static void virtio_vsock_remove(struct virtio_device *vdev) >>>>>>> vsock->event_run = false; >>>>>>> mutex_unlock(&vsock->event_lock); >>>>>>> + /* Flush all pending works */ >>>>>>> + virtio_vsock_flush_works(vsock); >>>>>>> + >>>>>>> /* Flush all device writes and interrupts, device will not use any >>>>>>> * more buffers. >>>>>>> */ >>>>>>> @@ -726,6 +732,11 @@ static void virtio_vsock_remove(struct virtio_device *vdev) >>>>>>> /* Delete virtqueues and flush outstanding callbacks if any */ >>>>>>> vdev->config->del_vqs(vdev); >>>>>>> + /* Other works can be queued before 'config->del_vqs()', so we flush >>>>>>> + * all works before to free the vsock object to avoid use after free. >>>>>>> + */ >>>>>>> + virtio_vsock_flush_works(vsock); >>>>>> Some questions after a quick glance: >>>>>> >>>>>> 1) It looks to me that the work could be queued from the path of >>>>>> vsock_transport_cancel_pkt() . Is that synchronized here? >>>>>> >>>>> Both virtio_transport_send_pkt() and vsock_transport_cancel_pkt() can >>>>> queue work from the upper layer (socket). >>>>> >>>>> Setting the_virtio_vsock to NULL, should synchronize, but after a careful look >>>>> a rare issue could happen: >>>>> we are setting the_virtio_vsock to NULL at the start of .remove() and we >>>>> are freeing the object pointed by it at the end of .remove(), so >>>>> virtio_transport_send_pkt() or vsock_transport_cancel_pkt() may still be >>>>> running, accessing the object that we are freed. >>>> Yes, that's my point. >>>> >>>> >>>>> Should I use something like RCU to prevent this issue? >>>>> >>>>> virtio_transport_send_pkt() and vsock_transport_cancel_pkt() >>>>> { >>>>> rcu_read_lock(); >>>>> vsock = rcu_dereference(the_virtio_vsock_mutex); >>>> RCU is probably a way to go. (Like what vhost_transport_send_pkt() did). >>>> >>> Okay, I'm going this way. >>> >>>>> ... >>>>> rcu_read_unlock(); >>>>> } >>>>> >>>>> virtio_vsock_remove() >>>>> { >>>>> rcu_assign_pointer(the_virtio_vsock_mutex, NULL); >>>>> synchronize_rcu(); >>>>> >>>>> ... >>>>> >>>>> free(vsock); >>>>> } >>>>> >>>>> Could there be a better approach? >>>>> >>>>> >>>>>> 2) If we decide to flush after dev_vqs(), is tx_run/rx_run/event_run still >>>>>> needed? It looks to me we've already done except that we need flush rx_work >>>>>> in the end since send_pkt_work can requeue rx_work. >>>>> The main reason of tx_run/rx_run/event_run is to prevent that a worker >>>>> function is running while we are calling config->reset(). >>>>> >>>>> E.g. if an interrupt comes between virtio_vsock_flush_works() and >>>>> config->reset(), it can queue new works that can access the device while >>>>> we are in config->reset(). >>>>> >>>>> IMHO they are still needed. >>>>> >>>>> What do you think? >>>> I mean could we simply do flush after reset once and without tx_rx/rx_run >>>> tricks? >>>> >>>> rest(); >>>> >>>> virtio_vsock_flush_work(); >>>> >>>> virtio_vsock_free_buf(); >>> My only doubt is: >>> is it safe to call config->reset() while a worker function could access >>> the device? >>> >>> I had this doubt reading the Michael's advice[1] and looking at >>> virtnet_remove() where there are these lines before the config->reset(): >>> >>> /* Make sure no work handler is accessing the device. */ >>> flush_work(&vi->config_work); >>> >>> Thanks, >>> Stefano >>> >>> [1] https://lore.kernel.org/netdev/20190521055650-mutt-send-email-mst@kernel.org >> >> Good point. Then I agree with you. But if we can use the RCU to detect the >> detach of device from socket for these, it would be even better. >> > What about checking 'the_virtio_vsock' in the worker functions in a RCU > critical section? > In this way, I can remove the rx_run/tx_run/event_run. > > Do you think it's cleaner? Yes, I think so. Thanks > > Thank you very much, > Stefano