From patchwork Thu Sep 17 03:41:34 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Herbert Xu X-Patchwork-Id: 518679 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 943531401AF for ; Thu, 17 Sep 2015 13:42:13 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752077AbbIQDlz (ORCPT ); Wed, 16 Sep 2015 23:41:55 -0400 Received: from helcar.hengli.com.au ([209.40.204.226]:58694 "EHLO helcar.hengli.com.au" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751496AbbIQDly (ORCPT ); Wed, 16 Sep 2015 23:41:54 -0400 Received: from gondolin.me.apana.org.au ([192.168.0.6]) by norbury.hengli.com.au with esmtp (Exim 4.80 #3 (Debian)) id 1ZcQ4t-0001jc-P9; Thu, 17 Sep 2015 13:41:43 +1000 Received: from herbert by gondolin.me.apana.org.au with local (Exim 4.80) (envelope-from ) id 1ZcQ4l-00052l-QT; Thu, 17 Sep 2015 11:41:35 +0800 Date: Thu, 17 Sep 2015 11:41:34 +0800 From: Herbert Xu To: Tejun Heo Cc: davem@davemloft.net, tom@herbertland.com, kafai@fb.com, kernel-team@fb.com, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, Linus Torvalds , Jiri Pirko , Cong Wang , Nicolas Dichtel , Thomas Graf , Scott Feldman Subject: Re: Possible netlink autobind regression Message-ID: <20150917034134.GA19327@gondor.apana.org.au> References: <20150917022909.GA22754@htj.duckdns.org> <20150917030845.GA19162@gondor.apana.org.au> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150917030845.GA19162@gondor.apana.org.au> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, Sep 17, 2015 at 11:08:45AM +0800, Herbert Xu wrote: > > Good catch! I think your explanation makes perfect sense. Linus > ran into this previously too after suspend-and-resume. Unfortunately you can't just postpone the setting of portid because once you pass it onto rhashtable the portid must never change while it's in custody. So what I've done is essentially revert my previous fix and instead add a new boolean "bound" to indicate whether the socket has been bound. ---8<--- netlink: Fix autobind race condition that leads to zero port ID The commit c0bb07df7d981e4091432754e30c9c720e2c0c78 ("netlink: Reset portid after netlink_insert failure") introduced a race condition where if two threads tried to autobind the same socket one of them may end up with a zero port ID. This patch reverts that commit and instead fixes it by introducing a separte "bound" variable to indicate whether a socket has been bound. Fixes: c0bb07df7d98 ("netlink: Reset portid after netlink_insert failure") Reported-by: Tejun Heo Reported-by: Linus Torvalds Signed-off-by: Herbert Xu Reviewed-by: Cong Wang diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 7f86d3b..abcbca5 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1095,7 +1095,7 @@ static int netlink_insert(struct sock *sk, u32 portid) lock_sock(sk); err = -EBUSY; - if (nlk_sk(sk)->portid) + if (nlk_sk(sk)->bound) goto err; err = -ENOMEM; @@ -1115,10 +1115,12 @@ static int netlink_insert(struct sock *sk, u32 portid) err = -EOVERFLOW; if (err == -EEXIST) err = -EADDRINUSE; - nlk_sk(sk)->portid = 0; sock_put(sk); + goto err; } + nlk_sk(sk)->bound = true; + err: release_sock(sk); return err; @@ -1285,7 +1287,7 @@ static int netlink_release(struct socket *sock) skb_queue_purge(&sk->sk_write_queue); - if (nlk->portid) { + if (nlk->bound) { struct netlink_notify n = { .net = sock_net(sk), .protocol = sk->sk_protocol, @@ -1519,7 +1521,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, return err; } - if (nlk->portid) + if (nlk->bound) if (nladdr->nl_pid != nlk->portid) return -EINVAL; @@ -1537,7 +1539,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, } } - if (!nlk->portid) { + if (!nlk->bound) { err = nladdr->nl_pid ? netlink_insert(sk, nladdr->nl_pid) : netlink_autobind(sock); @@ -1585,7 +1587,7 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr, !netlink_allowed(sock, NL_CFG_F_NONROOT_SEND)) return -EPERM; - if (!nlk->portid) + if (!nlk->bound) err = netlink_autobind(sock); if (err == 0) { @@ -2426,7 +2428,7 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len) dst_group = nlk->dst_group; } - if (!nlk->portid) { + if (!nlk->bound) { err = netlink_autobind(sock); if (err) goto out; diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h index 8900840..e6aae40 100644 --- a/net/netlink/af_netlink.h +++ b/net/netlink/af_netlink.h @@ -35,6 +35,7 @@ struct netlink_sock { unsigned long state; size_t max_recvmsg_len; wait_queue_head_t wait; + bool bound; bool cb_running; struct netlink_callback cb; struct mutex *cb_mutex;