* [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO
@ 2022-04-20 8:27 Hangbin Liu
2022-04-20 13:45 ` Willem de Bruijn
0 siblings, 1 reply; 12+ messages in thread
From: Hangbin Liu @ 2022-04-20 8:27 UTC (permalink / raw)
To: netdev
Cc: Michael S . Tsirkin, Jason Wang, David S . Miller,
Jakub Kicinski, Paolo Abeni, Maxim Mikityanskiy,
Willem de Bruijn, virtualization, Balazs Nemeth, Mike Pattrick,
Eric Dumazet, Hangbin Liu
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().
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO
2022-04-20 8:27 [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO Hangbin Liu
@ 2022-04-20 13:45 ` Willem de Bruijn
0 siblings, 0 replies; 12+ messages in thread
From: Willem de Bruijn @ 2022-04-20 13:45 UTC (permalink / raw)
To: Hangbin Liu
Cc: netdev, Michael S . Tsirkin, Jason Wang, David S . Miller,
Jakub Kicinski, Paolo Abeni, Maxim Mikityanskiy, virtualization,
Balazs Nemeth, Mike Pattrick, Eric Dumazet
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO
@ 2022-04-20 13:45 ` Willem de Bruijn
0 siblings, 0 replies; 12+ messages in thread
From: Willem de Bruijn @ 2022-04-20 13:45 UTC (permalink / raw)
To: Hangbin Liu
Cc: Maxim Mikityanskiy, Michael S . Tsirkin, netdev, Eric Dumazet,
virtualization, Balazs Nemeth, Jakub Kicinski, Paolo Abeni,
Mike Pattrick, David S . Miller
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO
2022-04-20 13:45 ` Willem de Bruijn
(?)
@ 2022-04-21 2:31 ` Hangbin Liu
2022-04-21 14:15 ` Willem de Bruijn
-1 siblings, 1 reply; 12+ messages in thread
From: Hangbin Liu @ 2022-04-21 2:31 UTC (permalink / raw)
To: Willem de Bruijn
Cc: netdev, Michael S . Tsirkin, Jason Wang, David S . Miller,
Jakub Kicinski, Paolo Abeni, Maxim Mikityanskiy, virtualization,
Balazs Nemeth, Mike Pattrick, Eric Dumazet
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.
Thanks
Hangbin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO
2022-04-21 2:31 ` Hangbin Liu
@ 2022-04-21 14:15 ` Willem de Bruijn
0 siblings, 0 replies; 12+ messages in thread
From: Willem de Bruijn @ 2022-04-21 14:15 UTC (permalink / raw)
To: Hangbin Liu
Cc: Willem de Bruijn, netdev, Michael S . Tsirkin, Jason Wang,
David S . Miller, Jakub Kicinski, Paolo Abeni,
Maxim Mikityanskiy, virtualization, Balazs Nemeth, Mike Pattrick,
Eric Dumazet
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO
@ 2022-04-21 14:15 ` Willem de Bruijn
0 siblings, 0 replies; 12+ messages in thread
From: Willem de Bruijn @ 2022-04-21 14:15 UTC (permalink / raw)
To: Hangbin Liu
Cc: Maxim Mikityanskiy, Willem de Bruijn, Michael S . Tsirkin,
netdev, Eric Dumazet, virtualization, Balazs Nemeth,
Jakub Kicinski, Paolo Abeni, Mike Pattrick, David S . Miller
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO
2022-04-21 14:15 ` Willem de Bruijn
(?)
@ 2022-04-22 2:08 ` Hangbin Liu
2022-04-22 21:39 ` Willem de Bruijn
-1 siblings, 1 reply; 12+ messages in thread
From: Hangbin Liu @ 2022-04-22 2:08 UTC (permalink / raw)
To: Willem de Bruijn
Cc: netdev, Michael S . Tsirkin, Jason Wang, David S . Miller,
Jakub Kicinski, Paolo Abeni, Maxim Mikityanskiy, virtualization,
Balazs Nemeth, Mike Pattrick, Eric Dumazet
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?
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.
Please pardon me if I didn't make something clear.
Let's me know what do you think.
Thanks
Hangbin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO
2022-04-22 2:08 ` Hangbin Liu
@ 2022-04-22 21:39 ` Willem de Bruijn
0 siblings, 0 replies; 12+ messages in thread
From: Willem de Bruijn @ 2022-04-22 21:39 UTC (permalink / raw)
To: Hangbin Liu
Cc: Maxim Mikityanskiy, Willem de Bruijn, Michael S . Tsirkin,
netdev, Eric Dumazet, virtualization, Balazs Nemeth,
Jakub Kicinski, Paolo Abeni, Mike Pattrick, David S . Miller
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO
@ 2022-04-22 21:39 ` Willem de Bruijn
0 siblings, 0 replies; 12+ messages in thread
From: Willem de Bruijn @ 2022-04-22 21:39 UTC (permalink / raw)
To: Hangbin Liu
Cc: Willem de Bruijn, netdev, Michael S . Tsirkin, Jason Wang,
David S . Miller, Jakub Kicinski, Paolo Abeni,
Maxim Mikityanskiy, virtualization, Balazs Nemeth, Mike Pattrick,
Eric Dumazet
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO
2022-04-22 21:39 ` Willem de Bruijn
(?)
@ 2022-04-24 2:29 ` Hangbin Liu
2022-04-24 13:46 ` Willem de Bruijn
-1 siblings, 1 reply; 12+ messages in thread
From: Hangbin Liu @ 2022-04-24 2:29 UTC (permalink / raw)
To: Willem de Bruijn
Cc: netdev, Michael S . Tsirkin, Jason Wang, David S . Miller,
Jakub Kicinski, Paolo Abeni, Maxim Mikityanskiy, virtualization,
Balazs Nemeth, Mike Pattrick, Eric Dumazet
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?
Hangbin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO
2022-04-24 2:29 ` Hangbin Liu
@ 2022-04-24 13:46 ` Willem de Bruijn
0 siblings, 0 replies; 12+ messages in thread
From: Willem de Bruijn @ 2022-04-24 13:46 UTC (permalink / raw)
To: Hangbin Liu
Cc: Willem de Bruijn, netdev, Michael S . Tsirkin, Jason Wang,
David S . Miller, Jakub Kicinski, Paolo Abeni,
Maxim Mikityanskiy, virtualization, Balazs Nemeth, Mike Pattrick,
Eric Dumazet
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO
@ 2022-04-24 13:46 ` Willem de Bruijn
0 siblings, 0 replies; 12+ messages in thread
From: Willem de Bruijn @ 2022-04-24 13:46 UTC (permalink / raw)
To: Hangbin Liu
Cc: Maxim Mikityanskiy, Willem de Bruijn, Michael S . Tsirkin,
netdev, Eric Dumazet, virtualization, Balazs Nemeth,
Jakub Kicinski, Paolo Abeni, Mike Pattrick, David S . Miller
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-04-24 13:47 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-20 8:27 [PATCH net-next] net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO Hangbin Liu
2022-04-20 13:45 ` Willem de Bruijn
2022-04-20 13:45 ` Willem de Bruijn
2022-04-21 2:31 ` Hangbin Liu
2022-04-21 14:15 ` Willem de Bruijn
2022-04-21 14:15 ` Willem de Bruijn
2022-04-22 2:08 ` Hangbin Liu
2022-04-22 21:39 ` Willem de Bruijn
2022-04-22 21:39 ` Willem de Bruijn
2022-04-24 2:29 ` Hangbin Liu
2022-04-24 13:46 ` Willem de Bruijn
2022-04-24 13:46 ` Willem de Bruijn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.