bug in ixgbe_atr

bug in ixgbe_atr

[Sowmini Varadhan]

When I was playing around with TPACKET_V2, I think I ran
into a bug in ixgbe_atr(): if I get here with an 
sk_buff that has, e.g., just 14 bytes in the header, then 
the skb_network_header is invalid. I found my kernel sometimes
wandering off into  ipv6_find_hdr() in an attempt to get the 
l4_proto, and then complaining that "IPv6 header not found\n"
(this was an ipv4 packet). 

I think we want to use skb_header_pointer in ixgbe_atr to get 
the network header itself.. I tried the patch below, and it 
works for simple (non-vlan, basic) ethernet header, but probably
needs more refinement to work for more complex encapsulations?

And other drivers may need a similar fix too, I've not checked yet.

--Sowmini

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index a244d9a..be453c6 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7627,6 +7627,10 @@ static void ixgbe_atr(struct ixgbe_ring *ring,
 		struct iphdr *ipv4;
 		struct ipv6hdr *ipv6;
 	} hdr;
+	union {
+		struct iphdr ipv4;
+		struct ipv6hdr ipv6;
+	} ip_hdr;
 	struct tcphdr *th;
 	unsigned int hlen;
 	struct sk_buff *skb;
@@ -7667,13 +7671,15 @@ static void ixgbe_atr(struct ixgbe_ring *ring,
 	}
 
 	/* Currently only IPv4/IPv6 with TCP is supported */
