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

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

On 2013-09-30 17:09, Arvid Brodin wrote:
> 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.
> 

Obviously, David is too busy to help me figure out what the problem is 
(I know he reviews several thousand patches each year, so maybe that's
no wonder). 

If anyone else has got an idea you are very welcome to chime in, and
perhaps we can solve this. I can't fix the problem if I don't understand
it.


On 2013-09-20 21:10, David Miller wrote:
> Secondly, everything after the header is now at the wrong offset from
> the beginning of the packet.

Maybe he's talking about systems with NET_SKBUFF_DATA_USES_OFFSET here? 
This means the transport, network and mac headers are all relative to 
skb->head, if I understand correctly. But at the point in the protocol 
stack where this code is (the Ethernet protocol handler), the transport 
and network headers have not been set yet, and the mac header is not
moved by the code. And tail is updated by the code. So that should not 
be a problem?


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