Re: [PATCH v4] net/hsr: Add support for the High-availability Seamless Redundancy protocol (HSRv0)

[Arvid Brodin <arvid.brodin@xdin.com>]

On 2013-09-20 21:10, David Miller wrote:
> From: Arvid Brodin <arvid.brodin@xdin.com>
> Date: Thu, 19 Sep 2013 03:11:58 +0200
> 
>> +#if !defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
>> +		/* We need to memmove the whole header to work around
>> +		 * alignment problems caused by the 6-byte HSR tag.
>> +		 */
>> +		memmove(skb_deliver->data - HSR_TAGLEN, skb_deliver->data,
>> +			skb_headlen(skb_deliver));
>> +		skb_deliver->data -= HSR_TAGLEN;
>> +		skb_deliver->tail -= HSR_TAGLEN;
>> +#endif
> 
> You can't do this.
> 
> First of all, you have no idea if subtracting skb->data a given amount
> will underflow the skb buffer start.  You aren't even checking, all
> of the standard skb_*() data adjustment interfaces do.

Isn't it reasonable to assume that skb points to an sk_buff with a
valid Ethernet header, and with skb->data pointing to the end of that
header, at the start of ETH_P_... protocol handlers?


> Secondly, everything after the header is now at the wrong offset from
> the beginning of the packet.
> 
> You will have to memmove() the entire packet if you want to realign
> where it starts.

Are you saying packet data may contain offsets from itself to the mac 
header? Or are you talking about fragments? This being the Ethernet 
protocol handler, e.g. skb->network_header and skb->transport_header 
should not have been set yet.

Anyway, I should do
	skb_deliver->len -= HSR_TAGLEN;
here! (I'm removing 6 bytes after the mac header from the packet, not 
just realigning it.)

Given all this, and looking at e.g. __pskb_pull_tail(), it should be 
correct to memmove() only the linear part of the packet after the removed 
bytes - any fragments will just be appended directly after the end of the 
(shortened) linear part on use?

It's weird though, the "len" bug doesn't show up in my practical tests - 
scp of 16 MiB file (sha1sum compared afterwards) and ping with default 
and 65 KB packets work just fine before and after this change - even
between one fixed system and one with the bug remaining. Could it be
that none of the network device drivers we use (e1000e and macb) make 
use of nonlinear skbs?


-- 
Arvid Brodin | Consultant (Linux)
XDIN AB | Knarrarnäsgatan 7 | SE-164 40 Kista | Sweden | xdin.com