| Message ID | 1569646705-10585-1-git-send-email-srirakr2@cisco.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | AF_PACKET doesnt strip VLAN information | expand |
On Mon, Sep 30, 2019 at 1:24 AM Sriram Krishnan <srirakr2@cisco.com> wrote: > > When an application sends with AF_PACKET and places a vlan header on > the raw packet; then the AF_PACKET needs to move the tag into the skb > so that it gets processed normally through the rest of the transmit > path. > > This is particularly a problem on Hyper-V where the host only allows > vlan in the offload info. This sounds like behavior that needs to be addressed in the driver, instead? > Cc: xe-linux-external@cisco.com > --- > net/packet/af_packet.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index e2742b0..cfe0904 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -1849,15 +1849,35 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev, > return 0; > } > > -static void packet_parse_headers(struct sk_buff *skb, struct socket *sock) > +static int packet_parse_headers(struct sk_buff *skb, struct socket *sock) > { > if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) && > sock->type == SOCK_RAW) { If inside this branch, may miss packets with skb->protocol set to one of the VLAN Ethertypes. > + __be16 ethertype; > + > skb_reset_mac_header(skb); > + > + ethertype = eth_hdr(skb)->h_proto; > + /* > + * If Vlan tag is present in the packet > + * move it to skb > + */ > + if (eth_type_vlan(ethertype)) { > + int err; > + __be16 vlan_tci; > + > + err = __skb_vlan_pop(skb, &vlan_tci); > + if (unlikely(err)) > + return err; > + > + __vlan_hwaccel_put_tag(skb, ethertype, vlan_tci); What happens with multiple tags (QinQ)? > + } > + > skb->protocol = dev_parse_header_protocol(skb); > } > > skb_probe_transport_header(skb); > + return 0; > } > > /* > @@ -1979,7 +1999,9 @@ static int packet_sendmsg_spkt(struct socket *sock, struct msghdr *msg, > if (unlikely(extra_len == 4)) > skb->no_fcs = 1; > > - packet_parse_headers(skb, sock); > + err = packet_parse_headers(skb, sock); > + if (err) > + goto out_unlock; This only tests the new return value in one of three callers of packet_sendmsg_spkt.
On Mon, 30 Sep 2019 11:16:14 -0400 Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > On Mon, Sep 30, 2019 at 1:24 AM Sriram Krishnan <srirakr2@cisco.com> wrote: > > > > When an application sends with AF_PACKET and places a vlan header on > > the raw packet; then the AF_PACKET needs to move the tag into the skb > > so that it gets processed normally through the rest of the transmit > > path. > > > > This is particularly a problem on Hyper-V where the host only allows > > vlan in the offload info. > > This sounds like behavior that needs to be addressed in the driver, instead? This was what we did first, but the problem was more general. For example, many filtering functions assume that vlan tag is in skb meta data, not the packet data itself. Therefore AF_PACKET would get around any filter rules. > > > Cc: xe-linux-external@cisco.com > > --- > > net/packet/af_packet.c | 26 ++++++++++++++++++++++++-- > > 1 file changed, 24 insertions(+), 2 deletions(-) > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > > index e2742b0..cfe0904 100644 > > --- a/net/packet/af_packet.c > > +++ b/net/packet/af_packet.c > > @@ -1849,15 +1849,35 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev, > > return 0; > > } > > > > -static void packet_parse_headers(struct sk_buff *skb, struct socket *sock) > > +static int packet_parse_headers(struct sk_buff *skb, struct socket *sock) > > { > > if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) && > > sock->type == SOCK_RAW) { > > If inside this branch, may miss packets with skb->protocol set to one > of the VLAN Ethertypes. > > > + __be16 ethertype; > > + > > skb_reset_mac_header(skb); > > + > > + ethertype = eth_hdr(skb)->h_proto; > > + /* > > + * If Vlan tag is present in the packet > > + * move it to skb > > + */ > > + if (eth_type_vlan(ethertype)) { > > + int err; > > + __be16 vlan_tci; > > + > > + err = __skb_vlan_pop(skb, &vlan_tci); > > + if (unlikely(err)) > > + return err; > > + > > + __vlan_hwaccel_put_tag(skb, ethertype, vlan_tci); > > What happens with multiple tags (QinQ)? Same as multiple tags in a normal sent packet. The second tag is in the packet itself. > > > + } > > + > > skb->protocol = dev_parse_header_protocol(skb); > > } > > > > skb_probe_transport_header(skb); > > + return 0; > > } > > > > /* > > @@ -1979,7 +1999,9 @@ static int packet_sendmsg_spkt(struct socket *sock, struct msghdr *msg, > > if (unlikely(extra_len == 4)) > > skb->no_fcs = 1; > > > > - packet_parse_headers(skb, sock); > > + err = packet_parse_headers(skb, sock); > > + if (err) > > + goto out_unlock; > > This only tests the new return value in one of three callers of > packet_sendmsg_spkt.
On Tue, Oct 1, 2019 at 11:44 AM Stephen Hemminger <stephen@networkplumber.org> wrote: > > On Mon, 30 Sep 2019 11:16:14 -0400 > Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > > On Mon, Sep 30, 2019 at 1:24 AM Sriram Krishnan <srirakr2@cisco.com> wrote: > > > > > > When an application sends with AF_PACKET and places a vlan header on > > > the raw packet; then the AF_PACKET needs to move the tag into the skb > > > so that it gets processed normally through the rest of the transmit > > > path. > > > > > > This is particularly a problem on Hyper-V where the host only allows > > > vlan in the offload info. > > > > This sounds like behavior that needs to be addressed in the driver, instead? > > This was what we did first, but the problem was more general. > For example, many filtering functions assume that vlan tag is in > skb meta data, not the packet data itself. Out of curiosity, can you share an example? > Therefore AF_PACKET would > get around any filter rules. Packet sockets are not the only way to inject packets into the kernel. This probably also affects tap.
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index e2742b0..cfe0904 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -1849,15 +1849,35 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev, return 0; } -static void packet_parse_headers(struct sk_buff *skb, struct socket *sock) +static int packet_parse_headers(struct sk_buff *skb, struct socket *sock) { if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) && sock->type == SOCK_RAW) { + __be16 ethertype; + skb_reset_mac_header(skb); + + ethertype = eth_hdr(skb)->h_proto; + /* + * If Vlan tag is present in the packet + * move it to skb + */ + if (eth_type_vlan(ethertype)) { + int err; + __be16 vlan_tci; + + err = __skb_vlan_pop(skb, &vlan_tci); + if (unlikely(err)) + return err; + + __vlan_hwaccel_put_tag(skb, ethertype, vlan_tci); + } + skb->protocol = dev_parse_header_protocol(skb); } skb_probe_transport_header(skb); + return 0; } /* @@ -1979,7 +1999,9 @@ static int packet_sendmsg_spkt(struct socket *sock, struct msghdr *msg, if (unlikely(extra_len == 4)) skb->no_fcs = 1; - packet_parse_headers(skb, sock); + err = packet_parse_headers(skb, sock); + if (err) + goto out_unlock; dev_queue_xmit(skb); rcu_read_unlock();