| 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
| ||
|
Message-ID: <aNQBuQYHGHuekAhV@boxer>
Date: Wed, 24 Sep 2025 16:35:37 +0200
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Jason Xing <kerneljasonxing@...il.com>
CC: Stanislav Fomichev <stfomichev@...il.com>, <bpf@...r.kernel.org>,
<ast@...nel.org>, <daniel@...earbox.net>, <andrii@...nel.org>,
<netdev@...r.kernel.org>, <magnus.karlsson@...el.com>
Subject: Re: [PATCH bpf-next 2/3] xsk: remove @first_frag from xsk_build_skb()
On Tue, Sep 23, 2025 at 05:25:01PM +0800, Jason Xing wrote:
> On Tue, Sep 23, 2025 at 1:48 AM Stanislav Fomichev <stfomichev@...il.com> wrote:
> >
> > On 09/22, Maciej Fijalkowski wrote:
> > > Devices that set IFF_TX_SKB_NO_LINEAR will not execute branch that
> > > handles metadata, as we set @first_frag only for !IFF_TX_SKB_NO_LINEAR
> > > code in xsk_build_skb().
> > >
> > > Same functionality can be achieved with checking if xsk_get_num_desc()
> > > returns 0. To replace current usage of @first_frag with
> > > XSKCB(skb)->num_descs check, pull out the code from
> > > xsk_set_destructor_arg() that initializes sk_buff::cb and call it before
> > > skb_store_bits() in branch that creates skb against first processed
> > > frag. This so error path has the XSKCB(skb)->num_descs initialized and
> > > can free skb in case skb_store_bits() failed.
> > >
> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> > > ---
> > > net/xdp/xsk.c | 20 +++++++++++---------
> > > 1 file changed, 11 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > index 72194f0a3fc0..064238400036 100644
> > > --- a/net/xdp/xsk.c
> > > +++ b/net/xdp/xsk.c
> > > @@ -605,6 +605,13 @@ static u32 xsk_get_num_desc(struct sk_buff *skb)
> > > return XSKCB(skb)->num_descs;
> > > }
> > >
> > > +static void xsk_init_cb(struct sk_buff *skb)
> > > +{
> > > + BUILD_BUG_ON(sizeof(struct xsk_addr_head) > sizeof(skb->cb));
> > > + INIT_LIST_HEAD(&XSKCB(skb)->addrs_list);
> > > + XSKCB(skb)->num_descs = 0;
> > > +}
> > > +
> > > static void xsk_destruct_skb(struct sk_buff *skb)
> > > {
> > > struct xsk_tx_metadata_compl *compl = &skb_shinfo(skb)->xsk_meta;
> > > @@ -620,9 +627,6 @@ static void xsk_destruct_skb(struct sk_buff *skb)
> > >
> > > static void xsk_set_destructor_arg(struct sk_buff *skb, u64 addr)
> > > {
> > > - BUILD_BUG_ON(sizeof(struct xsk_addr_head) > sizeof(skb->cb));
> > > - INIT_LIST_HEAD(&XSKCB(skb)->addrs_list);
> > > - XSKCB(skb)->num_descs = 0;
> > > skb_shinfo(skb)->destructor_arg = (void *)(uintptr_t)addr;
> > > }
> > >
> > > @@ -672,7 +676,7 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
> > > return ERR_PTR(err);
> > >
> > > skb_reserve(skb, hr);
> > > -
> > > + xsk_init_cb(skb);
> > > xsk_set_destructor_arg(skb, desc->addr);
> > > } else {
> > > xsk_addr = kmem_cache_zalloc(xsk_tx_generic_cache, GFP_KERNEL);
> > > @@ -725,7 +729,6 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > > struct xsk_tx_metadata *meta = NULL;
> > > struct net_device *dev = xs->dev;
> > > struct sk_buff *skb = xs->skb;
> > > - bool first_frag = false;
> > > int err;
> > >
> > > if (dev->priv_flags & IFF_TX_SKB_NO_LINEAR) {
> > > @@ -742,8 +745,6 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > > len = desc->len;
> > >
> > > if (!skb) {
> > > - first_frag = true;
> > > -
> > > hr = max(NET_SKB_PAD, L1_CACHE_ALIGN(dev->needed_headroom));
> > > tr = dev->needed_tailroom;
> > > skb = sock_alloc_send_skb(&xs->sk, hr + len + tr, 1, &err);
> > > @@ -752,6 +753,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > >
> > > skb_reserve(skb, hr);
> > > skb_put(skb, len);
> > > + xsk_init_cb(skb);
> > >
> > > err = skb_store_bits(skb, 0, buffer, len);
> > > if (unlikely(err))
> > > @@ -797,7 +799,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > > list_add_tail(&xsk_addr->addr_node, &XSKCB(skb)->addrs_list);
> > > }
> > >
> > > - if (first_frag && desc->options & XDP_TX_METADATA) {
> > > + if (!xsk_get_num_desc(skb) && desc->options & XDP_TX_METADATA) {
> > > if (unlikely(xs->pool->tx_metadata_len == 0)) {
> > > err = -EINVAL;
> > > goto free_err;
> > > @@ -839,7 +841,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > > return skb;
> > >
> > > free_err:
> > > - if (first_frag && skb)
> >
> > [..]
> >
> > > + if (skb && !xsk_get_num_desc(skb))
> > > kfree_skb(skb);
> > >
> > > if (err == -EOVERFLOW) {
> >
> > For IFF_TX_SKB_NO_LINEAR case, the 'goto free_err' is super confusing.
> > xsk_build_skb_zerocopy either returns skb or an IS_ERR. Can we
> > add a separate label to jump directly to 'if err == -EOVERFLOW' for
> > the IFF_TX_SKB_NO_LINEAR case?
Right, I got hit with this when running xdpsock within VM now against
virtio-net driver. Since I removed @first_frag and sock_alloc_send_skb()
managed to give me -EAGAIN at start, skb was treated as valid pointer and
then I got splat when accessing either cb or skb_shared_info.
So either we NULL the skb for xsk_build_skb_zerocopy() error path (which
would be fine even for -EOVERFLOW as error path uses xs->skb pointer, not
the local one) or we introduce separate label as you suggest. No strong
opinions here.
> >
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 72e34bd2d925..f56182c61c99 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -732,7 +732,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > skb = xsk_build_skb_zerocopy(xs, desc);
> > if (IS_ERR(skb)) {
> > err = PTR_ERR(skb);
> > - goto free_err;
> > + goto out;
> > }
> > } else {
> > u32 hr, tr, len;
> > @@ -842,6 +842,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > if (first_frag && skb)
> > kfree_skb(skb);
> >
> > +out:
> > if (err == -EOVERFLOW) {
> > /* Drop the packet */
> > xsk_inc_num_desc(xs->skb);
> >
> > After that, it seems we can look at skb_shinfo(skb)->nr_frags? Instead
> > of adding new xsk_init_cb, seems more robust?
Thanks! I'll do it.
>
> +1. It would be simpler.
>
> And I think this patch should be a standalone one because it actually
> supports the missing feature for the VM scenario.
Hi Jason,
in commit message, I wrote about enabling tx metadata for
xsk_build_skb_zerocopy() but code did not reflect that as you point out in
your later reply.
Unless there are any objections I will actually enable it there.
>
> Thanks,
> Jason
Powered by blists - more mailing lists