Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <20231123183835.635210-1-mkp@redhat.com> <655fc32bb506e_d14d4294b3@willemb.c.googlers.com.notmuch>
In-Reply-To: <655fc32bb506e_d14d4294b3@willemb.c.googlers.com.notmuch>
From:   Mike Pattrick <mkp@redhat.com>
Date:   Thu, 23 Nov 2023 17:53:21 -0500
Message-ID: <CAHcdBH7h-sq=Gzkan1du3uxx44WibK0yzdnUcZCuw-mp=9OxOg@mail.gmail.com>
Subject: Re: [PATCH net-next] packet: Account for VLAN_HLEN in csum_start when
 virtio_net_hdr is enabled
To:     Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc:     netdev@vger.kernel.org, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Nov 23, 2023 at 4:25=E2=80=AFPM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Mike Pattrick wrote:
> > Af_packet provides checksum offload offsets to usermode applications
> > through struct virtio_net_hdr when PACKET_VNET_HDR is enabled on the
> > socket. For skbuffs with a vlan being sent to a SOCK_RAW socket,
> > af_packet will include the link level header and so csum_start needs
> > to be adjusted accordingly.
>
> Is this patch based on observing an incorrect offset in a workload,
> or on code inspection?

Based on an incorrect offset in a workload. The setup involved sending
vxlan traffic though a veth interface configured with a vlan. The
vnet_hdr's csum_start value was off by 4, and this problem went away
when the vlan was removed.

I'll take another look at this patch.

>
> As the referenced patch mentions, VLAN_HLEN adjustment is needed
> in macvtap because it pulls the vlan header from skb->vlan_tci. At
> which point skb->csum_start is wrong.
>
> "Commit f09e2249c4f5 ("macvtap: restore vlan header on user read")
>  added this feature to macvtap. Commit 3ce9b20f1971 ("macvtap: Fix
>  csum_start when VLAN tags are present") then fixed up csum_start."
>
> But the commit also mentions "Virtio, packet and uml do not insert
> the vlan header in the user buffer.". This situation has not changed.
>
> Packet sockets may receive packets with VLAN headers present, but
> unless they were inserted manually before passing to user, as macvtap
> does, this does not affect csum_start.
>
> Packet sockets support reading those skb->vlan_tci stored VLAN
> headers using AUXDATA.
>
> > Fixes: fd3a88625844 ("net: in virtio_net_hdr only add VLAN_HLEN to csum=
_start if payload holds vlan")
>
> The fix should target net, not net-next.
>
> > Signed-off-by: Mike Pattrick <mkp@redhat.com>
> > ---
> >  net/packet/af_packet.c | 36 ++++++++++++++++++++++++++----------
> >  1 file changed, 26 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index a84e00b5904b..f6b602ffe383 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -2092,15 +2092,23 @@ static unsigned int run_filter(struct sk_buff *=
skb,
> >  }
> >
> >  static int packet_rcv_vnet(struct msghdr *msg, const struct sk_buff *s=
kb,
> > -                        size_t *len, int vnet_hdr_sz)
> > +                        size_t *len, int vnet_hdr_sz,
> > +                        const struct sock *sk)
> >  {
> >       struct virtio_net_hdr_mrg_rxbuf vnet_hdr =3D { .num_buffers =3D 0=
 };
> > +     int vlan_hlen;
> >
> >       if (*len < vnet_hdr_sz)
> >               return -EINVAL;
> >       *len -=3D vnet_hdr_sz;
> >
> > -     if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)&vnet_h=
dr, vio_le(), true, 0))
> > +     if (sk->sk_type =3D=3D SOCK_RAW && skb_vlan_tag_present(skb))
> > +             vlan_hlen =3D VLAN_HLEN;
> > +     else
> > +             vlan_hlen =3D 0;
> > +
> > +     if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)&vnet_h=
dr,
> > +                                 vio_le(), true, vlan_hlen))
> >               return -EINVAL;
> >
> >       return memcpy_to_msg(msg, (void *)&vnet_hdr, vnet_hdr_sz);
> > @@ -2368,13 +2376,21 @@ static int tpacket_rcv(struct sk_buff *skb, str=
uct net_device *dev,
> >               __set_bit(slot_id, po->rx_ring.rx_owner_map);
> >       }
> >
> > -     if (vnet_hdr_sz &&
> > -         virtio_net_hdr_from_skb(skb, h.raw + macoff -
> > -                                 sizeof(struct virtio_net_hdr),
> > -                                 vio_le(), true, 0)) {
> > -             if (po->tp_version =3D=3D TPACKET_V3)
> > -                     prb_clear_blk_fill_status(&po->rx_ring);
> > -             goto drop_n_account;
> > +     if (vnet_hdr_sz) {
> > +             int vlan_hlen;
> > +
> > +             if (sk->sk_type =3D=3D SOCK_RAW && skb_vlan_tag_present(s=
kb))
> > +                     vlan_hlen =3D VLAN_HLEN;
> > +             else
> > +                     vlan_hlen =3D 0;
> > +
> > +             if (virtio_net_hdr_from_skb(skb, h.raw + macoff -
> > +                                         sizeof(struct virtio_net_hdr)=
,
> > +                                         vio_le(), true, vlan_hlen)) {
> > +                     if (po->tp_version =3D=3D TPACKET_V3)
> > +                             prb_clear_blk_fill_status(&po->rx_ring);
> > +                     goto drop_n_account;
> > +             }
> >       }
> >
> >       if (po->tp_version <=3D TPACKET_V2) {
> > @@ -3464,7 +3480,7 @@ static int packet_recvmsg(struct socket *sock, st=
ruct msghdr *msg, size_t len,
> >       packet_rcv_try_clear_pressure(pkt_sk(sk));
> >
> >       if (vnet_hdr_len) {
> > -             err =3D packet_rcv_vnet(msg, skb, &len, vnet_hdr_len);
> > +             err =3D packet_rcv_vnet(msg, skb, &len, vnet_hdr_len, sk)=
;
> >               if (err)
> >                       goto out_free;
> >       }
> > --
> > 2.40.1
> >
>
>