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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 01 Mar 2016 11:18:12 +0100
From:	Daniel Borkmann <daniel@...earbox.net>
To:	Benjamin Poirier <bpoirier@...e.com>, netdev@...r.kernel.org
CC:	Eric Dumazet <eric.dumazet@...il.com>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>,
	Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>
Subject: Re: [PATCH net v2] mld, igmp: Fix reserved tailroom calculation

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.

Thanks again,
Daniel