Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: Fix transmission of final, 16th fragment

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]

On Montag, 13. Februar 2017 21:00:08 CET Linus Lüssing wrote:
[...]
> And one more thing which seems fishy to me in this function:
> 
> 526         /* Make room for the fragment header. */
> 527         if (batadv_skb_head_push(skb, header_size) < 0 ||
> 528             pskb_expand_head(skb, header_size + ETH_HLEN, 0, GFP_ATOMIC) < 0) {
> 529                 ret = -ENOMEM;
> 530                 goto put_primary_if;
> 531         }
> 532 
> 533         memcpy(skb->data, &frag_header, header_size);
> 
> 
> For the pskb_expand_head() case, there is an skb_push(header_size) missing,
> isn't it?

I am a little bit confused about your remark... and about the code.

So let's check what Martin wrote:

 * get header_size more room in our data section
 * allocate new buffer to get header_size + ETH_HLEN in front (but not part)
   of our data section

If one of these two fails then it will get in panic mode and leave the
function.

I agree that the header_size in pskb_expand_head is slightly odd and I don't
see why we would need it. My best guess would be to compensate the extra
header which "stole" some bytes from the headroom which the underlying
interface may need.

But more importantly, I don't understand why an extra skb_push(header_size)
(like you've suggested) would be necessary here. Why would you want to have an
empty header_size region in the fragment between the actual header and the
fragment data?

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]