Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200228105435.75298-1-lrizzo@google.com> <20200228132941.2c8b8d01@carbon>
In-Reply-To: <20200228132941.2c8b8d01@carbon>
From:   Luigi Rizzo <lrizzo@google.com>
Date:   Fri, 28 Feb 2020 05:19:54 -0800
Message-ID: <CAMOZA0KnC0n+U+n3vjx4bb6o_1_MtzLxUfoGj22NdNVQO33uAA@mail.gmail.com>
Subject: Re: [PATCH v4] netdev attribute to control xdpgeneric skb linearization
To:     Jesper Dangaard Brouer <jbrouer@redhat.com>
Cc:     netdev@vger.kernel.org,
        =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= <toke@redhat.com>,
        David Miller <davem@davemloft.net>,
        Jesper Dangaard Brouer <hawk@kernel.org>, sameehj@amazon.com,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Feb 28, 2020 at 4:30 AM Jesper Dangaard Brouer
<jbrouer@redhat.com> wrote:
>
> On Fri, 28 Feb 2020 02:54:35 -0800
> Luigi Rizzo <lrizzo@google.com> wrote:
>
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index dbbfff123196..c539489d3166 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -4520,9 +4520,12 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
> >       /* 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.
> > +      * For non shared skbs, xdpgeneric_linearize controls linearization.
> >        */
> > -     if (skb_cloned(skb) || skb_is_nonlinear(skb) ||
> > -         skb_headroom(skb) < XDP_PACKET_HEADROOM) {
> > +     if (skb_cloned(skb) ||
> > +         (skb->dev->xdpgeneric_linearize &&
> > +          (skb_is_nonlinear(skb) ||
> > +           skb_headroom(skb) < XDP_PACKET_HEADROOM))) {
> >               int hroom = XDP_PACKET_HEADROOM - skb_headroom(skb);
> >               int troom = skb->tail + skb->data_len - skb->end;
> >
>
> Have you checked that calling bpf_xdp_adjust_tail() is not breaking anything?

It won't leak memory or cause crashes if that is what you mean.
Of course if there are more segments the effect won't be the desired one,
as it will chop off the tail of the first segment.

But this is an opt-in feature and requires the same permissions needed to load
an xdp program, so I expect it to be used consciously.

It would be nice if we had a flag in the xdp_buff to communicate that
the packet is
incomplete, but there isn't a way that I can see.

cheers
luigi