From patchwork Fri Sep 25 03:39:57 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Herbert Xu X-Patchwork-Id: 522628 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 A9CA9140784 for ; Fri, 25 Sep 2015 13:40:50 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754486AbbIYDkd (ORCPT ); Thu, 24 Sep 2015 23:40:33 -0400 Received: from helcar.hengli.com.au ([209.40.204.226]:60327 "EHLO helcar.hengli.com.au" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753503AbbIYDkb (ORCPT ); Thu, 24 Sep 2015 23:40:31 -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 1ZfJrm-00079m-Lv; Fri, 25 Sep 2015 13:40:10 +1000 Received: from herbert by gondolin.me.apana.org.au with local (Exim 4.80) (envelope-from ) id 1ZfJra-0001EC-Vk; Fri, 25 Sep 2015 11:39:58 +0800 Date: Fri, 25 Sep 2015 11:39:57 +0800 From: Herbert Xu To: Linus Torvalds Cc: Tejun Heo , David Miller , Cong Wang , Tom Herbert , kafai@fb.com, kernel-team@fb.com, Linux Kernel Mailing List , Network Development , =?utf-8?B?SmnFmcOtIFDDrXJrbw==?= , Nicolas Dichtel , Thomas Graf , Scott Feldman Subject: Re: netlink: Add barrier to netlink_connect for theoretical case Message-ID: <20150925033957.GA4675@gondor.apana.org.au> References: <20150921133415.GA1740@gondor.apana.org.au> <20150921182022.GB13263@mtj.duckdns.org> <20150922033856.GA7851@gondor.apana.org.au> <20150924.121142.870602292135442487.davem@davemloft.net> <20150924200510.GE25415@mtj.duckdns.org> <20150925014327.GA3725@gondor.apana.org.au> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: 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 24, 2015 at 08:24:56PM -0700, Linus Torvalds wrote: > > The above looks very suspicious. You're right Linus. I've added the READ_ONCE there. The reason I kept the conditional is because the helper is always called in a context where the result is used as part of an if statement. The assembly actually looks sane, e.g., for netlink_bind: 3a06: 41 0f b6 94 24 e8 02 movzbl 0x2e8(%r12),%edx 3a0d: 00 00 3a0f: 84 d2 test %dl,%dl 3a11: 0f 85 97 00 00 00 jne 3aae 3a17: 49 83 bc 24 98 03 00 cmpq $0x0,0x398(%r12) 3a1e: 00 00 ... 3aae: 41 8b 84 24 a0 02 00 mov 0x2a0(%r12),%eax 3ab5: 00 3ab6: 41 39 46 04 cmp %eax,0x4(%r14) 3aba: 0f 84 57 ff ff ff je 3a17 3ac0: b8 ea ff ff ff mov $0xffffffea,%eax 3ac5: eb d1 jmp 3a98 So there is no unnecessary jumping around. I checked the other two call sites and they look the same. I'm on a fairly old compiler though (4.7.2) so it is possible that newer gcc's may do silly things. Thanks, ---8<--- If a netlink_connect call is followed by a netlink_getname call the portid returned may not be up-to-date. This patch adds a barrier for that case. As all nlk->bound dereferences now have barriers this patch also adds a netlink_bound helper to encapsulate the barrier, as was suggested by Tejun. Also as suggested by Linus, the lockless read of nlk->bound is now protected with READ_ONCE to ensure that the compiler doesn't do double reads that may screw up our use of the barrier. Fixes: da314c9923fe ("netlink: Replace rhash_portid with bound") Reported-by: Tejun Heo Signed-off-by: Herbert Xu diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 2c15fae..dd0a294 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -125,6 +125,17 @@ static inline u32 netlink_group_mask(u32 group) return group ? 1 << (group - 1) : 0; } +static inline bool netlink_bound(struct netlink_sock *nlk) +{ + bool bound = READ_ONCE(nlk->bound); + + /* Ensure nlk is hashed and visible. */ + if (bound) + smp_rmb(); + + return bound; +} + int netlink_add_tap(struct netlink_tap *nt) { if (unlikely(nt->dev->type != ARPHRD_NETLINK)) @@ -1524,14 +1535,10 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, return err; } - bound = nlk->bound; - if (bound) { - /* Ensure nlk->portid is up-to-date. */ - smp_rmb(); - + bound = netlink_bound(nlk); + if (bound) if (nladdr->nl_pid != nlk->portid) return -EINVAL; - } if (nlk->netlink_bind && groups) { int group; @@ -1547,9 +1554,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, } } - /* No need for barriers here as we return to user-space without - * using any of the bound attributes. - */ if (!bound) { err = nladdr->nl_pid ? netlink_insert(sk, nladdr->nl_pid) : @@ -1598,10 +1602,7 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr, !netlink_allowed(sock, NL_CFG_F_NONROOT_SEND)) return -EPERM; - /* No need for barriers here as we return to user-space without - * using any of the bound attributes. - */ - if (!nlk->bound) + if (!netlink_bound(nlk)) err = netlink_autobind(sock); if (err == 0) { @@ -2442,13 +2443,10 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len) dst_group = nlk->dst_group; } - if (!nlk->bound) { + if (!netlink_bound(nlk)) { err = netlink_autobind(sock); if (err) goto out; - } else { - /* Ensure nlk is hashed and visible. */ - smp_rmb(); } /* It's a really convoluted way for userland to ask for mmaped