[PATCH V2 RFC 0/2] ixgbe: ixgbe_atr() bug fixes

[PATCH V2 RFC 0/2] ixgbe: ixgbe_atr() bug fixes

[Sowmini Varadhan]

Two bug fixes:
- ixgbe_atr() should check for protocol == udp in the
  skb->encapsulation case (instead of !=)
- ixgbe_atr() should make sure the non-paged data has the
  needed network/transport header for computing l4_proto.

Sowmini Varadhan (2):
  ixgbe: ixgbe_atr() should access udp_hdr(skb) only for UDP packets
  ixgbe: ixgbe_atr() compute l4_proto only if non-paged data has
    network/transport headers

 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

[PATCH V2 RFC 1/2] ixgbe: ixgbe_atr() should access udp_hdr(skb) only for UDP packets

[Sowmini Varadhan]

Commit 9f12df906cd8 ("ixgbe: Store VXLAN port number in network order")
incorrectly checks for hdr.ipv4->protocol != IPPROTO_UDP
in ixgbe_atr(). This check should be for "==" instead.

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index a244d9a..eceb47b 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7653,7 +7653,7 @@ static void ixgbe_atr(struct ixgbe_ring *ring,
 	hdr.network = skb_network_header(skb);
 	if (skb->encapsulation &&
 	    first->protocol == htons(ETH_P_IP) &&
-	    hdr.ipv4->protocol != IPPROTO_UDP) {
+	    hdr.ipv4->protocol == IPPROTO_UDP) {
 		struct ixgbe_adapter *adapter = q_vector->adapter;
 
 		/* verify the port is recognized as VXLAN */
-- 
1.7.1

[PATCH V2 RFC 2/2] ixgbe: ixgbe_atr() compute l4_proto only if non-paged data has network/transport headers

[Sowmini Varadhan]

For some Tx paths (e.g., tpacket_snd()), ixgbe_atr may be
passed down an sk_buff that has the network and transport
header in the paged data, so it needs to make sure these
headers are available in the headlen bytes to calculate the
l4_proto.

This patch expect that network and transport headers are
already available in the non-paged header dat.  The assumption
is that the caller has set this up if l4_proto based Tx
steering is desired.

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index eceb47b..2cc1dae 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -54,6 +54,7 @@
 #include <net/pkt_cls.h>
 #include <net/tc_act/tc_gact.h>
 #include <net/tc_act/tc_mirred.h>
+#include <net/vxlan.h>
 
 #include "ixgbe.h"
 #include "ixgbe_common.h"
@@ -7651,11 +7652,16 @@ static void ixgbe_atr(struct ixgbe_ring *ring,
 	/* snag network header to get L4 type and address */
 	skb = first->skb;
 	hdr.network = skb_network_header(skb);
+	if (hdr.network <= skb->data || hdr.network >= skb_tail_pointer(skb))
+		return;
 	if (skb->encapsulation &&
 	    first->protocol == htons(ETH_P_IP) &&
 	    hdr.ipv4->protocol == IPPROTO_UDP) {
 		struct ixgbe_adapter *adapter = q_vector->adapter;
 
+		if (skb_tail_pointer(skb) < hdr.network + VXLAN_HEADROOM)
+			return;
+
 		/* verify the port is recognized as VXLAN */
 		if (adapter->vxlan_port &&
 		    udp_hdr(skb)->dest == adapter->vxlan_port)
@@ -7666,15 +7672,27 @@ static void ixgbe_atr(struct ixgbe_ring *ring,
 			hdr.network = skb_inner_network_header(skb);
 	}
 
+	/* Make sure we have at least [minimum IPv4 header + TCP]
+	 * or [IPv6 header] bytes
+	 */
+	if (skb_tail_pointer(skb) < hdr.network + 40)
+		return;
+
 	/* Currently only IPv4/IPv6 with TCP is supported */
 	switch (hdr.ipv4->version) {
 	case IPVERSION:
 		/* access ihl as u8 to avoid unaligned access on ia64 */
 		hlen = (hdr.network[0] & 0x0F) << 2;
+		if (skb_tail_pointer(skb) < hdr.network + hlen +
+					    sizeof(struct tcphdr))
+			return;
 		l4_proto = hdr.ipv4->protocol;
 		break;
 	case 6:
 		hlen = hdr.network - skb->data;
+		if (skb_tail_pointer(skb) < hdr.network + hlen +
+					    sizeof(struct tcphdr))
+			return;
 		l4_proto = ipv6_find_hdr(skb, &hlen, IPPROTO_TCP, NULL, NULL);
 		hlen -= hdr.network - skb->data;
 		break;
-- 
1.7.1

Re: [Intel-wired-lan] [PATCH V2 RFC 2/2] ixgbe: ixgbe_atr() compute l4_proto only if non-paged data has network/transport headers

[Alexander Duyck]

On Mon, Oct 17, 2016 at 10:25 AM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> For some Tx paths (e.g., tpacket_snd()), ixgbe_atr may be
> passed down an sk_buff that has the network and transport
> header in the paged data, so it needs to make sure these
> headers are available in the headlen bytes to calculate the
> l4_proto.
>
> This patch expect that network and transport headers are
> already available in the non-paged header dat.  The assumption
> is that the caller has set this up if l4_proto based Tx
> steering is desired.
>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index eceb47b..2cc1dae 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -54,6 +54,7 @@
>  #include <net/pkt_cls.h>
>  #include <net/tc_act/tc_gact.h>
>  #include <net/tc_act/tc_mirred.h>
> +#include <net/vxlan.h>
>
>  #include "ixgbe.h"
>  #include "ixgbe_common.h"
> @@ -7651,11 +7652,16 @@ static void ixgbe_atr(struct ixgbe_ring *ring,
>         /* snag network header to get L4 type and address */
>         skb = first->skb;
>         hdr.network = skb_network_header(skb);
> +       if (hdr.network <= skb->data || hdr.network >= skb_tail_pointer(skb))
> +               return;

I would say you probably only need the first check here for skb->data
and could probably skip the second part.  You will be testing for
skb_tail_pointer in all the other tests you added so this check is
redundant anyway.

Also you might want to go through and wrap these with unlikely() since
most of these are exception cases.

>         if (skb->encapsulation &&
>             first->protocol == htons(ETH_P_IP) &&
>             hdr.ipv4->protocol == IPPROTO_UDP) {
>                 struct ixgbe_adapter *adapter = q_vector->adapter;
>
> +               if (skb_tail_pointer(skb) < hdr.network + VXLAN_HEADROOM)
> +                       return;
> +
>                 /* verify the port is recognized as VXLAN */
>                 if (adapter->vxlan_port &&
>                     udp_hdr(skb)->dest == adapter->vxlan_port)
> @@ -7666,15 +7672,27 @@ static void ixgbe_atr(struct ixgbe_ring *ring,
>                         hdr.network = skb_inner_network_header(skb);
>         }
>
> +       /* Make sure we have at least [minimum IPv4 header + TCP]
> +        * or [IPv6 header] bytes
> +        */
> +       if (skb_tail_pointer(skb) < hdr.network + 40)
> +               return;
> +
>         /* Currently only IPv4/IPv6 with TCP is supported */
>         switch (hdr.ipv4->version) {
>         case IPVERSION:
>                 /* access ihl as u8 to avoid unaligned access on ia64 */
>                 hlen = (hdr.network[0] & 0x0F) << 2;
> +               if (skb_tail_pointer(skb) < hdr.network + hlen +
> +                                           sizeof(struct tcphdr))
> +                       return;
>                 l4_proto = hdr.ipv4->protocol;
>                 break;
>         case 6:
>                 hlen = hdr.network - skb->data;
> +               if (skb_tail_pointer(skb) < hdr.network + hlen +
> +                                           sizeof(struct tcphdr))
> +                       return;
>                 l4_proto = ipv6_find_hdr(skb, &hlen, IPPROTO_TCP, NULL, NULL);
>                 hlen -= hdr.network - skb->data;
>                 break;

I believe one more check is needed after this block to verify the TCP
header fields are present.

So you probably need to add a check for "skb_tail_pointer(skb) <
(hdr.network + hlen + 20)".

Thanks.

- Alex

Re: [Intel-wired-lan] [PATCH V2 RFC 2/2] ixgbe: ixgbe_atr() compute l4_proto only if non-paged data has network/transport headers

[Sowmini Varadhan]

On (10/17/16 11:15), Alexander Duyck wrote:
> I would say you probably only need the first check here for skb->data
> and could probably skip the second part.  You will be testing for
> skb_tail_pointer in all the other tests you added so this check is
> redundant anyway.
> 
> Also you might want to go through and wrap these with unlikely() since
> most of these are exception cases.

Ok.. v3 will have this.

> >         /* Currently only IPv4/IPv6 with TCP is supported */
> >         switch (hdr.ipv4->version) {
> >         case IPVERSION:
> >                 /* access ihl as u8 to avoid unaligned access on ia64 */
> >                 hlen = (hdr.network[0] & 0x0F) << 2;
> > +               if (skb_tail_pointer(skb) < hdr.network + hlen +
> > +                                           sizeof(struct tcphdr))
> > +                       return;
> >                 l4_proto = hdr.ipv4->protocol;
> >                 break;
> >         case 6:
> >                 hlen = hdr.network - skb->data;
> > +               if (skb_tail_pointer(skb) < hdr.network + hlen +
> > +                                           sizeof(struct tcphdr))
> > +                       return;
> >                 l4_proto = ipv6_find_hdr(skb, &hlen, IPPROTO_TCP, NULL, NULL);
> >                 hlen -= hdr.network - skb->data;
> >                 break;
> 
> I believe one more check is needed after this block to verify the TCP
> header fields are present.
> 
> So you probably need to add a check for "skb_tail_pointer(skb) <
> (hdr.network + hlen + 20)".

But isnt that the same thing as the checks before l4_proto computation above?

--Sowmini

Re: [Intel-wired-lan] [PATCH V2 RFC 2/2] ixgbe: ixgbe_atr() compute l4_proto only if non-paged data has network/transport headers

[Alexander Duyck]

On Mon, Oct 17, 2016 at 12:18 PM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (10/17/16 11:15), Alexander Duyck wrote:
>> I would say you probably only need the first check here for skb->data
>> and could probably skip the second part.  You will be testing for
>> skb_tail_pointer in all the other tests you added so this check is
>> redundant anyway.
>>
>> Also you might want to go through and wrap these with unlikely() since
>> most of these are exception cases.
>
> Ok.. v3 will have this.
>
>> >         /* Currently only IPv4/IPv6 with TCP is supported */
>> >         switch (hdr.ipv4->version) {
>> >         case IPVERSION:
>> >                 /* access ihl as u8 to avoid unaligned access on ia64 */
>> >                 hlen = (hdr.network[0] & 0x0F) << 2;
>> > +               if (skb_tail_pointer(skb) < hdr.network + hlen +
>> > +                                           sizeof(struct tcphdr))
>> > +                       return;
>> >                 l4_proto = hdr.ipv4->protocol;
>> >                 break;
>> >         case 6:
>> >                 hlen = hdr.network - skb->data;
>> > +               if (skb_tail_pointer(skb) < hdr.network + hlen +
>> > +                                           sizeof(struct tcphdr))
>> > +                       return;
>> >                 l4_proto = ipv6_find_hdr(skb, &hlen, IPPROTO_TCP, NULL, NULL);
>> >                 hlen -= hdr.network - skb->data;
>> >                 break;
>>
>> I believe one more check is needed after this block to verify the TCP
>> header fields are present.
>>
>> So you probably need to add a check for "skb_tail_pointer(skb) <
>> (hdr.network + hlen + 20)".
>
> But isnt that the same thing as the checks before l4_proto computation above?

Sort of.  The problem is IPv6 can include extension headers and that
can totally mess with us.  So we need to do one more check to verify
that we have enough space for IPv6 w/ TCP which would be hdr.raw + 20
+ hlenl.

Thanks.

- Alex

Re: [Intel-wired-lan] [PATCH V2 RFC 2/2] ixgbe: ixgbe_atr() compute l4_proto only if non-paged data has network/transport headers

[Sowmini Varadhan]

On (10/17/16 12:49), Alexander Duyck wrote:
> >> >         /* Currently only IPv4/IPv6 with TCP is supported */
> >> >         switch (hdr.ipv4->version) {
> >> >         case IPVERSION:
> >> >                 /* access ihl as u8 to avoid unaligned access on ia64 */
> >> >                 hlen = (hdr.network[0] & 0x0F) << 2;
> >> > +               if (skb_tail_pointer(skb) < hdr.network + hlen +
> >> > +                                           sizeof(struct tcphdr))
> >> > +                       return;
> >> >                 l4_proto = hdr.ipv4->protocol;
> >> >                 break;
> >> >         case 6:
> >> >                 hlen = hdr.network - skb->data;
> >> > +               if (skb_tail_pointer(skb) < hdr.network + hlen +
> >> > +                                           sizeof(struct tcphdr))
> >> > +                       return;
> >> >                 l4_proto = ipv6_find_hdr(skb, &hlen, IPPROTO_TCP, NULL, NULL);
> >> >                 hlen -= hdr.network - skb->data;
> >> >                 break;
   :
> >> So you probably need to add a check for "skb_tail_pointer(skb) <
> >> (hdr.network + hlen + 20)".
> >
> > But isnt that the same thing as the checks before l4_proto computation above?
> 
> Sort of.  The problem is IPv6 can include extension headers and that
> can totally mess with us.  So we need to do one more check to verify
> that we have enough space for IPv6 w/ TCP which would be hdr.raw + 20
> + hlenl.

Yes, you are right. So given that I already check that I have
at least 40 bytes past the network header, and ipv6_find_hdr
will pull up exthdrs as needed, my checks are not needed, and the
real ones should happen after we come out of that switch().

--Sowmini