linux-kernel - Re: [PATCH 2/2] virtio_net: Enforce minimum TX ring size for reliability

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250521043819-mutt-send-email-mst@kernel.org>
Date: Wed, 21 May 2025 04:39:08 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Laurent Vivier <lvivier@...hat.com>
Cc: Jason Wang <jasowang@...hat.com>, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Subject: Re: [PATCH 2/2] virtio_net: Enforce minimum TX ring size for
 reliability

On Wed, May 21, 2025 at 09:45:47AM +0200, Laurent Vivier wrote:
> On 21/05/2025 03:01, Jason Wang wrote:
> > On Tue, May 20, 2025 at 7:05 PM Laurent Vivier <lvivier@...hat.com> wrote:
> > > 
> > > The `tx_may_stop()` logic stops TX queues if free descriptors
> > > (`sq->vq->num_free`) fall below the threshold of (2 + `MAX_SKB_FRAGS`).
> > > If the total ring size (`ring_num`) is not strictly greater than this
> > > value, queues can become persistently stopped or stop after minimal
> > > use, severely degrading performance.
> > > 
> > > A single sk_buff transmission typically requires descriptors for:
> > > - The virtio_net_hdr (1 descriptor)
> > > - The sk_buff's linear data (head) (1 descriptor)
> > > - Paged fragments (up to MAX_SKB_FRAGS descriptors)
> > > 
> > > This patch enforces that the TX ring size ('ring_num') must be strictly
> > > greater than (2 + MAX_SKB_FRAGS). This ensures that the ring is
> > > always large enough to hold at least one maximally-fragmented packet
> > > plus at least one additional slot.
> > > 
> > > Reported-by: Lei Yang <leiyang@...hat.com>
> > > Signed-off-by: Laurent Vivier <lvivier@...hat.com>
> > > ---
> > >   drivers/net/virtio_net.c | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index e53ba600605a..866961f368a2 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -3481,6 +3481,12 @@ static int virtnet_tx_resize(struct virtnet_info *vi, struct send_queue *sq,
> > >   {
> > >          int qindex, err;
> > > 
> > > +       if (ring_num <= 2+MAX_SKB_FRAGS) {
> > 
> > Nit: space is probably needed around "+"
> 
> I agree, but I kept the original syntax used everywhere in the file. It
> eases the search of the value in the file.


it's a mixed bag:

drivers/net/virtio_net.c:       struct scatterlist sg[MAX_SKB_FRAGS + 2];
drivers/net/virtio_net.c:       struct scatterlist sg[MAX_SKB_FRAGS + 2];
drivers/net/virtio_net.c:       if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
drivers/net/virtio_net.c:       if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
drivers/net/virtio_net.c:                       if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
drivers/net/virtio_net.c:       if (*num_buf > MAX_SKB_FRAGS + 1)
drivers/net/virtio_net.c:       if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
drivers/net/virtio_net.c:               if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) {
drivers/net/virtio_net.c:       if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) {
drivers/net/virtio_net.c:               vi->big_packets_num_skbfrags = guest_gso ? MAX_SKB_FRAGS : DIV_ROUND_UP(mtu, PAGE_SIZE);


we should fix it all. I think MAX_SKB_FRAGS + 2 is also cleaner than the
weird 2 + syntax.



> > 
> > > +               netdev_err(vi->dev, "tx size (%d) cannot be smaller than %d\n",
> > > +                          ring_num, 2+MAX_SKB_FRAGS);
> > 
> > And here.
> > 
> > > +               return -EINVAL;
> > > +       }
> > > +
> > >          qindex = sq - vi->sq;
> > > 
> > >          virtnet_tx_pause(vi, sq);
> > > --
> > > 2.49.0
> > > 
> > 
> > Other than this.
> > 
> > Acked-by: Jason Wang <jasowang@...hat.com>
> > 
> > (Maybe we can proceed on don't stall if we had at least 1 left if
> > indirect descriptors are supported).
> 
> But in this case, how to know when to stall the queue?
> 
> Thank,
> Laurent
> > 
> > Thanks
> >