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.129.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 4D01C3E4C65 for ; Mon, 6 Jul 2026 08:23:56 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783326244; cv=none; b=pbVqvBCUex22qrsYf8R+Rca6QA6tyQjk/ovihiSZbji9+r76fOYiuQB6ctmBGXVbu+HjhdQaP/+6X1s/CGxVGS0ATiucFiOw3EzDuaO+IRmgeYYVrlkYMCIe9+m+sA8IRrdhGszEOwzvBscFiNc6EecbAPjptoMeiUbnVheu710= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783326244; c=relaxed/simple; bh=7ln20y4IAjjrQujbE/rINypQBgH3iWErEjKlwp9kHGs=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=dquMhxD6XbAE6jiUcDEzsU/GouB2glBOnEt7g7zROJVzS6zboJMcPDmdz7HtS2rVR2Owp5EPUvBsc6iRZ1KaewCfNBVdkGpIOfmEV6gbRrUENsAKgbCHpjIK/48rTggxriNxDWENq0xCdG4vhuQiLALpoRD5WFMI751eFHoK2lI= 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=auS/Rom/; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=L4op71px; arc=none smtp.client-ip=170.10.129.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="auS/Rom/"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="L4op71px" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1783326232; 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=KiZ7rFhsrLuWgwOq/cJtjOuC/zDIDMp/izDn3la0y/g=; b=auS/Rom/ilPrKEDZ6l8JFeIVu2P6p5DiGpWbytg97tQkOIUdzVjpoVNO2zVGtpGxUAbavt Xs/FLKVoqcl75We9otbmi1YlIERc4RX5Fl5zWbdPF8lozKd7FumxWYaAhWmFD2bcMdhlp0 x/2icNvSGHebVrJWhmZSEPW5WMeklYU= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-651-J5H_XosNNQuYKbQS3txmSw-1; Mon, 06 Jul 2026 04:23:51 -0400 X-MC-Unique: J5H_XosNNQuYKbQS3txmSw-1 X-Mimecast-MFC-AGG-ID: J5H_XosNNQuYKbQS3txmSw_1783326230 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-476d52b13d7so2068677f8f.0 for ; Mon, 06 Jul 2026 01:23:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1783326230; x=1783931030; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=KiZ7rFhsrLuWgwOq/cJtjOuC/zDIDMp/izDn3la0y/g=; b=L4op71pxRtlwKaLVyF3stv9l1Q5LjjbP7TkR16G5/nZqufTXxYJ3yb9sUeowT2FrDn w8Egc11jKaD+62QvLVszjK9Gz+KQo+tYCl++LhTbNjkrfqZTQivXU6nEj6Ojtxy2dLh8 sj9kTZokeOwwgTsaDfRCgfPl+yUeJBrufgefzpFMNMhN85tCf3taYhsIC4NWNmJ6eor+ 6iF+CVO3w+qjDxndc15Ud9jQfE/Ict6gpngBkjWlYDlRU1Hu2cGLF82c6zrk83HIZsM5 gZbBR5IQJ0Feb96PAId+xb5PjR8FAxnbRAGB+Qmg3N/x5V5bhS5SYoLRuewm7jKMfI/N XTsQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1783326230; x=1783931030; h=in-reply-to:content-disposition: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; bh=KiZ7rFhsrLuWgwOq/cJtjOuC/zDIDMp/izDn3la0y/g=; b=D9W6LrdlSQTd2mjt+p4d5DCRfwbQHaDRlLv5BDcoM0zei9mqWGTj3eIpc/BZBslebd 90yCAxstm3r3ISXl62U2igntBbMUrdpiJi4IALfA+IE7cS0lhV1aWbo+p81fDhTG4PPP bGE1k3LXAojcMe45ExOWhsi79/hbakp8wpyBWZ42Zs2Nzo8KDJOOAxGx73r2B5frhImb aid6GvHXr16qeP+2TUbi/fyclY1oTtcmX0QTnk20JhaVQlDpPlGFkTsPC3uk4+VINwLS IAZc49h9zN4i1NHPBA1w0pn2zTY5E6STU2Y6OjDL/x/voNvoxIfQdo/FYjUqPmGevEqF zNXQ== X-Gm-Message-State: AOJu0YynD4iDoP4+Sk/B2622ojPxGN3dlcQ4IwBvTYpsJ/GvAyYbwDiQ N6zZlod9ykLOub3rOOh8o39P0321zMvOfrwWWm4OWb5JfvItXUCl40bfDUkmE+Sv/miW+lH9Kq2 OSrRskU7IJwe8Q32XBWaf9twdSDWU8cS22w1L2kXcPGmiV0kJ7Z8hUvF6v7GFcQH3LQ== X-Gm-Gg: AfdE7cmF0us+uwrljYLHJe/dUbhxGH3+U5OFffAOeoGMwfYtc1C53C5RDw6eK0kSL0J rwGzavl5H0+XUPEAOljHp5BEb6PlfzaC5jHmBSMJL+hxqfCjvrGGo9gqiK3oXX/rNg41wdavvS6 OFQkBCV+Wx8nTVQ3h5nKhCVEcqIRKl/acl4HlIHtFAIGcwqCRBSq7nLG3fYQh3zNnTR/JBTeO1b L2HlRf4D94S8dgeWcOTL39DdFr4pYplci3ccb2P4fZh+hFde4mMFX/rOHU0y/UesAUNEH7Bs3bA vfIm6KrxgKvZlcFkgbb21d2cp5M6wi/KTzc5nxAFr7zh6mYGdPyw81sxW8AzjqxTA1rhjm+fiF8 eFaJpTbe1493HjME1waQMmh/ws2WtnUM9 X-Received: by 2002:a05:600c:a01:b0:493:a7bc:5bcf with SMTP id 5b1f17b1804b1-493d11f033cmr112299245e9.24.1783326230318; Mon, 06 Jul 2026 01:23:50 -0700 (PDT) X-Received: by 2002:a05:600c:a01:b0:493:a7bc:5bcf with SMTP id 5b1f17b1804b1-493d11f033cmr112298735e9.24.1783326229699; Mon, 06 Jul 2026 01:23:49 -0700 (PDT) Received: from redhat.com (IGLD-80-230-68-31.inter.net.il. [80.230.68.31]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-493c637bc21sm635500545e9.7.2026.07.06.01.23.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Jul 2026 01:23:49 -0700 (PDT) Date: Mon, 6 Jul 2026 04:23:46 -0400 From: "Michael S. Tsirkin" To: "David Hildenbrand (Arm)" Cc: linux-kernel@vger.kernel.org, Jason Wang , Xuan Zhuo , Eugenio =?iso-8859-1?Q?P=E9rez?= , Adam Litke , Rusty Russell , virtualization@lists.linux.dev, Andrew Morton , linux-mm@kvack.org Subject: Re: [PATCH v2] virtio_balloon: prime stats vq after virtio_device_ready() Message-ID: <20260706042207-mutt-send-email-mst@kernel.org> References: <2f0d0de033f3005d3b985883be3e5cf37b3f6c42.1783278596.git.mst@redhat.com> <13794c5e-375a-4fc5-84b4-a637d077f8a0@kernel.org> 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 Content-Disposition: inline In-Reply-To: <13794c5e-375a-4fc5-84b4-a637d077f8a0@kernel.org> On Mon, Jul 06, 2026 at 09:28:50AM +0200, David Hildenbrand (Arm) wrote: > On 7/5/26 21:12, Michael S. Tsirkin wrote: > > The virtio spec requires the driver not to kick the device before > > DRIVER_OK is set. init_vqs() primes the stats virtqueue with a buffer > > and kicks the device before virtio_device_ready() is called in > > virtballoon_probe(), violating this requirement. > > > > Further, if the device responds to the early kick by processing the > > buffer before DRIVER_OK, stats_request() fires and queues > > update_balloon_stats_work. Should probe then fail and free vb, the work > > runs against freed memory. > > > > To fix, move buffer setup to after DRIVER_OK. Be careful to > > disable update_balloon_stats_work while this is going on, > > to make sure it does not race with the setup. > > > > setup_vqs() warns but does not fail probe or restore if > > virtqueue_add_outbuf() fails; the call never actually fails in these > > contexts since the queue is freshly initialized and empty. > > > > Testing: tested that stats still work after the change. > > > > Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)") > > Reported-by: Sashiko:gemini-3.1-pro-preview > > Cc: David Hildenbrand > > Assisted-by: Claude:claude-sonnet-4-6 > > Signed-off-by: Michael S. Tsirkin > > --- > > > > changes from v1: > > check that work enable/disable is balanced > > explain how add buf never fails in probe/restore > > > > drivers/virtio/virtio_balloon.c | 50 +++++++++++++++++++++------------ > > 1 file changed, 32 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > > index 088b3a0e6ce6..bc0a2b19ca7d 100644 > > --- a/drivers/virtio/virtio_balloon.c > > +++ b/drivers/virtio/virtio_balloon.c > > @@ -611,25 +611,8 @@ static int init_vqs(struct virtio_balloon *vb) > > vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE]; > > vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE]; > > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { > > - struct scatterlist sg; > > - unsigned int num_stats; > > vb->stats_vq = vqs[VIRTIO_BALLOON_VQ_STATS]; > > - > > - /* > > - * Prime this virtqueue with one buffer so the hypervisor can > > - * use it to signal us later (it can't be broken yet!). > > - */ > > - num_stats = update_balloon_stats(vb); > > - > > - sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats); > > - err = virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, > > - GFP_KERNEL); > > - if (err) { > > - dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n", > > - __func__); > > - return err; > > - } > > - virtqueue_kick(vb->stats_vq); > > + disable_work(&vb->update_balloon_stats_work); > > That's to stop the stats queue triggering stats_request() I assume? Yes. > Is that > valid before we actually added+kicked ourselves? Why not? > Also, can't we simply handle that in stats_request(), just ignoring it there? Then we'd need to maintain a special flag and I dislike that. > Then we wouldn't have to go through the siable + enable. We'd still need to flip the flag on/off. > > } > > > > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) > > @@ -916,6 +899,33 @@ static int virtio_balloon_register_shrinker(struct virtio_balloon *vb) > > return 0; > > } > > > > +static void setup_vqs(struct virtio_balloon *vb) > > +{ > > + struct scatterlist sg; > > + unsigned int num_stats; > > + bool ret; > > + > > + if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) > > + return; > > + > > + /* > > + * Prime this virtqueue with one buffer so the hypervisor can > > + * use it to signal us later (it can't be broken yet!). > > + */ > > + num_stats = update_balloon_stats(vb); > > + sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats); > > + if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)) { > > + dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n", __func__); > > + return; > > + } > > + virtqueue_kick(vb->stats_vq); > > + > > + ret = enable_and_queue_work(system_freezable_wq, > > + &vb->update_balloon_stats_work); > > + /* Make sure we balanced enable/disable, or we won't report stats. */ > > + BUG_ON(!ret); > > A WARN_ON_ONCE() should be good enough here, no need to crash the kernel (no new > BUG_ON's). Will do, thanks! > -- > Cheers, > > David