From patchwork Tue Dec 7 22:03:55 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Eric Dumazet X-Patchwork-Id: 74605 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id CC271B70A9 for ; Wed, 8 Dec 2010 09:04:08 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753665Ab0LGWED (ORCPT ); Tue, 7 Dec 2010 17:04:03 -0500 Received: from mail-ww0-f44.google.com ([74.125.82.44]:33193 "EHLO mail-ww0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752761Ab0LGWEB (ORCPT ); Tue, 7 Dec 2010 17:04:01 -0500 Received: by wwa36 with SMTP id 36so438348wwa.1 for ; Tue, 07 Dec 2010 14:04:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:subject:from:to:cc :in-reply-to:references:content-type:date:message-id:mime-version :x-mailer:content-transfer-encoding; bh=d+qRfGpnZBPPLVj67xMpb06IFS9eNxni6cbo2fu/1i0=; b=QAgty0LJ+FONivSRU4z9UKTYcWYjQrpKcRSf3pUDHVIhaICzk25hQcE5iP5/DQGixg vGFZpROuKbVcrbBcqiLz798m6yO9srKBffU9RqcaNPiHfhM3YQS/6GKfuenVM2P2wXJY 8cD+l8m3a0qspkdIy+Ns224ZwFUkjY8ULqtM4= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=subject:from:to:cc:in-reply-to:references:content-type:date :message-id:mime-version:x-mailer:content-transfer-encoding; b=tG/JelZ1ieWsgfZtFQv5KjDfAAf8MzmQ4tF5Gbc94HikCRoUduCzAIEWFgodmYNweo RplRNxyYYFygmu9xfIDUhZ+dvLTxiFkCmPjsqg8kXclZYp4NCXPaEyisY6HcDvbHM6S1 OfVGLGAH/hX0wmUUN9P+b08AwiXZfY5yJ5GFY= Received: by 10.227.134.10 with SMTP id h10mr7981507wbt.200.1291759439372; Tue, 07 Dec 2010 14:03:59 -0800 (PST) Received: from [192.168.1.21] (13.237.66-86.rev.gaoland.net [86.66.237.13]) by mx.google.com with ESMTPS id j58sm3210867wes.21.2010.12.07.14.03.57 (version=SSLv3 cipher=RC4-MD5); Tue, 07 Dec 2010 14:03:58 -0800 (PST) Subject: Re: [PATCH] tcp: avoid a possible divide by zero From: Eric Dumazet To: Ben Hutchings Cc: David Miller , Martin Steigerwald , netdev In-Reply-To: <1291757563.21627.15.camel@bwh-desktop> References: <201012071639.58884.Martin@lichtvoll.de> <1291738321.2695.338.camel@edumazet-laptop> <1291755776.21627.13.camel@bwh-desktop> <1291757288.5324.18.camel@edumazet-laptop> <1291757563.21627.15.camel@bwh-desktop> Date: Tue, 07 Dec 2010 23:03:55 +0100 Message-ID: <1291759435.5324.25.camel@edumazet-laptop> Mime-Version: 1.0 X-Mailer: Evolution 2.30.3 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Le mardi 07 décembre 2010 à 21:32 +0000, Ben Hutchings a écrit : > On Tue, 2010-12-07 at 22:28 +0100, Eric Dumazet wrote: > [...] > > Thanks > > > > Great, I feel we are going to fix all sysctls, one by one then :( > > > > lkml removed from Cc > > > > > > [PATCH] tcp: avoid a possible divide by zero > > > > sysctl_tcp_tso_win_divisor might be set to zero while one cpu runs in > > tcp_tso_should_defer(). Make sure we dont allow a divide by zero by > > reading sysctl_tcp_tso_win_divisor once. > > > > Signed-off-by: Eric Dumazet > > --- > > net/ipv4/tcp_output.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > > index 05b1ecf..0281223 100644 > > --- a/net/ipv4/tcp_output.c > > +++ b/net/ipv4/tcp_output.c > > @@ -1513,6 +1513,7 @@ static int tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb) > > struct tcp_sock *tp = tcp_sk(sk); > > const struct inet_connection_sock *icsk = inet_csk(sk); > > u32 send_win, cong_win, limit, in_flight; > > + int win_divisor; > > > > if (TCP_SKB_CB(skb)->flags & TCPHDR_FIN) > > goto send_now; > > @@ -1544,13 +1545,14 @@ static int tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb) > > if ((skb != tcp_write_queue_tail(sk)) && (limit >= skb->len)) > > goto send_now; > > > > - if (sysctl_tcp_tso_win_divisor) { > > + win_divisor = sysctl_tcp_tso_win_divisor; > > You need to use ACCESS_ONCE(sysctl_tcp_tso_win_divisor). Otherwise the > compiler may eliminate the local variable and read the global twice. Yes, I knew that, of course :) I wonder how many bugs like that we have in sysctls Thanks [PATCH v2] tcp: avoid a possible divide by zero sysctl_tcp_tso_win_divisor might be set to zero while one cpu runs in tcp_tso_should_defer(). Make sure we dont allow a divide by zero by reading sysctl_tcp_tso_win_divisor exactly once. Signed-off-by: Eric Dumazet --- v2: Use ACCESS_ONCE() as Ben suggested net/ipv4/tcp_output.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 05b1ecf..0464d70 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -1513,6 +1513,7 @@ static int tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb) struct tcp_sock *tp = tcp_sk(sk); const struct inet_connection_sock *icsk = inet_csk(sk); u32 send_win, cong_win, limit, in_flight; + int win_divisor; if (TCP_SKB_CB(skb)->flags & TCPHDR_FIN) goto send_now; @@ -1544,13 +1545,14 @@ static int tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb) if ((skb != tcp_write_queue_tail(sk)) && (limit >= skb->len)) goto send_now; - if (sysctl_tcp_tso_win_divisor) { + win_divisor = ACCESS_ONCE(sysctl_tcp_tso_win_divisor); + if (win_divisor) { u32 chunk = min(tp->snd_wnd, tp->snd_cwnd * tp->mss_cache); /* If at least some fraction of a window is available, * just use it. */ - chunk /= sysctl_tcp_tso_win_divisor; + chunk /= win_divisor; if (limit >= chunk) goto send_now; } else {