From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7CF8A4CA296 for ; Thu, 23 Jul 2026 14:39:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784817569; cv=none; b=ig0W9KKxCQmPd8fXe6WknvylD7nKj1NU1kJVC6nd+YD1j1gJ4lnVJMQJjYfi98nqJtKX1atcuJcxx6xAq9LGCbnj48efzxmBB1I5E7Wp9NE+ZNYEbRw+iURNAQu9/GxMUTAOl69WZulKzICnuQ4MglvXOatmhXjc5DME0CFnC7E= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784817569; c=relaxed/simple; bh=epZHNJdb1jvaOZ4O6cy2hUvP2FOzogSGFkn2SNuLRcA=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=c+Rhu/uuo1tryNK7ADX6jIg+FocrfBYhBcYvaYVjs6o/SANx7Qu7ZazedqfmfahzKIEYdVYpMl+YBbOkrLZ0NvdRbJsgrPKyLAj+nt4fBGeQN2mov7ESGoT0IIBX/GMU01v/KOANTfwddw1Cjy6wLogVNuCSU9OEkG0vlt9HRi8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=MP8n1E3w; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=gKY6oDxr; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="MP8n1E3w"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="gKY6oDxr" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1784817560; 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: in-reply-to:in-reply-to:references:references; bh=gqFSWQaMTz+T5mXHuwtGx1/VHZD0uStv1s9M8jprGVg=; b=MP8n1E3wkpjQpj98nIEC11pHyU6OaUVvEILOWuYB/HQtvbZ/UgsLwaasA8ovyi8Hy9zFEt B4dOYSnt1Hh0SZuHKNFGIum4DlfQX8Kv1nN0CQJSix+cGa05+Cyi0QnxLUUXyMQ/PWfDVR Ci1QsCkvfoj6egblcfKV8TocKqtehFg= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-682-JKCppg6MNji2WZ-UuYjewg-1; Thu, 23 Jul 2026 10:39:18 -0400 X-MC-Unique: JKCppg6MNji2WZ-UuYjewg-1 X-Mimecast-MFC-AGG-ID: JKCppg6MNji2WZ-UuYjewg_1784817558 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-47f8580ed9eso738267f8f.0 for ; Thu, 23 Jul 2026 07:39:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1784817557; x=1785422357; darn=vger.kernel.org; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to:content-type; bh=gqFSWQaMTz+T5mXHuwtGx1/VHZD0uStv1s9M8jprGVg=; b=gKY6oDxrAy2Obu8I1vVxtX6n7bSkViclVpzfFw7u7g0FzrYd2GliCIHqIv01WbBkjR KAvMetW8vZyh2KhV0997Go81hYSe6aQe5akaFg35/mUVqWaToBb3gDY2E0Ize2fFIZCQ yLeA/5DVLvJjXA/R1s2tkD0WSiz46YNNtgKXXwsx4aOg2HDruxqtjHlnwTacda4DoxKs 3yqUR3hvkIL9ZCtIpj4weO/urH1X9Irt2sjB5tbtjRpagb1Nc7szpwlnzjzL8p/fdEkE ks1iapW/EmILr1UOoQe2HlfqhauJ3k8qbZ/WJBQsSYpa5cHKXMsOBHjr0lvoMnjiV8Bs kR9w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1784817557; x=1785422357; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=gqFSWQaMTz+T5mXHuwtGx1/VHZD0uStv1s9M8jprGVg=; b=nt2/yobf4nSZUWIDYUtPa38JLxfSsc5LdU/NQ3R7za14MTphbzX7Gf7xglj7hHrxsX lF/S6tECLSqAzYU5nES5r2vFPGq5QDENr7sSDB5A3KoweSU5lMR6zI2+MJm7zeg0I5Ta 9d6BUqHn8BuahHxtamjuTjkbar9Q3thU/LHk6FqXwrLfBa5iMWg32KmfsZ5Fg4KeWReq DkJYRQZ5s1kUBMDG75iDwtjpIzlCXs0BlckcueFxbHUdItm5VlvDcJuUyN3Zfg3++kKB 5pY5DAntBJEsSi45OQsx26YYWfp3bl9E50HNW2qAfAPgjz5Z+eMpe5lhgn0g7GWbi6cr m5Ng== X-Gm-Message-State: AOJu0Yx1kYKpm5r3HObi3E/5Knyyo0Btl56NirfMh5Fd0MRE1OizyztK lpEvrDQen9vkvP/DTLvPYuVr1VgzNV3Cmcxics4hagO0lpU6ANvBqfpnzE1L3zvnJATSdhoivyH EmR+Le2eAZJUOa/NnsMUOwEpapBZxME4KFn4VGEWAcihflRBdhOPxnEx49wcNb6F/BQ== X-Gm-Gg: AR+sD13qGvMWQX7gGkN6tu55m06hUVT4iMp5EbgrK1u3JyOq4iDepOxbVLZlAJBnph0 TucyA/5M6+omCy1naoKRhor6LBKXDRLtTARqVdmMqzgIDZMYRJ8y3v4M3nASywwtLNF+UgIDD5f uTj67fDqb66J3lt47t1MYJ/IHmWQZ5d3ol7dAzAz3ZIigM4nkslb+3C2iGc7aCpR0IvJ3qVFAWV bPbuq6cx+U8lWxY3Sem8mOcrsShOV0l+FeoazC3L9RB9lPdNsp/WZU9gIV4/3DSxpW5yucLt46n x49kHfpvNgceff3dWdfv/jLMldD8KnDblHSp9N28Y0LuLHA3xXh4MZqlDQ3W9SCTGhEyrJnZAcH MtRMurl+Z1pTCxi/Kw5wyMAtdYpFV/tGXP5uBhJgDhxXf4d1RqQ== X-Received: by 2002:a5d:5e87:0:b0:47f:6d9a:d7a6 with SMTP id ffacd0b85a97d-47f8d72996fmr4387783f8f.25.1784817557350; Thu, 23 Jul 2026 07:39:17 -0700 (PDT) X-Received: by 2002:a5d:5e87:0:b0:47f:6d9a:d7a6 with SMTP id ffacd0b85a97d-47f8d72996fmr4387716f8f.25.1784817556761; Thu, 23 Jul 2026 07:39:16 -0700 (PDT) Received: from sgarzare-redhat (host-82-53-135-65.retail.telecomitalia.it. [82.53.135.65]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-47f85c531basm15770037f8f.19.2026.07.23.07.39.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Jul 2026 07:39:15 -0700 (PDT) Date: Thu, 23 Jul 2026 16:39:11 +0200 From: Stefano Garzarella To: Andrey Drobyshev Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, mst@redhat.com, stefanha@redhat.com, dongli.zhang@oracle.com, maciej.szmigiero@oracle.com, bchaney@akamai.com, mark.kanda@oracle.com, ptikhomirov@virtuozzo.com, den@openvz.org Subject: Re: [PATCH v5 4/5] vhost: synchronize with RCU readers when freeing workers Message-ID: References: <20260720102241.371610-1-andrey.drobyshev@virtuozzo.com> <20260720102241.371610-5-andrey.drobyshev@virtuozzo.com> <82066fcb-150f-47ef-86a7-0df0f9eb41c5@virtuozzo.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: On Thu, Jul 23, 2026 at 05:29:12PM +0300, Andrey Drobyshev wrote: >On 7/23/26 5:03 PM, Stefano Garzarella wrote: >> On Thu, Jul 23, 2026 at 04:57:47PM +0300, Andrey Drobyshev wrote: >>> On 7/22/26 12:43 PM, Stefano Garzarella wrote: >>>> On Mon, Jul 20, 2026 at 01:22:40PM +0300, Andrey Drobyshev wrote: >>>>> vhost_vq_work_queue() only holds the RCU read lock while it dereferences >>>>> vq->worker and queues work on it. vhost_workers_free() however clears >>>>> the vq->worker pointers and immediately frees the workers, without >>>>> waiting for a grace period. A caller that fetched the worker right >>>>> before the pointer was cleared can therefore still be queueing work on >>>>> it while it is freed. And even when the queueing itself wins the race, >>>>> the work is never run, so its VHOST_WORK_QUEUED bit stays set and all >>>>> future attempts to queue it are silently skipped. >>>>> >>>>> None of the current callers can actually hit this: net and scsi stop >>>>> their virtqueues before the workers are freed, and vsock unhashes the >>>>> device and does synchronize_rcu() of its own in vhost_vsock_dev_release() >>>>> before the workers go away. But the upcoming VHOST_RESET_OWNER support >>>>> in vhost-vsock keeps the device hashed while its workers are freed, so >>>>> the lockless send/cancel paths become able to race with the teardown. >>>>> >>>>> Fix this by clearing the vq->worker pointers, waiting for a grace >>>>> period, and then flushing the workers so any work the last readers >>>>> queued runs before the workers are freed. >>>>> >>>>> Fixes: 228a27cf78af ("vhost: Allow worker switching while work is queueing") >>>>> Suggested-by: Stefano Garzarella >>>>> Signed-off-by: Andrey Drobyshev >>>>> --- >>>>> drivers/vhost/vhost.c | 11 +++++++++++ >>>>> 1 file changed, 11 insertions(+) >>>> >>>> Sashiko reported some potential issues here: >>>> https://sashiko.dev/#/patchset/20260720102241.371610-1-andrey.drobyshev@virtuozzo.com?part=4 >>>> >>>> IMO the first one is pre-existing, but not really sure it is a real >>>> issue since happening when the worker/vmm is going to be killed. >>>> >>> >>> Sashiko claims: >>> >>>> Will this leave the queued work unexecuted and permanently break the >>> virtqueue by leaving VHOST_WORK_QUEUED set? >>> >>> I agree this issue is pre-existing and doesn't have much to do with our >>> series here. It looks real, but in reality should be harmless since we >>> may only hit it while the device already dying. Means there's no VQ >>> state to be saved. The only potentially observable artifact I guess is >>> a warning here: >>> >>> vhost_workers_free() >>> vhost_worker_destroy() >>> WARN_ON(!llist_empty()) >>> >>> So more of a cosmetic noise on a dying device. Again, not relevant to >>> this series. But one optional way to make it go away would be to clear >>> vq->worker under vq->mutex in vhost_workers_free() (mirroring >>> vhost_worker_killed()). >>>> The second one also not sure if it's an issue since the sender is not >>>> lockless IIUC. >>>> >>> >>> The term 'sender' is confusing here: >>> >>> * vhost_transport_send_pkt() is the .send_pkt() method of struct >>> virtio_transport. It only stores skbs into the queue, doesn't process >>> them. It indeed is lockless as it doesn't take vq->mutex. >>> >>> * vhost_transport_send_pkt_work() is the .fn() method of send_pkt_work. >>> It's called by the worker thread to process skbs in the queue, calls >>> vhost_transport_do_send_pkt() which does in turn take vq->mutex. >>> >>> I think sashiko points out to the former. Still, I don't think it's an >>> actual bug. Look: >>> >>> 1) By invoking flush, we wake the worker thread: >>> vhost_dev_flush() >>> __vhost_worker_flush() >>> vhost_worker_queue(flush.work) >>> worker->ops->wakeup() >>> >>> 2) Then woken worker does: >>> vhost_run_work_list() >>> llist_for_each_entry_safe(work) { >>> clear_bit(VHOST_WORK_QUEUED, &work->flags) >>> work->fn(work) // for send_pkt_work = vhost_transport_send_pkt_work >>> } >>> >>> 3) And then in work->fn() (vhost_transport_send_pkt_work): >>> vhost_transport_send_pkt_work() >>> vhost_transport_do_send_pkt() >>> if (!vhost_vq_get_backend(vq)) >>> goto out; >>> >>> As you can see, we return early in case backend was unset. >>> >>> So after doing this flush, we have: 1) QUEUED bit is unset; that makes >>> send_pkt_work re-queueable. 2) But the queue doesn't actually get >>> processed, and VQ state isn't actually touched here - so I guess >>> Sashiko's conclusion is incorrect and there's no actual bug. >>> >>>> But, please can you double check them? >>>> >>> >>> In general I'd leave this patch as-is as Sashiko's complaints aren't >>> very convincing so far. If you want I can add another patch which wraps >>> NULLifying workers in vhost_workers_free() in vq->mutex. >>> >>> WDYT? >> >> Yeah, I agree, about the other patch, up to you, but I'll eventually >> send it separately. >> >> Thanks, >> Stefano >> > >Alright, then let me resend it once more along with this 6th patch, so >that we don't have it uncovered. But why it should be part of this series? If there is no strong reason, I'd send it as a separate patch. E.g. even this patch in theory may be a separate one, but this is related to this series, so makes sense to have this included. Thanks, Stefano