From patchwork Mon May 18 07:24:23 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Ilpo_J=C3=A4rvinen?= X-Patchwork-Id: 27341 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@bilbo.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from ozlabs.org (ozlabs.org [203.10.76.45]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "mx.ozlabs.org", Issuer "CA Cert Signing Authority" (verified OK)) by bilbo.ozlabs.org (Postfix) with ESMTPS id 15BA2B6F44 for ; Mon, 18 May 2009 17:24:32 +1000 (EST) Received: by ozlabs.org (Postfix) id 028DFDE076; Mon, 18 May 2009 17:24:32 +1000 (EST) Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by ozlabs.org (Postfix) with ESMTP id 5DBA6DE06E for ; Mon, 18 May 2009 17:24:31 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755904AbZERHYY (ORCPT ); Mon, 18 May 2009 03:24:24 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1755796AbZERHYX (ORCPT ); Mon, 18 May 2009 03:24:23 -0400 Received: from courier.cs.helsinki.fi ([128.214.9.1]:56910 "EHLO mail.cs.helsinki.fi" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755786AbZERHYW (ORCPT ); Mon, 18 May 2009 03:24:22 -0400 Received: from wrl-59.cs.helsinki.fi (wrl-59.cs.helsinki.fi [128.214.166.179]) (AUTH: PLAIN cs-relay, TLS: TLSv1/SSLv3,256bits,AES256-SHA) by mail.cs.helsinki.fi with esmtp; Mon, 18 May 2009 10:24:23 +0300 id 0008C198.4A110D27.00005CED Received: by wrl-59.cs.helsinki.fi (Postfix, from userid 50795) id 35BA5A0096; Mon, 18 May 2009 10:24:23 +0300 (EEST) Received: from localhost (localhost [127.0.0.1]) by wrl-59.cs.helsinki.fi (Postfix) with ESMTP id 32BBCA0091; Mon, 18 May 2009 10:24:23 +0300 (EEST) Date: Mon, 18 May 2009 10:24:23 +0300 (EEST) From: "=?ISO-8859-1?Q?Ilpo_J=E4rvinen?=" X-X-Sender: ijjarvin@wrl-59.cs.helsinki.fi To: David Miller cc: elendil@planet.nl, matthias.andree@gmx.de, Netdev Subject: Re: [PATCH v2] tcp: fix MSG_PEEK race check In-Reply-To: <20090517.154137.104422195.davem@davemloft.net> Message-ID: References: <200905092014.35642.elendil@planet.nl> <20090517.154137.104422195.davem@davemloft.net> MIME-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sun, 17 May 2009, David Miller wrote: > From: "Ilpo Järvinen" > Date: Mon, 11 May 2009 09:32:34 +0300 (EEST) > > > [PATCH v2] tcp: fix MSG_PEEK race check > > > > Commit 518a09ef11 (tcp: Fix recvmsg MSG_PEEK influence of > > blocking behavior) lets the loop run longer than the race check > > did previously expect, so we need to be more careful with this > > check and consider the work we have been doing. > > > > I tried my best to deal with urg hole madness too which happens > > here: > > if (!sock_flag(sk, SOCK_URGINLINE)) { > > ++*seq; > > ... > > by using additional offset by one but I certainly have very > > little interest in testing that part. > > > > Signed-off-by: Ilpo Järvinen > > Tested-by: Frans Pop > > Ok, now that I've looked at this, the urg_hole part of this change has > to be removed. Thanks for taking a look... :-) The first patch btw was with RFC and without urg bits btw but you put that into some discarded like(?) state in patchwork... :-/ > That case being accounted for with urg_hole is exactly what the > debugging message is trying to catch, where we are doing MSG_PEEK and > tcp_check_urg() advances ->copied_seq on us during one of those > "release_sock();/lock_sock();" sequences (which thus invoke TCP input > processing). Sorry, I'm not sure you thought this fully through here. What my patch with urg_hole does is that it keeps peek_seq non-advancing using the offsets. Now without the urg offsetting, the peek_seq side of the race check advances, which means that we didn't catch the race when it happened for real as copied_seq advanced too?!? From another perspective when the race didn't happen but potential for it existed, the current check should catch that since peek_seq advanced and copied_seq stayed where it was. But that certainly doesn't match to your description above. I wonder why all this fuzz about this particular race as we will do our best anyway to skip the hole on MSG_PEEK side (if I read the code right)? Either we never see the hole in MSG_PEEK side, or we're happily past it already, does it make any difference anymore in the latter case? . Not that I'm too interested in "improving" urg or so anywa, I'm just curious... :-) > Could you please respin this patch with the URG bits removed? Below. diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 1d7f49c..ccbd69b 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -1532,7 +1532,7 @@ do_prequeue: } } } - if ((flags & MSG_PEEK) && peek_seq != tp->copied_seq) { + if ((flags & MSG_PEEK) && (peek_seq - copied != tp->copied_seq)) { if (net_ratelimit()) printk(KERN_DEBUG "TCP(%s:%d): Application bug, race in MSG_PEEK.\n", current->comm, task_pid_nr(current));