netdev - Re: [PATCH net-next] packet: Account for VLAN_HLEN in csum_start when virtio_net

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: <CAHcdBH7h-sq=Gzkan1du3uxx44WibK0yzdnUcZCuw-mp=9OxOg@mail.gmail.com>
Date: Thu, 23 Nov 2023 17:53:21 -0500
From: Mike Pattrick <mkp@...hat.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] packet: Account for VLAN_HLEN in csum_start when
 virtio_net_hdr is enabled

On Thu, Nov 23, 2023 at 4:25 PM Willem de Bruijn
<willemdebruijn.kernel@...il.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@...hat.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 *skb,
> > -                        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 = { .num_buffers = 0 };
> > +     int vlan_hlen;
> >
> >       if (*len < vnet_hdr_sz)
> >               return -EINVAL;
> >       *len -= vnet_hdr_sz;
> >
> > -     if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)&vnet_hdr, vio_le(), true, 0))
> > +     if (sk->sk_type == SOCK_RAW && skb_vlan_tag_present(skb))
> > +             vlan_hlen = VLAN_HLEN;
> > +     else
> > +             vlan_hlen = 0;
> > +
> > +     if (virtio_net_hdr_from_skb(skb, (struct virtio_net_hdr *)&vnet_hdr,
> > +                                 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, struct 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 == 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 == SOCK_RAW && skb_vlan_tag_present(skb))
> > +                     vlan_hlen = VLAN_HLEN;
> > +             else
> > +                     vlan_hlen = 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 == TPACKET_V3)
> > +                             prb_clear_blk_fill_status(&po->rx_ring);
> > +                     goto drop_n_account;
> > +             }
> >       }
> >
> >       if (po->tp_version <= TPACKET_V2) {
> > @@ -3464,7 +3480,7 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> >       packet_rcv_try_clear_pressure(pkt_sk(sk));
> >
> >       if (vnet_hdr_len) {
> > -             err = packet_rcv_vnet(msg, skb, &len, vnet_hdr_len);
> > +             err = packet_rcv_vnet(msg, skb, &len, vnet_hdr_len, sk);
> >               if (err)
> >                       goto out_free;
> >       }
> > --
> > 2.40.1
> >
>
>