Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO

Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO

[Willem de Bruijn]

On Wed, Apr 20, 2022 at 4:28 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> Currently, the kernel drops GSO VLAN tagged packet if it's created with
> socket(AF_PACKET, SOCK_RAW, 0) plus virtio_net_hdr.
>
> The reason is AF_PACKET doesn't adjust the skb network header if there is
> a VLAN tag. Then after virtio_net_hdr_set_proto() called, the skb->protocol
> will be set to ETH_P_IP/IPv6. And in later inet/ipv6_gso_segment() the skb
> is dropped as network header position is invalid.
>
> Let's handle VLAN packets by adjusting network header position in
> packet_parse_headers(), and move the function in packet_snd() before
> calling virtio_net_hdr_set_proto().

The network header is set in

        skb_reset_network_header(skb);

        err = -EINVAL;
        if (sock->type == SOCK_DGRAM) {
                offset = dev_hard_header(skb, dev, ntohs(proto), addr,
NULL, len);
                if (unlikely(offset < 0))
                        goto out_free;
        } else if (reserve) {
                skb_reserve(skb, -reserve);
                if (len < reserve + sizeof(struct ipv6hdr) &&
                    dev->min_header_len != dev->hard_header_len)
                        skb_reset_network_header(skb);
        }

If all that is needed is to move the network header beyond an optional
VLAN tag in the case of SOCK_RAW, then this can be done in the else
for Ethernet packets.

It is not safe to increase reserve, as that would eat into the
reserved hlen LL_RESERVED_SPACE(dev), which does not account for
optional VLAN headers.

Instead of setting here first, then patching up again later in
packet_parse_headers.

This change affects all packets with VLAN headers, not just those with
GSO. I imagine that moving the network header is safe for all, but
don't know that code well enough to verify that it does not have
unintended side effects. Does dev_queue_xmit expect the network header
to point to the start of the VLAN headers or after, for instance?

