On Sat, Jan 30, 2016 at 8:17 AM, Sowmini Varadhan <sowmini.varad...@oracle.com> wrote: > On (01/29/16 19:23), Eric Dumazet wrote: >> BTW, even a memcpy(&key_addrs->v4addrs, &iph->saddr, 8) could crash, as >> the compiler can certainly assume src and dst are 4 bytes aligned, and >> could use word accesses when inlining memcpy() even on Sparc. >> >> Apparently the compiler used by Sowmini is gentle. > > One more subtlety that I missed until now.. > > eth_get_headlen passes in flow_keys_buf_dissector (NOT flow_keys_dissector!) > > So FLOW_DISSECTOR_KEY_IPV4_ADDRS is not set, and this helps to dodge > the unaligned iph->saddr access.
Right this is a bug that got introduced in 4.2 when the check for !skb was dropped in favor of the flow keys. My thought was the patch I provided fixes 4.3, and could be applied to stable for 4.2. The concerns about GRE TEB and VXLAN go much further back then that since if I am not mistaken the transmit path for both is pretty much borked if we try to do GSO as either the inner headers or outer headers are not aligned so assembling the header will likely result in unaligned accesses. > But as others have pointed out, much of this code is brittle > because it's accessing the data before the driver has had a chance > to align things. The page_offset initialization of NET_IP_ALIGN, > with all its weaknesses, at least matches (in principle) the prescription > used for the xmit path. That is essentially an unfortunate side effect of this being a function that has two very different roles to perform. One goes through and just determines the length of the frame, the other goes through and gathers data for computing a hash. As far as the NET_IP_ALIGN on the page offset I think it is a horrible idea. Basically it means we have to allocate at least 1K more space than we need since page sizes are powers of 2, and buffer sizes in hardware are measured in 1K increments. Adding the unaligned reads probably doesn't do much to fix the actual issue which is the unaligned headers (either inner or outer) on architectures that don't support unaligned access. If anything what I really think we should look at doing is coming up with a way to just pad the inner headers by NET_IP_ALIGN bytes if the architecture doesn't support unaligned access and then output the inner and outer headers as two separate buffers. - Alex
