netdev - Re: [PATCH net v2] mld, igmp: Fix reserved tailroom calculation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56D5BCA1.4050206@stressinduktion.org>
Date:	Tue, 1 Mar 2016 17:00:33 +0100
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Daniel Borkmann <daniel@...earbox.net>,
	Benjamin Poirier <bpoirier@...e.com>, netdev@...r.kernel.org
Cc:	Eric Dumazet <eric.dumazet@...il.com>,
	Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>
Subject: Re: [PATCH net v2] mld, igmp: Fix reserved tailroom calculation

On 01.03.2016 11:18, Daniel Borkmann wrote:
> On 03/01/2016 12:03 AM, Benjamin Poirier wrote:
> [...]
>> Notes:
>>      Changes v1->v2
>>      As suggested by Hannes, move the code to an inline helper and
>> express it
>>      using "if" rather than "min".
>
> The code is correct, thanks!
>
> Therefore:
>
> Acked-by: Daniel Borkmann <daniel@...earbox.net>
>
> However, I actually think that v1 was much better/easier as a fix
> though. :/
>
> Meaning 1) it's likely easier to backport, and 2) that we now need a
> comment
> above each skb->reserved_tailroom assignment probably says that min() might
> perhaps have been more self-documenting ...
>
> skb_tailroom_reserve() looks quite generic, but it only makes sense to
> use in
> combination with skb_availroom(), which would have been good to put a
> note to
> the header comment. Also "the required headroom should already have been
> reserved before using this function", places one more requirement for
> usage.
>
> If we really want to go that path, maybe rather a skb_setroom() that is
> coupled
> with skb_availroom() like:
>
> static inline int __skb_tailroom(const struct sk_buff *skb)
> {
>      return skb->end - skb->tail;
> }
>
> static inline void skb_setroom(struct sk_buff *skb,
>                                 unsigned int needed_headroom,
>                                 unsigned int size,
>                                 unsigned int needed_tailroom)
> {
>          SKB_LINEAR_ASSERT(skb);
>
>          skb_reserve(skb, needed_headroom);
>          skb->reserved_tailroom = needed_tailroom;
>
>          if (size < __skb_tailroom(skb) - needed_tailroom)
>                  skb->reserved_tailroom = __skb_tailroom(skb) - size;
> }
>
> Then, skb_tailroom() would internally use __skb_tailroom(), too. And we
> can also
> spare us the two unneeded skb_is_nonlinear() checks in our helper which
> will
> currently always evaluate to false anyway.
>
> ... just a thought.

I think it is fine. The code will be inlined anyway and probably skb 
linear assertions will be optimized away.

I like the current code in its form:

skb_reserve(skb, header);
skb_tailroom_reserve(skb, mtu, tailroom);

Combined form has too many arguments, so one has to look up the header 
file again.

Bye,
Hannes