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] [day] [month] [year] [list]

Date:   Fri, 16 Feb 2018 15:25:31 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     jon.maloy@...csson.com
Cc:     netdev@...r.kernel.org,
        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

From: Jon Maloy <jon.maloy@...csson.com>
Date: Thu, 15 Feb 2018 08:57:14 +0000

> The buffers we are cloning are linearized 1 MTU incoming
> buffers. There are no fragments.  Each clone normally points to only
> a tiny fraction of the data area of the base buffer.  I don't claim
> that copying always is bad, but in this case it happens in the
> majority of cases, and as I see it completely unnecessarily.
> 
> There is actually some under accounting, however, since we now only
> count the data area of the base buffer (== the sum of the data area
> of the clones) plus the overhead of the clones.  A more accurate
> calculation, taking into account even the overhead of the base
> buffer, would look like this: (*iskb)->truesize = SKB_TRUSIZE(imsz)
> + (skb->truesize - skb->len) / msg_msgcnt(msg);
> 
> I.e., we calculate the overhead of the base buffer and divide it
> equally among the clones.  Now I really can't see we are missing
> anything.

Maybe there is some miscommunication between us.  Let me try using
different words.

I you have a single SKB which has a truesize of, say, 10K and then you
clone this several times to point the SKB into small windows into that
original SKB's buffer, then you must use the same 10K truesize for the
clones that you did for the original SKB.

It absolutely does not matter that the clones are only pointing to a
small part of the original SKB's buffer.  Until they are freed up each
individual SKB still causes that _ENTIRE_ buffer to be held up and not
freeable.

This is why you have to be careful how you manage SKBs, and in fact
what you are doing by cloning and constricting the data pointers, is
actually troublesome and you can see this with the truesize problems
you are running into.

SKBs are really not built to be managed like this.