| Message ID | 20140902082929.GA8483@kria |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Di, 2014-09-02 at 10:29 +0200, Sabrina Dubroca wrote: > Calling setsockopt with IPV6_JOIN_ANYCAST or IPV6_LEAVE_ANYCAST > triggers the assertion in addrconf_join_solict()/addrconf_leave_solict() > > ipv6_sock_ac_join(), ipv6_sock_ac_drop(), ipv6_sock_ac_close() need to > take RTNL before calling ipv6_dev_ac_inc/dec. Same thing with > ipv6_sock_mc_join(), ipv6_sock_mc_drop(), ipv6_sock_mc_close() before > calling ipv6_dev_mc_inc/dec. > > This patch moves ASSERT_RTNL() up a level in the call stack. > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> > Reported-by: Tommi Rantala <tt.rantala@gmail.com> > --- > As was said earlier, this should go in stable. > > v2: > - based on net > - keep dev_get_by_flags_rcu and RCU in ipv6_sock_ac_* > - remove two ASSERT_RTNL() that are not necessary > > Thank you for your help, Hannes! Thanks for fixing! ;) Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org> -- 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
On Tue, Sep 2, 2014 at 1:29 AM, Sabrina Dubroca <sd@queasysnail.net> wrote: > > v2: > - based on net > - keep dev_get_by_flags_rcu and RCU in ipv6_sock_ac_* > - remove two ASSERT_RTNL() that are not necessary There is no point to keep RCU here. Hannes' reply doesn't make any sense. -- 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
From: Sabrina Dubroca <sd@queasysnail.net> Date: Tue, 2 Sep 2014 10:29:29 +0200 > Calling setsockopt with IPV6_JOIN_ANYCAST or IPV6_LEAVE_ANYCAST > triggers the assertion in addrconf_join_solict()/addrconf_leave_solict() > > ipv6_sock_ac_join(), ipv6_sock_ac_drop(), ipv6_sock_ac_close() need to > take RTNL before calling ipv6_dev_ac_inc/dec. Same thing with > ipv6_sock_mc_join(), ipv6_sock_mc_drop(), ipv6_sock_mc_close() before > calling ipv6_dev_mc_inc/dec. > > This patch moves ASSERT_RTNL() up a level in the call stack. > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> > Reported-by: Tommi Rantala <tt.rantala@gmail.com> Applied and queued up for -stable, thanks. -- 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
On Fri, Sep 5, 2014 at 11:53 AM, David Miller <davem@davemloft.net> wrote: > From: Sabrina Dubroca <sd@queasysnail.net> > Date: Tue, 2 Sep 2014 10:29:29 +0200 > >> Calling setsockopt with IPV6_JOIN_ANYCAST or IPV6_LEAVE_ANYCAST >> triggers the assertion in addrconf_join_solict()/addrconf_leave_solict() >> >> ipv6_sock_ac_join(), ipv6_sock_ac_drop(), ipv6_sock_ac_close() need to >> take RTNL before calling ipv6_dev_ac_inc/dec. Same thing with >> ipv6_sock_mc_join(), ipv6_sock_mc_drop(), ipv6_sock_mc_close() before >> calling ipv6_dev_mc_inc/dec. >> >> This patch moves ASSERT_RTNL() up a level in the call stack. >> >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> >> Reported-by: Tommi Rantala <tt.rantala@gmail.com> > > Applied and queued up for -stable, thanks. I believe you applied a wrong version, at least the following is not correct: + if (!dev) + return -ENODEV; Sabrina took that from my draft patch, but they all don't realize this is wrong. (I did provide a correct version which is just ignored by you.) -- 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
On Fr, 2014-09-05 at 11:58 -0700, Cong Wang wrote: > On Fri, Sep 5, 2014 at 11:53 AM, David Miller <davem@davemloft.net> wrote: > > From: Sabrina Dubroca <sd@queasysnail.net> > > Date: Tue, 2 Sep 2014 10:29:29 +0200 > > > >> Calling setsockopt with IPV6_JOIN_ANYCAST or IPV6_LEAVE_ANYCAST > >> triggers the assertion in addrconf_join_solict()/addrconf_leave_solict() > >> > >> ipv6_sock_ac_join(), ipv6_sock_ac_drop(), ipv6_sock_ac_close() need to > >> take RTNL before calling ipv6_dev_ac_inc/dec. Same thing with > >> ipv6_sock_mc_join(), ipv6_sock_mc_drop(), ipv6_sock_mc_close() before > >> calling ipv6_dev_mc_inc/dec. > >> > >> This patch moves ASSERT_RTNL() up a level in the call stack. > >> > >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > >> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> > >> Reported-by: Tommi Rantala <tt.rantala@gmail.com> > > > > Applied and queued up for -stable, thanks. > > I believe you applied a wrong version, at least the following > is not correct: > > + if (!dev) > + return -ENODEV; > > Sabrina took that from my draft patch, but they all don't > realize this is wrong. > > (I did provide a correct version which is just ignored by you.) What games are you playing? You know how patches are processed by David and I even let him the choice by pointing out a problem in your patch so that you could an update and send v2. I really feel miffed about your behavior! Anyway, I saw the hunk adding the return -ENODEV and didn't see any problems with it. Sure it might be better if it would gone into a separate patch. Can you elaborate what problems you see? Thanks, Hannes -- 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
From: Cong Wang <cwang@twopensource.com> Date: Fri, 5 Sep 2014 11:58:31 -0700 > On Fri, Sep 5, 2014 at 11:53 AM, David Miller <davem@davemloft.net> wrote: >> From: Sabrina Dubroca <sd@queasysnail.net> >> Date: Tue, 2 Sep 2014 10:29:29 +0200 >> >>> Calling setsockopt with IPV6_JOIN_ANYCAST or IPV6_LEAVE_ANYCAST >>> triggers the assertion in addrconf_join_solict()/addrconf_leave_solict() >>> >>> ipv6_sock_ac_join(), ipv6_sock_ac_drop(), ipv6_sock_ac_close() need to >>> take RTNL before calling ipv6_dev_ac_inc/dec. Same thing with >>> ipv6_sock_mc_join(), ipv6_sock_mc_drop(), ipv6_sock_mc_close() before >>> calling ipv6_dev_mc_inc/dec. >>> >>> This patch moves ASSERT_RTNL() up a level in the call stack. >>> >>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >>> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> >>> Reported-by: Tommi Rantala <tt.rantala@gmail.com> >> >> Applied and queued up for -stable, thanks. > > I believe you applied a wrong version, at least the following > is not correct: > > + if (!dev) > + return -ENODEV; > > Sabrina took that from my draft patch, but they all don't > realize this is wrong. > > (I did provide a correct version which is just ignored by you.) Not ignored, but rather it was hard to interpret the situation due to poor communication in the feedback emails. The onus is on you guys to communicate things precisely so that I understand what patch is in what state. -- 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
On Fri, Sep 5, 2014 at 12:12 PM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > What games are you playing? You know how patches are processed by David > and I even let him the choice by pointing out a problem in your patch so > that you could an update and send v2. I assume David goes over all the discussion before applying any patch. So in the original discussion, obviously I disagree with your point on RCU, and sent my own version. I do understand David may miss something due to massive emails in this list, but you make no point here nor I understand your broken English "let him the choice...". > > I really feel miffed about your behavior! > I SEE. Please do redirect my email to your /dev/null. Thanks a lot! > Anyway, I saw the hunk adding the return -ENODEV and didn't see any > problems with it. Sure it might be better if it would gone into a > separate patch. Can you elaborate what problems you see? > Yes, it should be not in this patch at the very very least. -- 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
From: Cong Wang <cwang@twopensource.com> Date: Fri, 5 Sep 2014 12:23:37 -0700 > On Fri, Sep 5, 2014 at 12:12 PM, Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: >> >> What games are you playing? You know how patches are processed by David >> and I even let him the choice by pointing out a problem in your patch so >> that you could an update and send v2. > > I assume David goes over all the discussion before applying any patch. > So in the original discussion, obviously I disagree with your point on RCU, > and sent my own version. I thought the retention of RCU locking was reasonable, and that your feedback was something I disagreed with. This is different from ignoring your feedback. -- 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
On Fri, Sep 5, 2014 at 12:25 PM, David Miller <davem@davemloft.net> wrote: > > I thought the retention of RCU locking was reasonable, and that your > feedback was something I disagreed with. > > This is different from ignoring your feedback. The point is we don't need to backport the patch that far, as I already mentioned, up to commit c15b1ccadb323ea ("ipv6: move DAD and addrconf_verify processing to workqueue"), simply because no one reports any bug. So we don't have to adjust the patch just for stable in this case. -- 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/ipv6/addrconf.c b/net/ipv6/addrconf.c index 0b239fc1816e..aa0e135b808c 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -1690,14 +1690,12 @@ void addrconf_dad_failure(struct inet6_ifaddr *ifp) addrconf_mod_dad_work(ifp, 0); } -/* Join to solicited addr multicast group. */ - +/* Join to solicited addr multicast group. + * caller must hold RTNL */ void addrconf_join_solict(struct net_device *dev, const struct in6_addr *addr) { struct in6_addr maddr; - ASSERT_RTNL(); - if (dev->flags&(IFF_LOOPBACK|IFF_NOARP)) return; @@ -1705,12 +1703,11 @@ void addrconf_join_solict(struct net_device *dev, const struct in6_addr *addr) ipv6_dev_mc_inc(dev, &maddr); } +/* caller must hold RTNL */ void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr) { struct in6_addr maddr; - ASSERT_RTNL(); - if (idev->dev->flags&(IFF_LOOPBACK|IFF_NOARP)) return; @@ -1718,12 +1715,11 @@ void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr) __ipv6_dev_mc_dec(idev, &maddr); } +/* caller must hold RTNL */ static void addrconf_join_anycast(struct inet6_ifaddr *ifp) { struct in6_addr addr; - ASSERT_RTNL(); - if (ifp->prefix_len >= 127) /* RFC 6164 */ return; ipv6_addr_prefix(&addr, &ifp->addr, ifp->prefix_len); @@ -1732,12 +1728,11 @@ static void addrconf_join_anycast(struct inet6_ifaddr *ifp) ipv6_dev_ac_inc(ifp->idev->dev, &addr); } +/* caller must hold RTNL */ static void addrconf_leave_anycast(struct inet6_ifaddr *ifp) { struct in6_addr addr; - ASSERT_RTNL(); - if (ifp->prefix_len >= 127) /* RFC 6164 */ return; ipv6_addr_prefix(&addr, &ifp->addr, ifp->prefix_len); diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c index 210183244689..45b9d81d91e8 100644 --- a/net/ipv6/anycast.c +++ b/net/ipv6/anycast.c @@ -77,6 +77,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr) pac->acl_next = NULL; pac->acl_addr = *addr; + rtnl_lock(); rcu_read_lock(); if (ifindex == 0) { struct rt6_info *rt; @@ -137,6 +138,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr) error: rcu_read_unlock(); + rtnl_unlock(); if (pac) sock_kfree_s(sk, pac, sizeof(*pac)); return err; @@ -171,13 +173,17 @@ int ipv6_sock_ac_drop(struct sock *sk, int ifindex, const struct in6_addr *addr) spin_unlock_bh(&ipv6_sk_ac_lock); + rtnl_lock(); rcu_read_lock(); dev = dev_get_by_index_rcu(net, pac->acl_ifindex); if (dev) ipv6_dev_ac_dec(dev, &pac->acl_addr); rcu_read_unlock(); + rtnl_unlock(); sock_kfree_s(sk, pac, sizeof(*pac)); + if (!dev) + return -ENODEV; return 0; } @@ -198,6 +204,7 @@ void ipv6_sock_ac_close(struct sock *sk) spin_unlock_bh(&ipv6_sk_ac_lock); prev_index = 0; + rtnl_lock(); rcu_read_lock(); while (pac) { struct ipv6_ac_socklist *next = pac->acl_next; @@ -212,6 +219,7 @@ void ipv6_sock_ac_close(struct sock *sk) pac = next; } rcu_read_unlock(); + rtnl_unlock(); } static void aca_put(struct ifacaddr6 *ac) @@ -233,6 +241,8 @@ int ipv6_dev_ac_inc(struct net_device *dev, const struct in6_addr *addr) struct rt6_info *rt; int err; + ASSERT_RTNL(); + idev = in6_dev_get(dev); if (idev == NULL) @@ -302,6 +312,8 @@ int __ipv6_dev_ac_dec(struct inet6_dev *idev, const struct in6_addr *addr) { struct ifacaddr6 *aca, *prev_aca; + ASSERT_RTNL(); + write_lock_bh(&idev->lock); prev_aca = NULL; for (aca = idev->ac_list; aca; aca = aca->aca_next) { diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c index 617f0958e164..a23b655a7627 100644 --- a/net/ipv6/mcast.c +++ b/net/ipv6/mcast.c @@ -172,6 +172,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr) mc_lst->next = NULL; mc_lst->addr = *addr; + rtnl_lock(); rcu_read_lock(); if (ifindex == 0) { struct rt6_info *rt; @@ -185,6 +186,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr) if (dev == NULL) { rcu_read_unlock(); + rtnl_unlock(); sock_kfree_s(sk, mc_lst, sizeof(*mc_lst)); return -ENODEV; } @@ -202,6 +204,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr) if (err) { rcu_read_unlock(); + rtnl_unlock(); sock_kfree_s(sk, mc_lst, sizeof(*mc_lst)); return err; } @@ -212,6 +215,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr) spin_unlock(&ipv6_sk_mc_lock); rcu_read_unlock(); + rtnl_unlock(); return 0; } @@ -229,6 +233,7 @@ int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr) if (!ipv6_addr_is_multicast(addr)) return -EINVAL; + rtnl_lock(); spin_lock(&ipv6_sk_mc_lock); for (lnk = &np->ipv6_mc_list; (mc_lst = rcu_dereference_protected(*lnk, @@ -252,12 +257,15 @@ int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr) } else (void) ip6_mc_leave_src(sk, mc_lst, NULL); rcu_read_unlock(); + rtnl_unlock(); + atomic_sub(sizeof(*mc_lst), &sk->sk_omem_alloc); kfree_rcu(mc_lst, rcu); return 0; } } spin_unlock(&ipv6_sk_mc_lock); + rtnl_unlock(); return -EADDRNOTAVAIL; } @@ -302,6 +310,7 @@ void ipv6_sock_mc_close(struct sock *sk) if (!rcu_access_pointer(np->ipv6_mc_list)) return; + rtnl_lock(); spin_lock(&ipv6_sk_mc_lock); while ((mc_lst = rcu_dereference_protected(np->ipv6_mc_list, lockdep_is_held(&ipv6_sk_mc_lock))) != NULL) { @@ -328,6 +337,7 @@ void ipv6_sock_mc_close(struct sock *sk) spin_lock(&ipv6_sk_mc_lock); } spin_unlock(&ipv6_sk_mc_lock); + rtnl_unlock(); } int ip6_mc_source(int add, int omode, struct sock *sk, @@ -845,6 +855,8 @@ int ipv6_dev_mc_inc(struct net_device *dev, const struct in6_addr *addr) struct ifmcaddr6 *mc; struct inet6_dev *idev; + ASSERT_RTNL(); + /* we need to take a reference on idev */ idev = in6_dev_get(dev); @@ -916,6 +928,8 @@ int __ipv6_dev_mc_dec(struct inet6_dev *idev, const struct in6_addr *addr) { struct ifmcaddr6 *ma, **map; + ASSERT_RTNL(); + write_lock_bh(&idev->lock); for (map = &idev->mc_list; (ma=*map) != NULL; map = &ma->next) { if (ipv6_addr_equal(&ma->mca_addr, addr)) {