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 & 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?