> No need to update tpacket_snd() as it calls packet_parse_headers() in
> tpacket_fill_skb(), which is already before calling virtio_net_hdr_*
> functions.
>
> skb->no_fcs setting is also moved upper to make all skb settings together
> and keep consistence with function packet_sendmsg_spkt().
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/packet/af_packet.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 002d2b9c69dd..cfdbda28ef82 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1924,12 +1924,20 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,
>
>  static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
>  {
> +       int depth;
> +
>         if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) &&
>             sock->type == SOCK_RAW) {
>                 skb_reset_mac_header(skb);
>                 skb->protocol = dev_parse_header_protocol(skb);
>         }
>
> +       /* Move network header to the right position for VLAN tagged packets */
> +       if (likely(skb->dev->type == ARPHRD_ETHER) &&
> +           eth_type_vlan(skb->protocol) &&
> +           __vlan_get_protocol(skb, skb->protocol, &depth) != 0)
> +               skb_set_network_header(skb, depth);
> +
>         skb_probe_transport_header(skb);
>  }
>
> @@ -3047,6 +3055,11 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>         skb->mark = sockc.mark;
>         skb->tstamp = sockc.transmit_time;
>
> +       if (unlikely(extra_len == 4))
> +               skb->no_fcs = 1;
> +
> +       packet_parse_headers(skb, sock);
> +
>         if (has_vnet_hdr) {
>                 err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
>                 if (err)
> @@ -3055,11 +3068,6 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>                 virtio_net_hdr_set_proto(skb, &vnet_hdr);
>         }
>
> -       packet_parse_headers(skb, sock);
> -
> -       if (unlikely(extra_len == 4))
> -               skb->no_fcs = 1;
> -
>         err = po->xmit(skb);
>         if (unlikely(err != 0)) {
>                 if (err > 0)
> --
> 2.35.1
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO

[Willem de Bruijn]

On Wed, Apr 20, 2022 at 10:32 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Wed, Apr 20, 2022 at 09:45:15AM -0400, Willem de Bruijn wrote:
> > On Wed, Apr 20, 2022 at 4:28 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
> > >
> > > Currently, the kernel drops GSO VLAN tagged packet if it's created with
> > > socket(AF_PACKET, SOCK_RAW, 0) plus virtio_net_hdr.
> > >
> > > The reason is AF_PACKET doesn't adjust the skb network header if there is
> > > a VLAN tag. Then after virtio_net_hdr_set_proto() called, the skb->protocol
> > > will be set to ETH_P_IP/IPv6. And in later inet/ipv6_gso_segment() the skb
> > > is dropped as network header position is invalid.
> > >
> > > Let's handle VLAN packets by adjusting network header position in
> > > packet_parse_headers(), and move the function in packet_snd() before
> > > calling virtio_net_hdr_set_proto().
> >
> > The network header is set in
> >
> >         skb_reset_network_header(skb);
> >
> >         err = -EINVAL;
> >         if (sock->type == SOCK_DGRAM) {
> >                 offset = dev_hard_header(skb, dev, ntohs(proto), addr,
> > NULL, len);
> >                 if (unlikely(offset < 0))
> >                         goto out_free;
> >         } else if (reserve) {
> >                 skb_reserve(skb, -reserve);
> >                 if (len < reserve + sizeof(struct ipv6hdr) &&
> >                     dev->min_header_len != dev->hard_header_len)
> >                         skb_reset_network_header(skb);
> >         }
> >
> > If all that is needed is to move the network header beyond an optional
> > VLAN tag in the case of SOCK_RAW, then this can be done in the else
> > for Ethernet packets.
>
> Before we set network position, we need to check the skb->protocol to make
> sure it's a VLAN packet.
>
> If we set skb->protocol and adjust network header here, like
> packet_parse_headers() does. How should we do with later
>
>         skb->protocol = proto;
>         skb->dev = dev;
>
> settings?
>
> If you are afraid that skb_probe_transport_header(skb) would affect the
> virtio_net_hdr operation. How about split the skb_probe_transport_header()
> from packet_parse_headers()? Something like
>
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1924,13 +1924,19 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,
>
>  static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
>  {
> +       int depth;
> +
>         if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) &&
>             sock->type == SOCK_RAW) {
>                 skb_reset_mac_header(skb);
>                 skb->protocol = dev_parse_header_protocol(skb);
>         }
>
> -       skb_probe_transport_header(skb);
> +       /* Move network header to the right position for VLAN tagged packets */
> +       if (likely(skb->dev->type == ARPHRD_ETHER) &&
> +           eth_type_vlan(skb->protocol) &&
> +           __vlan_get_protocol(skb, skb->protocol, &depth) != 0)
> +               skb_set_network_header(skb, depth);
>  }
>
>  /*
> @@ -3047,6 +3055,8 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>         skb->mark = sockc.mark;
>         skb->tstamp = sockc.transmit_time;
>
> +       packet_parse_headers(skb, sock);
> +
>         if (has_vnet_hdr) {
>                 err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
>                 if (err)
> @@ -3055,7 +3065,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>                 virtio_net_hdr_set_proto(skb, &vnet_hdr);
>         }
>
> -       packet_parse_headers(skb, sock);
> +       skb_probe_transport_header(skb);
>
>         if (unlikely(extra_len == 4))
>                 skb->no_fcs = 1;
>
>
> >
> > It is not safe to increase reserve, as that would eat into the
> > reserved hlen LL_RESERVED_SPACE(dev), which does not account for
> > optional VLAN headers.
> >
> > Instead of setting here first, then patching up again later in
> > packet_parse_headers.
> >
> > This change affects all packets with VLAN headers, not just those with
> > GSO. I imagine that moving the network header is safe for all, but
> > don't know that code well enough to verify that it does not have
> > unintended side effects. Does dev_queue_xmit expect the network header
> > to point to the start of the VLAN headers or after, for instance?
>
> I think adjust the network position should be safe, as tap device also did that.

Oh, it's great to be able to point to such prior art. Can you mention
that in the commit message?

Your approach does sound simpler than the above. Thanks for looking
into that alternative, though.

>
> Thanks
> Hangbin
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO

[Willem de Bruijn]

On Thu, Apr 21, 2022 at 10:10 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> Hi Willem,
> On Thu, Apr 21, 2022 at 10:15:16AM -0400, Willem de Bruijn wrote:
> > Your approach does sound simpler than the above. Thanks for looking
> > into that alternative, though.
>
> Sorry I have to bring this topic back. I just remembered that
> tpacket_snd() already called skb_probe_transport_header() before calling
> virtio_net_hdr_* functions. e.g.
>
> - tpacket_snd()
>   - tpacket_fill_skb()
>     - packet_parse_headers()
>       - skb_probe_transport_header()
>   - if (po->has_vnet_hdr)
>     - virtio_net_hdr_to_skb()
>     - virtio_net_hdr_set_proto()
>
> While for packet_snd()
>
> - packet_snd()
>   - if (has_vnet_hdr)
>     - virtio_net_hdr_to_skb()
>     - virtio_net_hdr_set_proto()
>   - packet_parse_headers()
>     - skb_probe_transport_header()
>
> If we split skb_probe_transport_header() from packet_parse_headers() and
> move it before calling virtio_net_hdr_* function in packet_snd(). Should
> we do the same for tpacket_snd(), i.e. move skb_probe_transport_header()
> after the virtio_net_hdr_* function?

That sounds like the inverse: "move after" instead of "move before"?

But I thought the plan was to go back to your last patch which brings
packet_snd in line with tpacket_snd by moving packet_parse_headers in
its entirety before virtio_net_hdr_*?

> I think it really doesn't matter whether calls skb_probe_transport_header()
> before or after virtio_net_hdr_* functions if we could set the skb->protocol
> and network header correctly. Because
>
> - skb_probe_transport_header()
>   - skb_flow_dissect_flow_keys_basic()
>     - __skb_flow_dissect()
>
> In __skb_flow_dissect()
> ```
>  * @data: raw buffer pointer to the packet, if NULL use skb->data
>  * @proto: protocol for which to get the flow, if @data is NULL use skb->protocol
>  * @nhoff: network header offset, if @data is NULL use skb_network_offset(skb)
>  * @hlen: packet header length, if @data is NULL use skb_headlen(skb)
> ```
>
> So when data is NULL, we need to make sure the protocol, network header offset,
> packet header length are correct.
>
> Before this patch, the VLAN packet network header offset is incorrect when calls
> skb_probe_transport_header(). After the fix, this issue should be gone
> and we can call skb_probe_transport_header() safely.
>
> So my conclusion is. There is no need to split packet_parse_headers(). Move
> packet_parse_headers() before calling virtio_net_hdr_* function in packet_snd()
> should be safe.

Ack. Sorry if my last response was not entirely clear on this point.

> Please pardon me if I didn't make something clear.
> Let's me know what do you think.
>
> Thanks
> Hangbin
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO

[Willem de Bruijn]

On Sat, Apr 23, 2022 at 10:31 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Fri, Apr 22, 2022 at 05:39:48PM -0400, Willem de Bruijn wrote:
> > > If we split skb_probe_transport_header() from packet_parse_headers() and
> > > move it before calling virtio_net_hdr_* function in packet_snd(). Should
> > > we do the same for tpacket_snd(), i.e. move skb_probe_transport_header()
> > > after the virtio_net_hdr_* function?
> >
> > That sounds like the inverse: "move after" instead of "move before"?
>
> That's for "split packet_parse_headers()" option.
>
> >
> > But I thought the plan was to go back to your last patch which brings
> > packet_snd in line with tpacket_snd by moving packet_parse_headers in
> > its entirety before virtio_net_hdr_*?
>
> Yes, exactly.
>
> > > So my conclusion is. There is no need to split packet_parse_headers(). Move
> > > packet_parse_headers() before calling virtio_net_hdr_* function in packet_snd()
> > > should be safe.
> >
> > Ack. Sorry if my last response was not entirely clear on this point.
>
> Thanks a lot for your review. Do you think if I need to re-post the patch?
> Or will you give an Acked-by for this one?

Please resubmit. And then please also add the comment about tap having
the same path.

>
> Hangbin
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization