Re: bug in ixgbe_atr

[Sowmini Varadhan <sowmini.varadhan@oracle.com>]

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