linux-kernel - RE: [PATCH] The vdpasim_net_work function handles both transmit TX completion and RX processing. A descriptor is taken from the TX VQ at the start of the loop, representing a buffer previously used for the TX path's initial read from the virtual networ...

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] [day] [month] [year] [list]

Message-ID:
 <SEZPR06MB49983E2773ADF9BFA25F9FEF85ABA@SEZPR06MB4998.apcprd06.prod.outlook.com>
Date: Wed, 17 Dec 2025 09:18:03 +0000
From: Angus Chen <angus.chen@...uarmicro.com>
To: Jason Wang <jasowang@...hat.com>
CC: "mst@...hat.com" <mst@...hat.com>, "xuanzhuo@...ux.alibaba.com"
	<xuanzhuo@...ux.alibaba.com>, "eperezma@...hat.com" <eperezma@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"virtualization@...ts.linux.dev" <virtualization@...ts.linux.dev>
Subject: RE: [PATCH] The vdpasim_net_work function handles both transmit TX
 completion and RX processing. A descriptor is taken from the TX VQ at the
 start of the loop, representing a buffer previously used for the TX path's
 initial read from the virtual networ...



> -----Original Message-----
> From: Jason Wang <jasowang@...hat.com>
> Sent: Wednesday, December 17, 2025 10:52 AM
> To: Angus Chen <angus.chen@...uarmicro.com>
> Cc: mst@...hat.com; xuanzhuo@...ux.alibaba.com; eperezma@...hat.com;
> linux-kernel@...r.kernel.org; virtualization@...ts.linux.dev
> Subject: Re: [PATCH] The vdpasim_net_work function handles both transmit TX
> completion and RX processing. A descriptor is taken from the TX VQ at the start
> of the loop, representing a buffer previously used for the TX path's initial read
> from the virtual networ...
> 
> On Wed, Dec 17, 2025 at 10:30 AM Angus Chen <angus.chen@...uarmicro.com>
> wrote:
> >
> 
> It looks like the title is too long.
> 
> > However, the error handling logic was flawed:
> >
> > 1. Read Failure (err <= 0): The TX VQ completion
> >    (vdpasim_net_complete(txq, 0)) was performed, but the unconditional
> >    break that followed prevented theprocessing of subsequent pending work
> >    items (other descriptors) in the queue in the same work function call.
> >
> > 2. Write Failure (write <= 0): If data could not be written to the RX VQ
> >    descriptor (e.g., vringh_iov_push_iotlb failed), the flow would 'break'
> >    before the TX VQ descriptor was completed. This led to a leak of TX VQ
> >    descriptors, as the work was never notified that the TX buffer was
> >    free to use again.
> >
> > This commit refactors the control flow to ensure:
> > a) The TX VQ descriptor is completed back to the rx in all error and
> >    success paths related to the current descriptor processing.
> > b) The unconditional break on read failure is removed, allowing the
> >    function to continue processing remaining work items
> >    until the loop naturally exits.
> >
> > Fixes: 0899774cb360 ("vdpa_sim_net: vendor satistics")
> 
> This is suspicious, the logic was there since introduced? Or we can
> split this patch
Yes，you are right, I just use the 0899774cb360 which be influenced by the error handler logic.
> 
> 1) fix the stats
> 2) fix the error handling
> 
> > ---
> >
> > Signed-off-by: Angus Chen <angus.chen@...uarmicro.com>
> > ---
> >  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 28 ++++++++++++++--------------
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > index 6caf09a1907b..298bd6e8e0a0 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > @@ -242,22 +242,22 @@ static void vdpasim_net_work(struct vdpasim
> *vdpasim)
> >                 if (err <= 0) {
> >                         ++rx_overruns;
> >                         vdpasim_net_complete(txq, 0);
> > -                       break;
> > -               }
> > -
> > -               write = vringh_iov_push_iotlb(&rxq->vring, &rxq->in_iov,
> > -                                             net->buffer, read);
> > -               if (write <= 0) {
> > -                       ++rx_errors;
> > -                       break;
> > +               } else {
> 
> This else seem useless?
> 
> > +
> > +                       write = vringh_iov_push_iotlb(&rxq->vring,
> &rxq->in_iov,
> > +
> net->buffer, read);
> > +                       if (write <= 0) {
> > +                               ++rx_errors;
> > +                               vdpasim_net_complete(txq, 0);
> 
> I'd move this after:
> 
>                 read = vringh_iov_pull_iotlb(&txq->vring, &txq->out_iov,
>                                              net->buffer,
> PAGE_SIZE);
> 
> So we would have a single vdpasim_net_complete()
Yes,I will send v2 to fix this.
> 
> > +                               break;
> > +                       }
> > +
> > +                       ++rx_pkts;
> > +                       rx_bytes += write;
> > +                       vdpasim_net_complete(txq, 0);
> > +                       vdpasim_net_complete(rxq, write);
> >                 }
> >
> > -               ++rx_pkts;
> > -               rx_bytes += write;
> > -
> > -               vdpasim_net_complete(txq, 0);
> > -               vdpasim_net_complete(rxq, write);
> > -
> >                 if (tx_pkts > 4) {
> >                         vdpasim_schedule_work(vdpasim);
> >                         goto out;
> > --
> > 2.34.1
> >
> 
> Thanks