From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Hutchings Subject: Re: [PATCH net 1/2] tcp: Limit number of segments generated by GSO per skb Date: Mon, 30 Jul 2012 20:35:52 +0100 Message-ID: <1343676952.2667.26.camel@bwh-desktop.uk.solarflarecom.com> References: <1343668498.2667.5.camel@bwh-desktop.uk.solarflarecom.com> <1343668602.2667.6.camel@bwh-desktop.uk.solarflarecom.com> <1343669507.21269.33.camel@edumazet-glaptop> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: David Miller , , To: Eric Dumazet Return-path: Received: from webmail.solarflare.com ([12.187.104.25]:47500 "EHLO ocex02.SolarFlarecom.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753487Ab2G3Tf6 (ORCPT ); Mon, 30 Jul 2012 15:35:58 -0400 In-Reply-To: <1343669507.21269.33.camel@edumazet-glaptop> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, 2012-07-30 at 19:31 +0200, Eric Dumazet wrote: > On Mon, 2012-07-30 at 18:16 +0100, Ben Hutchings wrote: > > A peer (or local user) may cause TCP to use a nominal MSS of as little > > as 88 (actual MSS of 76 with timestamps). Given that we have a > > sufficiently prodigious local sender and the peer ACKs quickly enough, > > it is nevertheless possible to grow the window for such a connection > > to the point that we will try to send just under 64K at once. This > > results in a single skb that expands to 861 segments. > > > > In some drivers with TSO support, such an skb will require hundreds of > > DMA descriptors; a substantial fraction of a TX ring or even more than > > a full ring. The TX queue selected for the skb may stall and trigger > > the TX watchdog repeatedly (since the problem skb will be retried > > after the TX reset). This particularly affects sfc, for which the > > issue is designated as CVE-2012-3412. However it may be that some > > hardware or firmware also fails to handle such an extreme TSO request > > correctly. > > > > Therefore, limit the number of segments per skb to 100. This should > > make no difference to behaviour unless the actual MSS is less than > > about 700. > > > > Signed-off-by: Ben Hutchings > > --- > > > Hmm, isnt GRO path also vulnerable ? You mean, for forwarding? If page fragments are used, the number of segments is limited to MAX_SKB_FRAGS < 100. But if skbs are aggregated and build_skb() is not used (e.g. due to jumbo MTU) it appears we would need an explicit limit. Something like this: --- From: Ben Hutchings Subject: [PATCH net] tcp: Limit number of segments merged by GRO In the case where GRO aggregates skbs that cannot be converted to page-fragments, there is currently no limit to the number of segments that may be merged and subsequently re-segmented by GSO. Apply the same limit as was introduced for locally-generated GSO skbs in 'tcp: Limit number of segments generated by GSO per skb'. Signed-off-by: Ben Hutchings --- net/ipv4/tcp.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 51d8daf..a052d07 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -3144,7 +3144,8 @@ out_check_final: TCP_FLAG_RST | TCP_FLAG_SYN | TCP_FLAG_FIN)); - if (p && (!NAPI_GRO_CB(skb)->same_flow || flush)) + if (p && (!NAPI_GRO_CB(skb)->same_flow || flush || + NAPI_GRO_CB(p)->count == TCP_MAX_GSO_SEGS)) pp = head; out: --- > An alternative would be to drop such frames in the ndo_start_xmit(), and > cap sk->sk_gso_max_size (since skb are no longer orphaned...) I have implemented that workaround for the out-of-tree version of sfc. For the in-tree driver, I thought it would be better to limit the number of segments at source, which will avoid penalising any cases where the window can grow so much larger than MSS. > Or you could introduce a new wk->sk_gso_max_segments, that your sfc > driver sets to whatever limit ? Yes, that's another option. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked.