From patchwork Thu Nov 3 22:24:27 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Soheil Hassas Yeganeh X-Patchwork-Id: 691059 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 3t8zzf2W5jz9snk for ; Fri, 4 Nov 2016 09:24:46 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b=kSHdc0bn; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932874AbcKCWYm (ORCPT ); Thu, 3 Nov 2016 18:24:42 -0400 Received: from mail-qt0-f196.google.com ([209.85.216.196]:36726 "EHLO mail-qt0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752404AbcKCWYl (ORCPT ); Thu, 3 Nov 2016 18:24:41 -0400 Received: by mail-qt0-f196.google.com with SMTP id n34so2210952qtb.3 for ; Thu, 03 Nov 2016 15:24:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id; bh=QkCRdwet1/5T1RKXtihBftPpY7bgg9roBTOQwVjqkhc=; b=kSHdc0bneQgxQQtG9mYcuBgNlhIpUOGRrHWAJCas04b7FmWY5LNax4rtSNIGHneQy2 q/76M/YQ/rh+BzvUNTVLp8xvj0AUyT9OsmTC6WvM+Vw1FTRF4EZOsNqFu5OhRj6/g4Nv rtzys/dR/0+wzEGcqraDsToK9mAJFpND5nYajWh7zPar6PHjTHw9Fpa9AXBtisxARhn6 9LmCLE5nyCMr9ENA2Gc9YIXqARbfH+o/jrSOqJAS+zvo74AdPBuF2ZZ2bxxLiPDwpS2x pwOlu7IyzqMVHSXERdEKXFF0WrZ9m9LNVa/M7T5bXx1FyvwEHQCaUp4q/gvOdaOJGzC3 tJ0g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=QkCRdwet1/5T1RKXtihBftPpY7bgg9roBTOQwVjqkhc=; b=FekZNh553CFcvsmPL8HenCdA1cV/v4qtZwcgULSHM8ehw0JRrPxuJIbQHBbA3J6ulE erFITogtUfAIJTZVz2qWjFJu51Z+6P6jLB5e3ghCviAg3V7HpUZwvrHZNsntHw6QiF7S v8n7bj4ShWP43Ic5hwxIPxLB5lDIw1R2f05UmOCEumCXU+N6ZUYqkY1SEqifS17feo89 Oc3qhZ50Kes3dMn5Ng+XKXLOfNDsokRxDzAY3tguLGbGqIfvBC9Tu2Q20EF5ofgYWFgV NP7iOE3jU0rTthQXTfMFK/lMwhz6nC/k1qiBWyUirGJwTOt+V39agxhjtI2lKvqs29sG sLJw== X-Gm-Message-State: ABUngvfwiDofNIFuSNSNOAkNt7pBuQ+sv7XSsDxXLu7AWbikjGxlzeyKjSvSCN3CGJKegw== X-Received: by 10.200.47.110 with SMTP id k43mr10039333qta.112.1478211880851; Thu, 03 Nov 2016 15:24:40 -0700 (PDT) Received: from soheil.nyc.corp.google.com ([100.101.230.57]) by smtp.gmail.com with ESMTPSA id r8sm5754309qtc.32.2016.11.03.15.24.40 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 03 Nov 2016 15:24:40 -0700 (PDT) From: Soheil Hassas Yeganeh To: davem@davemloft.net, netdev@vger.kernel.org Cc: edumazet@google.com, willemb@google.com, ncardwell@google.com, Soheil Hassas Yeganeh Subject: [PATCH net-next] sock: do not set sk_err in sock_dequeue_err_skb Date: Thu, 3 Nov 2016 18:24:27 -0400 Message-Id: <1478211867-24569-1-git-send-email-soheil.kdev@gmail.com> X-Mailer: git-send-email 2.8.0.rc3.226.g39d4020 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From: Soheil Hassas Yeganeh Do not set sk_err when dequeuing errors from the error queue. Doing so results in: a) Bugs: By overwriting existing sk_err values, it possibly hides legitimate errors. It is also incorrect when local errors are queued with ip_local_error. That happens in the context of a system call, which already returns the error code. b) Inconsistent behavior: When there are pending errors on the error queue, sk_err is sometimes 0 (e.g., for the first timestamp on the error queue) and sometimes set to an error code (after dequeuing the first timestamp). c) Suboptimality: Setting sk_err to ENOMSG on simple TX timestamps can abort parallel reads and writes. Removing this line doesn't break userspace. This is because userspace code cannot rely on sk_err for detecting whether there is something on the error queue. Except for ICMP messages received for UDP and RAW, sk_err is not set at enqueue time, and as a result sk_err can be 0 while there are plenty of errors on the error queue. For ICMP packets in UDP and RAW, sk_err is set when they are enqueued on the error queue, but that does not result in aborting reads and writes. For such cases, sk_err is only readable via getsockopt(SO_ERROR) which will reset the value of sk_err on its own. More importantly, prior to this patch, recvmsg(MSG_ERRQUEUE) has a race on setting sk_err (i.e., sk_err is set by sock_dequeue_err_skb without atomic ops or locks) which can store 0 in sk_err even when we have ICMP messages pending. Removing this line from sock_dequeue_err_skb eliminates that race. Signed-off-by: Soheil Hassas Yeganeh Signed-off-by: Eric Dumazet Signed-off-by: Willem de Bruijn Signed-off-by: Neal Cardwell Acked-by: Hannes Frederic Sowa --- net/core/skbuff.c | 1 - 1 file changed, 1 deletion(-) diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 1e3e008..0b2a6e9 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -3725,7 +3725,6 @@ struct sk_buff *sock_dequeue_err_skb(struct sock *sk) err = SKB_EXT_ERR(skb_next)->ee.ee_errno; spin_unlock_irqrestore(&q->lock, flags); - sk->sk_err = err; if (err) sk->sk_error_report(sk);