On 11/30, Lorenzo Bianconi wrote:
> >
> >
> > On 11/30/23 10:11, Lorenzo Bianconi wrote:
> > > Similar to native xdp, do not always linearize the skb in
> > > netif_receive_generic_xdp routine but create a non-linear xdp_buff to be
> > > processed by the eBPF program. This allow to add multi-buffer support
> > > for xdp running in generic mode.
> > >
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> > > ---
> > > net/core/dev.c | 144 ++++++++++++++++++++++++++++++++++++++++---------
> > > 1 file changed, 119 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 4df68d7f04a2..0d08e755bb7f 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -4853,6 +4853,12 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
> > > xdp_init_buff(xdp, frame_sz, &rxqueue->xdp_rxq);
> > > xdp_prepare_buff(xdp, hard_start, skb_headroom(skb) - mac_len,
> > > skb_headlen(skb) + mac_len, true);
> > > + if (skb_is_nonlinear(skb)) {
> > > + skb_shinfo(skb)->xdp_frags_size = skb->data_len;
> > > + xdp_buff_set_frags_flag(xdp);
> > > + } else {
> > > + xdp_buff_clear_frags_flag(xdp);
> > > + }
> > > orig_data_end = xdp->data_end;
> > > orig_data = xdp->data;
> > > @@ -4882,6 +4888,14 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
> > > skb->len += off; /* positive on grow, negative on shrink */
> > > }
> > > + /* XDP frag metadata (e.g. nr_frags) are updated in eBPF helpers
> > > + * (e.g. bpf_xdp_adjust_tail), we need to update data_len here.
> > > + */
> > > + if (xdp_buff_has_frags(xdp))
> > > + skb->data_len = skb_shinfo(skb)->xdp_frags_size;
> > > + else
> > > + skb->data_len = 0;
> > > +
> > > /* check if XDP changed eth hdr such SKB needs update */
> > > eth = (struct ethhdr *)xdp->data;
> > > if ((orig_eth_type != eth->h_proto) ||
> > > @@ -4915,54 +4929,134 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
> > > return act;
> > > }
> > > -static u32 netif_receive_generic_xdp(struct sk_buff **pskb,
> > > - struct xdp_buff *xdp,
> > > - struct bpf_prog *xdp_prog)
> > > +static int netif_skb_check_for_generic_xdp(struct sk_buff **pskb,
> > > + struct bpf_prog *prog)
> >
> > I like this is split out into a check function.
> >
> > > {
> > > struct sk_buff *skb = *pskb;
> > > - u32 act = XDP_DROP;
> > > -
> > > - /* Reinjected packets coming from act_mirred or similar should
> > > - * not get XDP generic processing.
> > > - */
> > > - if (skb_is_redirected(skb))
> > > - return XDP_PASS;
> >
> > (For other reviewers)
> > This reinjected check is moved further down.
> >
> > > + int err;
> > > - /* XDP packets must be linear and must have sufficient headroom
> > > - * of XDP_PACKET_HEADROOM bytes. This is the guarantee that also
> > > - * native XDP provides, thus we need to do it here as well.
> > > + /* XDP does not support fraglist so we need to linearize
> > > + * the skb.
> > > */
> > > - if (skb_cloned(skb) || skb_is_nonlinear(skb) ||
> > > - skb_headroom(skb) < XDP_PACKET_HEADROOM) {
> > > + if (skb_has_frag_list(skb) || !prog->aux->xdp_has_frags) {
> > > int hroom = XDP_PACKET_HEADROOM - skb_headroom(skb);
> > > int troom = skb->tail + skb->data_len - skb->end;
> > > /* In case we have to go down the path and also linearize,
> > > * then lets do the pskb_expand_head() work just once here.
> > > */
> > > - if (pskb_expand_head(skb,
> > > - hroom > 0 ? ALIGN(hroom, NET_SKB_PAD) : 0,
> > > - troom > 0 ? troom + 128 : 0, GFP_ATOMIC))
> > > - goto do_drop;
> > > - if (skb_linearize(skb))
> > > - goto do_drop;
> > > + err = pskb_expand_head(skb,
> > > + hroom > 0 ? ALIGN(hroom, NET_SKB_PAD) : 0,
> > > + troom > 0 ? troom + 128 : 0, GFP_ATOMIC);
> > > + if (err)
> > > + return err;
> > > +
> > > + err = skb_linearize(skb);
> > > + if (err)
> > > + return err;
> > > +
> > > + return 0;
> > > + }
> > > +
> > > + /* XDP packets must have sufficient headroom of XDP_PACKET_HEADROOM
> > > + * bytes. This is the guarantee that also native XDP provides,
> > > + * thus we need to do it here as well.
> > > + */
> > > + if (skb_cloned(skb) || skb_shinfo(skb)->nr_frags ||
> >
> > I though we could allow a SKB with skb_shinfo(skb)->nr_frags (that isn't
> > cloned or shared) to be processed by generic XDP without any reallocation?
>
> I do not think so, we discussed about it with Jakub here [0]
>
> [0] https://lore.kernel.org/netdev/20231128105145.7b39db7d@xxxxxxxxxx/
Can this be done as an optimization later on? If, from the bpf side,
the verifier can attest that the program is not calling
bpf_xdp_{load,store}_bytes on the frags for example.
