netdev - Re: [net-next 1/1] tipc: avoid unnecessary copying of bundled messages

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]

Message-ID: <1518615756.3715.198.camel@gmail.com>
Date:   Wed, 14 Feb 2018 05:42:36 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Jon Maloy <jon.maloy@...csson.com>, davem@...emloft.net,
        netdev@...r.kernel.org
Cc:     mohan.krishna.ghanta.krishnamurthy@...csson.com,
        tung.q.nguyen@...tech.com.au, hoang.h.le@...tech.com.au,
        canh.d.luu@...tech.com.au, ying.xue@...driver.com,
        tipc-discussion@...ts.sourceforge.net
Subject: Re: [net-next  1/1] tipc: avoid unnecessary copying of bundled
 messages

On Wed, 2018-02-14 at 13:50 +0100, Jon Maloy wrote:
> A received sk buffer may contain dozens of smaller 'bundled' messages
> which after extraction go each in their own direction.
> 
> Unfortunately, when we extract those messages using skb_clone() each
> of the extracted buffers inherit the truesize value of the original
> buffer. Apart from causing massive overaccounting of the base buffer's
> memory, this often causes tipc_msg_validate() to come to the false
> conclusion that the ratio truesize/datasize > 4, and perform an
> unnecessary copying of the extracted buffer.
> 
> We now fix this problem by explicitly correcting the truesize value of
> the buffer clones to be the truesize of the clone itself. This change
> eliminates both the overaccounting and the unnecessary buffer copying.
> 
> Reported-by: Hoang Le <hoang.h.le@...tek.com.au>
> Acked-by: Ying Xue <ying.xue@...driver.com>
> Signed-off-by: Jon Maloy <jon.maloy@...csson.com>
> ---
>  net/tipc/msg.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 4e1c6f6..a368fa8 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -434,6 +434,9 @@ bool tipc_msg_extract(struct sk_buff *skb, struct sk_buff **iskb, int *pos)
>  	skb_pull(*iskb, offset);
>  	imsz = msg_size(buf_msg(*iskb));
>  	skb_trim(*iskb, imsz);
> +
> +	/* Scale extracted buffer's truesize to avoid double accounting */
> +	(*iskb)->truesize = SKB_TRUESIZE(imsz);

How do you guarantee that under accounting wont happen here ?

Copying data to avoid OOM is not necessarily bad.

TCP stack does this under stress (this is called collapsing),
and this definitely can happen.

skb_clone() will also clones frags, and you absolutely do not know what
memory each frag can hold (that could be 64KB on arches with 64KB
pages)