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.

(Shorter and more to the point than my previous replies:)

I _do_ know: this can't possibly underflow since strip_hsr_tag() a 
few lines above pulled the same amount of data. I will rename 
strip_hsr_tag() to hsr_pull_tag() to make this clearer.


> Secondly, everything after the header is now at the wrong offset from
> the beginning of the packet.

How does this matter? The memmove moves everything back (restores the 
changes made to the packet on the sending side) so that it is at the
"normal" position for an ethernet packet.


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