netdev - RE: [PATCH 1/1] cdc_ncm: Fix TCP Window Size issue by CDC NCM driver

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:	Mon, 13 Jun 2011 13:24:38 +0200
From:	Elof Vrigborn <elof.vrigborn@...csson.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	Oliver Neukum <oliver@...kum.name>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH 1/1] cdc_ncm: Fix TCP Window Size issue by CDC NCM driver

> 
> I find this patch dubious.
> 
> skb truesize is not meant to be skb->len + sizeof(struct sk_buff);
> 
> truesize is really accounting for the truesize of memory 
> blocks, not the used one.
> 
> Many drivers allocate a full 2Kbyte block, even if only 100 
> bytes are used in it. If we want to reduce "truesize", then 
> we perform a "copybreak", to allocate a right sized skb. Some 
> NICS do that for small frames.
> 
> If you have TCP performance problems, this might be because 
> of another high level problem. This truesize underestimation 
> is only hiding the problem, but makes the whole machine more 
> subject to OOM bugs.
> 
> Could you provide more information ?
> 
> 
> 
I'll explain further: The skb_in input of cdc_ncm_rx_fixup function holds the data of several ethernet packets received, as collected in one NCM packet. In CDC NCM driver cdc_ncm_rx_fixup function the skb_in is traversed and a new skb clone is created for each of the contained ethernet packets. Each of these clones are then modified by the len and data members to present only one ethernet packet each, but the truesize still is indicating the truesize of the parent skb which were holding the data of all clones. Each of the clones, but not the parent, will end up in the tcp_grow_window function now indicating the len of a single ethernet packet but a truesize of the collected ethernet packets. To the tcp_grow_window function of the TCP stack this seems as there is a huge overhead for each skb, as it analyzes the skb clones but not the parent skb when comparing the len to the truesize when determining how to increase rcv_ssthres. By limiting rcv_ssthres, also the the advertised TCP window size will be limited, leading to the TCP throughput performance being low.

The original reason for simply cloning the skb holding the several packets, instead of any "copybreak" would probably be to limit the need for memory operations on the received data. I can not tell if this was a good or not so good choise.

Drivers that allocate 2Kbyte blocks and then use only a portion of that does in fact introduce the indicated overhead. But the situation with the CDC NCM driver is that the skb clones are only seemingly introducing an overhead. In reality, as indicated by the further non-used skb parent, there is in fact no such overhead.

Yes, we do have TCP throughput performance problems. By comparing the handling of data received through other interfaces we could conclude that the performance issue was due to that tcp_grow_window function of the TCP stack did limit the rcv_ssthres in Check #2 because of skb->truesize was much larger than the skb->len, for the abocve described clones. 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html