netdev - Re: [PATCH net v4] xsk: avoid data corruption on cq descriptor number

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: <CAL+tcoC3ZkhV5d7rStShghVFdmGDx9pb13S4ZUqSo9KmrJesLg@mail.gmail.com>
Date: Thu, 20 Nov 2025 18:56:09 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Fernando Fernandez Mancera <fmancera@...e.de>
Cc: netdev@...r.kernel.org, csmate@....hu, maciej.fijalkowski@...el.com, 
	bpf@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, horms@...nel.org
Subject: Re: [PATCH net v4] xsk: avoid data corruption on cq descriptor number

On Thu, Nov 20, 2025 at 5:06 PM Fernando Fernandez Mancera
<fmancera@...e.de> wrote:
>
>
>
> On 11/20/25 4:07 AM, Jason Xing wrote:
> > On Tue, Nov 18, 2025 at 8:48 PM Fernando Fernandez Mancera
> > <fmancera@...e.de> wrote:
> [...]>> @@ -828,11 +840,20 @@ static struct sk_buff
> *xsk_build_skb(struct xdp_sock *xs,
> >>                                  goto free_err;
> >>                          }
> >>
> >> -                       xsk_addr = kmem_cache_zalloc(xsk_tx_generic_cache, GFP_KERNEL);
> >> -                       if (!xsk_addr) {
> >> -                               __free_page(page);
> >> -                               err = -ENOMEM;
> >> -                               goto free_err;
> >> +                       if (xsk_skb_destructor_is_addr(skb)) {
> >> +                               xsk_addr = kmem_cache_zalloc(xsk_tx_generic_cache,
> >> +                                                            GFP_KERNEL);
> >> +                               if (!xsk_addr) {
> >> +                                       __free_page(page);
> >> +                                       err = -ENOMEM;
> >> +                                       goto free_err;
> >> +                               }
> >> +
> >> +                               xsk_addr->num_descs = 1;
> >> +                               xsk_addr->addrs[0] = xsk_skb_destructor_get_addr(skb);
> >> +                               skb_shinfo(skb)->destructor_arg = (void *)xsk_addr;
> >> +                       } else {
> >> +                               xsk_addr = (struct xsk_addrs *)skb_shinfo(skb)->destructor_arg;
> >>                          }
> >>
> >>                          vaddr = kmap_local_page(page);
> >> @@ -842,13 +863,11 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> >>                          skb_add_rx_frag(skb, nr_frags, page, 0, len, PAGE_SIZE);
> >>                          refcount_add(PAGE_SIZE, &xs->sk.sk_wmem_alloc);
> >>
> >> -                       xsk_addr->addr = desc->addr;
> >> -                       list_add_tail(&xsk_addr->addr_node, &XSKCB(skb)->addrs_list);
> >> +                       xsk_addr->addrs[xsk_addr->num_descs] = desc->addr;
> >> +                       xsk_addr->num_descs++;
> >
> > Wait, it's too late to increment it... Please find below.
> >
> >>                  }
> >>          }
> >>
> >> -       xsk_inc_num_desc(skb);
> >> -
>
>
>
> >>          return skb;
> >>
> >>   free_err:
> >> @@ -857,7 +876,6 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> >>
> >>          if (err == -EOVERFLOW) {
> >>                  /* Drop the packet */
> >> -               xsk_inc_num_desc(xs->skb);
> >
> > Why did you remove this line? The error can occur in the above hidden
> > snippet[1] without IFF_TX_SKB_NO_LINEAR setting and then we will fail
> > to increment it by one.
> >
> >
> That is a good catch. Let me fix this logic.. I missed that the
> -EOVERFLOW is returned in different moments for xsk_build_skb_zerocopy()
> and xsk_build_skb(). Keeping the increment logic as it was it is better.

Right. Thanks!

My new solution based on net-next with your patch is ready now :)

Thanks,
Jason