netdev - Re: [PATCH v21 bpf-next 06/23] net: marvell: rely on xdp_update_skb_shared

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: <CAJ0CqmXaeJkJ8SDjTA1u_JNsqpxS8GA4J29S9wGPH0qOmqjp0w@mail.gmail.com>
Date:   Tue, 11 Jan 2022 14:05:23 +0100
From:   Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To:     Andy Gospodarek <andrew.gospodarek@...adcom.com>
Cc:     Lorenzo Bianconi <lorenzo@...nel.org>,
        BPF-dev-list <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "Agroskin, Shay" <shayagr@...zon.com>,
        John Fastabend <john.fastabend@...il.com>,
        David Ahern <dsahern@...nel.org>,
        Jesper Brouer <brouer@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Saeed Mahameed <saeed@...nel.org>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        "Sarkar, Tirthendu" <tirthendu.sarkar@...el.com>,
        Toke Hoiland Jorgensen <toke@...hat.com>, andy@...yhouse.net
Subject: Re: [PATCH v21 bpf-next 06/23] net: marvell: rely on
 xdp_update_skb_shared_info utility routine

>
> On Sat, Jan 08, 2022 at 12:53:09PM +0100, Lorenzo Bianconi wrote:
> > Rely on xdp_update_skb_shared_info routine in order to avoid
> > resetting frags array in skb_shared_info structure building
> > the skb in mvneta_swbm_build_skb(). Frags array is expected to
> > be initialized by the receiving driver building the xdp_buff
> > and here we just need to update memory metadata.
> >
> > Acked-by: Toke Hoiland-Jorgensen <toke@...hat.com>
> > Acked-by: John Fastabend <john.fastabend@...il.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> > ---
> >  drivers/net/ethernet/marvell/mvneta.c | 23 ++++++++++-------------
> >  1 file changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> > index 775ffd91b741..267a306d9c75 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -2332,8 +2332,12 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,
> >               skb_frag_size_set(frag, data_len);
> >               __skb_frag_set_page(frag, page);
> >
> > -             if (!xdp_buff_is_mb(xdp))
> > +             if (!xdp_buff_is_mb(xdp)) {
> > +                     sinfo->xdp_frags_size = *size;
> >                       xdp_buff_set_mb(xdp);
> > +             }
> > +             if (page_is_pfmemalloc(page))
> > +                     xdp_buff_set_frag_pfmemalloc(xdp);
> >       } else {
> >               page_pool_put_full_page(rxq->page_pool, page, true);
> >       }
> > @@ -2347,7 +2351,6 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
> >       struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> >       struct sk_buff *skb;
> >       u8 num_frags;
> > -     int i;
> >
> >       if (unlikely(xdp_buff_is_mb(xdp)))
> >               num_frags = sinfo->nr_frags;
> > @@ -2362,18 +2365,12 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
> >       skb_put(skb, xdp->data_end - xdp->data);
> >       skb->ip_summed = mvneta_rx_csum(pp, desc_status);
> >
> > -     if (likely(!xdp_buff_is_mb(xdp)))
> > -             goto out;
> > -
> > -     for (i = 0; i < num_frags; i++) {
> > -             skb_frag_t *frag = &sinfo->frags[i];
> > -
> > -             skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> > -                             skb_frag_page(frag), skb_frag_off(frag),
> > -                             skb_frag_size(frag), PAGE_SIZE);
>
> Maybe I'm missing something but I'm not sure you have a suitable
> replacement for the 3 lines above this in your proposed change.
>

Hi Andy,

mvneta_swbm_add_rx_fragment() initializes frags array in
skb_shared_info for xdp whenever we receive a multi-descriptors frame.
Since frags array is at the same offset for the xdp_buff and for the
new skb and build_skb() in mvneta_swbm_build_skb() does not overwrite
it, we do not need to initialize it again allocating the skb, just
account metadata info running xdp_update_skb_shared_info(). Agree?

> > -     }
> > +     if (unlikely(xdp_buff_is_mb(xdp)))
> > +             xdp_update_skb_shared_info(skb, num_frags,
> > +                                        sinfo->xdp_frags_size,
> > +                                        num_frags * xdp->frame_sz,
> > +                                        xdp_buff_is_frag_pfmemalloc(xdp));
> >
>
> When I did an implementation of this on a different driver I also needed
> to add:
>
>         for (i = 0; i < num_frags; i++)
>                 skb_frag_set_page(skb, i, skb_frag_page(&sinfo->frags[i]));
>
> to make sure that frames that were given XDP_PASS were formatted
> correctly so they could be handled by the stack.  Don't you need
> something similar to make sure frags are properly set?
>
> Thanks,
>
> -andy
>
> P.S.  Sorry for noticing this so late in the process; I realize this version
> was just a rebase of v20 and this would have been useful information
> earlier if I'm correct.
>

no worries :)

Regards,
Lorenzo

> > -out:
> >       return skb;
> >  }
> >
> > --
> > 2.33.1
> >