-	switch (hdr.ipv4->version) {
-	case IPVERSION:
+	switch (ntohs(first->protocol)) {
+	case ETH_P_IP:
+		skb_header_pointer(skb, ETH_HLEN, sizeof (struct iphdr),
+				   &ip_hdr);
 		/* access ihl as u8 to avoid unaligned access on ia64 */
-		hlen = (hdr.network[0] & 0x0F) << 2;
-		l4_proto = hdr.ipv4->protocol;
+		hlen = ip_hdr.ipv4.ihl << 2;
+		l4_proto = ip_hdr.ipv4.protocol;
 		break;
-	case 6:
+	case ETH_P_IPV6:
 		hlen = hdr.network - skb->data;
 		l4_proto = ipv6_find_hdr(skb, &hlen, IPPROTO_TCP, NULL, NULL);
 		hlen -= hdr.network - skb->data;

RE: bug in ixgbe_atr

[Duyck, Alexander H]

> -----Original Message-----
> From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
> Sent: Thursday, October 13, 2016 6:44 PM
> To: Duyck, Alexander H <alexander.h.duyck@intel.com>;
> netdev@vger.kernel.org
> Subject: bug in ixgbe_atr
> 
> When I was playing around with TPACKET_V2, I think I ran into a bug in
> ixgbe_atr(): if I get here with an sk_buff that has, e.g., just 14 bytes in the
> header, then the skb_network_header is invalid. I found my kernel sometimes
> wandering off into  ipv6_find_hdr() in an attempt to get the l4_proto, and then
> complaining that "IPv6 header not found\n"
> (this was an ipv4 packet).
> 
> I think we want to use skb_header_pointer in ixgbe_atr to get the network
> header itself.. I tried the patch below, and it works for simple (non-vlan, basic)
> ethernet header, but probably needs more refinement to work for more
> complex encapsulations?
> 
> And other drivers may need a similar fix too, I've not checked yet.
> 
> --Sowmini
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index a244d9a..be453c6 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7627,6 +7627,10 @@ static void ixgbe_atr(struct ixgbe_ring *ring,
>  		struct iphdr *ipv4;
>  		struct ipv6hdr *ipv6;
>  	} hdr;
> +	union {
> +		struct iphdr ipv4;
> +		struct ipv6hdr ipv6;
> +	} ip_hdr;
>  	struct tcphdr *th;
>  	unsigned int hlen;
>  	struct sk_buff *skb;
> @@ -7667,13 +7671,15 @@ static void ixgbe_atr(struct ixgbe_ring *ring,
>  	}
> 
>  	/* Currently only IPv4/IPv6 with TCP is supported */
> -	switch (hdr.ipv4->version) {
> -	case IPVERSION:
> +	switch (ntohs(first->protocol)) {
> +	case ETH_P_IP:
> +		skb_header_pointer(skb, ETH_HLEN, sizeof (struct iphdr),
> +				   &ip_hdr);
>  		/* access ihl as u8 to avoid unaligned access on ia64 */
> -		hlen = (hdr.network[0] & 0x0F) << 2;
> -		l4_proto = hdr.ipv4->protocol;
> +		hlen = ip_hdr.ipv4.ihl << 2;
> +		l4_proto = ip_hdr.ipv4.protocol;
>  		break;
> -	case 6:
> +	case ETH_P_IPV6:
>  		hlen = hdr.network - skb->data;
>  		l4_proto = ipv6_find_hdr(skb, &hlen, IPPROTO_TCP, NULL,
> NULL);
>  		hlen -= hdr.network - skb->data;

The problem is this will break other stuff, for example I have seen the ihl access actually cause problems with unaligned accesses as some architectures decide to pull it as a u32 and then mask it.

My advice would be to keep this simple.  Add a check to make sure we have room for at least skb_headlen(skb) - 40  >= hrd.raw - skb->data.  Messing with the protocol bits will break stuff since there is support for tunneling also floating around in here now.

I believe we are planning on dropping this code in favor of ndo_rx_flow_steer in the future.  If we do that then the whole problem becomes moot.

- Alex

Re: bug in ixgbe_atr

[Sowmini Varadhan]

On (10/14/16 02:06), Duyck, Alexander H wrote:
> > +	case ETH_P_IP:
> > +		skb_header_pointer(skb, ETH_HLEN, sizeof (struct iphdr),
> > +				   &ip_hdr);
> >  		/* access ihl as u8 to avoid unaligned access on ia64 */
> > -		hlen = (hdr.network[0] & 0x0F) << 2;
> > -		l4_proto = hdr.ipv4->protocol;
> > +		hlen = ip_hdr.ipv4.ihl << 2;
> > +		l4_proto = ip_hdr.ipv4.protocol;
> >  		break;
  :
> The problem is this will break other stuff, for example I have seen
> the ihl access actually cause problems with unaligned accesses as some
> architectures decide to pull it as a u32 and then mask it.

Yes, I noticed that u8 comment for ia64.. if that's the only issue
here, we could just reset hdr.network to &ip_hdr..

However, I suspect the above patch is probably not going to work for
the vlan case (it was just a first-pass hack)

> My advice would be to keep this simple.  Add a check to make sure we
> have room for at least skb_headlen(skb) - 40  >= hrd.raw - skb->data.

I don't parse that- the hdr union in ixgbe_atr doesnt have a ->raw
field. Can you explain?

> Messing with the protocol bits will break stuff since there is support
> for tunneling also floating around in here now.
> 
> I believe we are planning on dropping this code in favor of
> ndo_rx_flow_steer in the future.  If we do that then the whole problem
> becomes moot.

Dropping it is fine with me I guess - maybe just return, if the
skb_headlen() doesnt have enough bytes for a network header, 
i.e., skb_headlen is at least ETH_HLEN + sizeof (struct iphdr) for
ETH_P_IP, or  ETH_HLEN + sizeof (struct ipv6hdr) for ETH_P_IPV6?

--Sowmini

RE: bug in ixgbe_atr

[Duyck, Alexander H]

> -----Original Message-----
> From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
> Sent: Thursday, October 13, 2016 8:49 PM
> To: Duyck, Alexander H <alexander.h.duyck@intel.com>
> Cc: netdev@vger.kernel.org
> Subject: Re: bug in ixgbe_atr
> 
> On (10/14/16 02:06), Duyck, Alexander H wrote:
> > > +	case ETH_P_IP:
> > > +		skb_header_pointer(skb, ETH_HLEN, sizeof (struct iphdr),
> > > +				   &ip_hdr);
> > >  		/* access ihl as u8 to avoid unaligned access on ia64 */
> > > -		hlen = (hdr.network[0] & 0x0F) << 2;
> > > -		l4_proto = hdr.ipv4->protocol;
> > > +		hlen = ip_hdr.ipv4.ihl << 2;
> > > +		l4_proto = ip_hdr.ipv4.protocol;
> > >  		break;
>   :
> > The problem is this will break other stuff, for example I have seen
> > the ihl access actually cause problems with unaligned accesses as some
> > architectures decide to pull it as a u32 and then mask it.
> 
> Yes, I noticed that u8 comment for ia64.. if that's the only issue here, we could
> just reset hdr.network to &ip_hdr..
> 
> However, I suspect the above patch is probably not going to work for the vlan
> case (it was just a first-pass hack)

I kind of figured that.  Ideally we only wat to pick out the pieces we need.  I would prefer to avoid skb_header_pointer if possible since we only need a few parts of the header and don't really need to copy the whole thing.

> > My advice would be to keep this simple.  Add a check to make sure we
> > have room for at least skb_headlen(skb) - 40  >= hrd.raw - skb->data.
> 
> I don't parse that- the hdr union in ixgbe_atr doesnt have a ->raw field. Can you
> explain?

Sorry I was thinking of a different piece of code.  In the case of the atr code it would be hdr.network, not hdr.raw.  Basically the thought was to validate that there is enough data in skb_headlen() that we can verify that from where the network header should be we have at least 40 bytes of data as that would be the minimum needed for a TCP header and an IPv4 header, or just an IPv6 header.  We would probably need a separate follow-up for the TCP header after we validate network header.

> > Messing with the protocol bits will break stuff since there is support
> > for tunneling also floating around in here now.
> >
> > I believe we are planning on dropping this code in favor of
> > ndo_rx_flow_steer in the future.  If we do that then the whole problem
> > becomes moot.
> 
> Dropping it is fine with me I guess - maybe just return, if the
> skb_headlen() doesnt have enough bytes for a network header, i.e., skb_headlen
> is at least ETH_HLEN + sizeof (struct iphdr) for ETH_P_IP, or  ETH_HLEN + sizeof
> (struct ipv6hdr) for ETH_P_IPV6?
> 
> --Sowmini

Right that is kind of what I was thinking.  If we validate that we have at least 40 before inspecting the network header, and at least 20 before we validate the TCP header that would work for me.

- Alex

Re: bug in ixgbe_atr

[Sowmini Varadhan]

On (10/14/16 16:09), Duyck, Alexander H wrote:
> Sorry I was thinking of a different piece of code.  In the case of the
> atr code it would be hdr.network, not hdr.raw.  Basically the thought
> was to validate that there is enough data in skb_headlen() that we can
> verify that from where the network header should be we have at least
> 40 bytes of data as that would be the minimum needed for a TCP header
> and an IPv4 header, or just an IPv6 header.  We would probably need a
> separate follow-up for the TCP header after we validate network header.
   :
>> Dropping it is fine with me I guess - maybe just return, if the
>> skb_headlen() doesnt have enough bytes for a network header, i.e.,
>> skb_headlen
>> is at least ETH_HLEN + sizeof (struct iphdr) for ETH_P_IP, or  ETH_HLEN +
>> sizeof (struct ipv6hdr) for ETH_P_IPV6?

> Right that is kind of what I was thinking.  If we validate that we
> have at least 40 before inspecting the network header, and at least 20
> before we validate the TCP header that would work for me.

yes, I was on a plane through most of the day today but thought about
this. I think we can check if skb_network_offset() is between
skb->data and tail, and also make sure there are "enough" bytes for
trying to find the ip and transport header. 
Let me try to put a RFC patch together for this tomorrow.