ipv6: Drop RTNL from mcast.c and anycast.c
Subsystems (3)
v3 cover ipv6: Drop RTNL from mcast.c and anycast.c
From: Kuniyuki Iwashima <[email protected]>
This is a prep series for RCU conversion of RTM_NEWNEIGH, which needs RTNL during neigh_table.{pconstructor,pdestructor}() touching IPv6 multicast code.
Currently, IPv6 multicast code is protected by lock_sock() and inet6_dev->mc_lock, and RTNL is not actually needed.
In addition, anycast code is also in the same situation and does not need RTNL at all.
This series removes RTNL from net/ipv6/{mcast.c,anycast.c} and finally removes setsockopt_needs_rtnl() from do_ipv6_setsockopt().
Changes:
v3:
* Add Eric's tag for Patch 2 ~ 15
* Patch 1: Return the retval of ipv6_dev_mc_inc()
* Patch 6, 9, 14: Use dst_dev() for rt->dst.dev
* Patch 13: Use READ_ONCE() for net->ipv6.devconf_all->forwarding and idev->conf.forwarding
v2: https://lore.kernel.org/netdev/[email protected]/
* Patch 2: Clarify which function doesn't need assertion
* Patch 6, 9, 14: Call rt6_lookup() and dev_hold() under RCU
v1: https://lore.kernel.org/netdev/[email protected]/
Kuniyuki Iwashima (15):
ipv6: ndisc: Remove __in6_dev_get() in
pndisc_{constructor,destructor}().
ipv6: mcast: Replace locking comments with lockdep annotations.
ipv6: mcast: Check inet6_dev->dead under idev->mc_lock in
__ipv6_dev_mc_inc().
ipv6: mcast: Remove mca_get().
ipv6: mcast: Use in6_dev_get() in ipv6_dev_mc_dec().
ipv6: mcast: Don't hold RTNL for IPV6_ADD_MEMBERSHIP and
MCAST_JOIN_GROUP.
ipv6: mcast: Don't hold RTNL for IPV6_DROP_MEMBERSHIP and
MCAST_LEAVE_GROUP.
ipv6: mcast: Don't hold RTNL in ipv6_sock_mc_close().
ipv6: mcast: Don't hold RTNL for MCAST_ socket options.
ipv6: mcast: Remove unnecessary ASSERT_RTNL and comment.
ipv6: anycast: Don't use rtnl_dereference().
ipv6: anycast: Don't hold RTNL for IPV6_LEAVE_ANYCAST and
IPV6_ADDRFORM.
ipv6: anycast: Unify two error paths in ipv6_sock_ac_join().
ipv6: anycast: Don't hold RTNL for IPV6_JOIN_ANYCAST.
ipv6: Remove setsockopt_needs_rtnl().
include/linux/netdevice.h | 4 +- net/core/dev.c | 38 +++-- net/ipv6/addrconf.c | 12 +- net/ipv6/anycast.c | 100 +++++++----- net/ipv6/ipv6_sockglue.c | 28 +--- net/ipv6/mcast.c | 332 +++++++++++++++++++------------------- net/ipv6/ndisc.c | 13 +- 7 files changed, 260 insertions(+), 267 deletions(-)
Signature · 1 line
2.49.0
PATCH v3 1/15 ipv6: ndisc: Remove __in6_dev_get() in pndisc_{constructor,destructor}().
From: Kuniyuki Iwashima <[email protected]>
ipv6_dev_mc_{inc,dec}() has the same check.
Let's remove __in6_dev_get() from pndisc_constructor() and pndisc_destructor().
---
v3: Return the retval of ipv6_dev_mc_inc()
--- net/ipv6/ndisc.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
net/ipv6/ndisc.c +7 -6
@@ -377,24 +377,25 @@ static int ndisc_constructor(struct neighbour *neigh)static int pndisc_constructor (struct pneigh_entry *n) {struct in6_addr *addr = (struct in6_addr *)&n->key; -struct in6_addr maddr;struct net_device *dev = n->dev; +struct in6_addr maddr; -if (!dev || !__in6_dev_get (dev)) +if (!dev)return -EINVAL ; +addrconf_addr_solict_mult (addr, &maddr); -ipv6_dev_mc_inc (dev, &maddr); -return 0 ; +return ipv6_dev_mc_inc (dev, &maddr); }static void pndisc_destructor (struct pneigh_entry *n) {struct in6_addr *addr = (struct in6_addr *)&n->key; -struct in6_addr maddr;struct net_device *dev = n->dev; +struct in6_addr maddr; -if (!dev || !__in6_dev_get (dev)) +if (!dev)return ; +addrconf_addr_solict_mult (addr, &maddr);ipv6_dev_mc_dec (dev, &maddr); }
Signature · 1 line
2.49.0
Re: [PATCH v3 net-next 01/15] ipv6: ndisc: Remove __in6_dev_get() in pndisc_{constructor,destructor}().
Quoted 9 lines · Kuniyuki Iwashima
From: Kuniyuki Iwashima <[email protected]>
ipv6_dev_mc_{inc,dec}() has the same check.
Let's remove
__in6_dev_get()frompndisc_constructor()andpndisc_destructor().Signed-off-by Kuniyuki Iwashima <[email protected]>
PATCH v3 2/15 ipv6: mcast: Replace locking comments with lockdep annotations.
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
Commit 63ed8de4be81 ("mld: add mc_lock for protecting per-interface mld data") added the same comments regarding locking to many functions.
Let's replace the comments with lockdep annotation, which is more helpful.
Note that we just remove the comment for mld_clear_zeros() and mld_send_cr(), where mc_dereference() is used in the entry of the function.
While at it, a comment for __ipv6_sock_mc_join() is moved back to the correct place.
--- net/ipv6/mcast.c | 125 +++++++++++++++++++++++++++-------------------- 1 file changed, 71 insertions(+), 54 deletions(-)
net/ipv6/mcast.c +71 -54
@@ -108,9 +108,9 @@ static int __ipv6_dev_mc_inc(struct net_device *dev,int sysctl_mld_max_msf__read_mostly =IPV6_MLD_MAX_MSF ;int sysctl_mld_qrv__read_mostly =MLD_QRV_DEFAULT ; -/* -* socket join on multicast group -*/ +#define mc_assert_locked (idev) \ + lockdep_assert_held(&(idev)->mc_lock) +#define mc_dereference (e, idev) \ rcu_dereference_protected(e, lockdep_is_held(&(idev)->mc_lock))
@@ -169,6 +169,9 @@ static int unsolicited_report_interval(struct inet6_dev *idev)return iv >0 ? iv :1 ; } +/* +* socket join on multicast group +*/ static int __ipv6_sock_mc_join (struct sock *sk,int ifindex,const struct in6_addr *addr,unsigned int mode) {
@@ -668,12 +671,13 @@ bool inet6_mc_check(const struct sock *sk, const struct in6_addr *mc_addr,return rv; } -/* called with mc_lock */ static void igmp6_group_added (struct ifmcaddr6 *mc) {struct net_device *dev = mc->idev->dev;char buf[MAX_ADDR_LEN ]; +mc_assert_locked (mc->idev); +if (IPV6_ADDR_MC_SCOPE (&mc->mca_addr) <IPV6_ADDR_SCOPE_LINKLOCAL )return ;
@@ -703,12 +707,13 @@ static void igmp6_group_added(struct ifmcaddr6 *mc)mld_ifc_event (mc->idev); } -/* called with mc_lock */ static void igmp6_group_dropped (struct ifmcaddr6 *mc) {struct net_device *dev = mc->idev->dev;char buf[MAX_ADDR_LEN ]; +mc_assert_locked (mc->idev); +if (IPV6_ADDR_MC_SCOPE (&mc->mca_addr) <IPV6_ADDR_SCOPE_LINKLOCAL )return ;
@@ -729,14 +734,13 @@ static void igmp6_group_dropped(struct ifmcaddr6 *mc)refcount_dec (&mc->mca_refcnt); } -/* -* deleted ifmcaddr6 manipulation -* called with mc_lock -*/ +/* deleted ifmcaddr6 manipulation */ static void mld_add_delrec (struct inet6_dev *idev,struct ifmcaddr6 *im) {struct ifmcaddr6 *pmc; +mc_assert_locked (idev); +/* this is an "ifmcaddr6" for convenience; only the fields below * are actually used. In particular, the refcnt and users are not * used for management of the delete list. Using the same structure
@@ -770,13 +774,14 @@ static void mld_add_delrec(struct inet6_dev *idev, struct ifmcaddr6 *im)rcu_assign_pointer (idev->mc_tomb, pmc); } -/* called with mc_lock */ static void mld_del_delrec (struct inet6_dev *idev,struct ifmcaddr6 *im) {struct ip6_sf_list *psf, *sources, *tomb;struct in6_addr *pmca = &im->mca_addr;struct ifmcaddr6 *pmc, *pmc_prev; +mc_assert_locked (idev); + pmc_prev =NULL ;for_each_mc_tomb (idev, pmc) {if (ipv6_addr_equal (&pmc->mca_addr, pmca))
@@ -813,11 +818,12 @@ static void mld_del_delrec(struct inet6_dev *idev, struct ifmcaddr6 *im) } } -/* called with mc_lock */ static void mld_clear_delrec (struct inet6_dev *idev) {struct ifmcaddr6 *pmc, *nextpmc; +mc_assert_locked (idev); + pmc =mc_dereference (idev->mc_tomb, idev);RCU_INIT_POINTER (idev->mc_tomb,NULL );
@@ -874,13 +880,14 @@ static void ma_put(struct ifmcaddr6 *mc) } } -/* called with mc_lock */ static struct ifmcaddr6 *mca_alloc (struct inet6_dev *idev,const struct in6_addr *addr,unsigned int mode) {struct ifmcaddr6 *mc; +mc_assert_locked (idev); + mc =kzalloc (sizeof (*mc),GFP_KERNEL );if (!mc)return NULL ;
@@ -1091,46 +1098,51 @@ bool ipv6_chk_mcast_addr(struct net_device *dev, const struct in6_addr *group,return rv; } -/* called with mc_lock */ static void mld_gq_start_work (struct inet6_dev *idev) {unsigned long tv =get_random_u32_below (idev->mc_maxdelay); +mc_assert_locked (idev); + idev->mc_gq_running =1 ;if (!mod_delayed_work (mld_wq, &idev->mc_gq_work, tv +2 ))in6_dev_hold (idev); } -/* called with mc_lock */ static void mld_gq_stop_work (struct inet6_dev *idev) { +mc_assert_locked (idev); + idev->mc_gq_running =0 ;if (cancel_delayed_work (&idev->mc_gq_work))__in6_dev_put (idev); } -/* called with mc_lock */ static void mld_ifc_start_work (struct inet6_dev *idev,unsigned long delay) {unsigned long tv =get_random_u32_below (delay); +mc_assert_locked (idev); +if (!mod_delayed_work (mld_wq, &idev->mc_ifc_work, tv +2 ))in6_dev_hold (idev); } -/* called with mc_lock */ static void mld_ifc_stop_work (struct inet6_dev *idev) { +mc_assert_locked (idev); + idev->mc_ifc_count =0 ;if (cancel_delayed_work (&idev->mc_ifc_work))__in6_dev_put (idev); } -/* called with mc_lock */ static void mld_dad_start_work (struct inet6_dev *idev,unsigned long delay) {unsigned long tv =get_random_u32_below (delay); +mc_assert_locked (idev); +if (!mod_delayed_work (mld_wq, &idev->mc_dad_work, tv +2 ))in6_dev_hold (idev); }
@@ -1155,14 +1167,13 @@ static void mld_report_stop_work(struct inet6_dev *idev)__in6_dev_put (idev); } -/* -* IGMP handling (alias multicast ICMPv6 messages) -* called with mc_lock -*/ +/* IGMP handling (alias multicast ICMPv6 messages) */ static void igmp6_group_queried (struct ifmcaddr6 *ma,unsigned long resptime) {unsigned long delay = resptime; +mc_assert_locked (ma->idev); +/* Do not start work for these addresses */ if (ipv6_addr_is_ll_all_nodes (&ma->mca_addr) ||IPV6_ADDR_MC_SCOPE (&ma->mca_addr) <IPV6_ADDR_SCOPE_LINKLOCAL )
@@ -1181,15 +1192,15 @@ static void igmp6_group_queried(struct ifmcaddr6 *ma, unsigned long resptime) ma->mca_flags |=MAF_TIMER_RUNNING ; } -/* mark EXCLUDE-mode sources -* called with mc_lock -*/ +/* mark EXCLUDE-mode sources */ static bool mld_xmarksources (struct ifmcaddr6 *pmc,int nsrcs,const struct in6_addr *srcs) {struct ip6_sf_list *psf;int i, scount; +mc_assert_locked (pmc->idev); + scount =0 ;for_each_psf_mclock (pmc, psf) {if (scount == nsrcs)
@@ -1212,13 +1223,14 @@ static bool mld_xmarksources(struct ifmcaddr6 *pmc, int nsrcs,return true; } -/* called with mc_lock */ static bool mld_marksources (struct ifmcaddr6 *pmc,int nsrcs,const struct in6_addr *srcs) {struct ip6_sf_list *psf;int i, scount; +mc_assert_locked (pmc->idev); +if (pmc->mca_sfmode ==MCAST_EXCLUDE )return mld_xmarksources (pmc, nsrcs, srcs);
@@ -1913,7 +1925,6 @@ static struct sk_buff *add_grhead(struct sk_buff *skb, struct ifmcaddr6 *pmc,#define AVAILABLE (skb) ((skb) ? skb_availroom(skb) : 0) -/* called with mc_lock */ static struct sk_buff *add_grec (struct sk_buff *skb,struct ifmcaddr6 *pmc,int type,int gdeleted,int sdeleted,int crsend)
@@ -1927,6 +1938,8 @@ static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,struct mld2_report *pmr;unsigned int mtu; +mc_assert_locked (idev); +if (pmc->mca_flags &MAF_NOREPORT )return skb;
@@ -2045,12 +2058,13 @@ static struct sk_buff *add_grec(struct sk_buff *skb, struct ifmcaddr6 *pmc,return skb; } -/* called with mc_lock */ static void mld_send_report (struct inet6_dev *idev,struct ifmcaddr6 *pmc) {struct sk_buff *skb =NULL ;int type; +mc_assert_locked (idev); +if (!pmc) {for_each_mc_mclock (idev, pmc) {if (pmc->mca_flags &MAF_NOREPORT )
@@ -2072,10 +2086,7 @@ static void mld_send_report(struct inet6_dev *idev, struct ifmcaddr6 *pmc)mld_sendpack (skb); } -/* -* remove zero-count source records from a source filter list -* called with mc_lock -*/ +/* remove zero-count source records from a source filter list */ static void mld_clear_zeros (struct ip6_sf_list __rcu **ppsf,struct inet6_dev *idev) {struct ip6_sf_list *psf_prev, *psf_next, *psf;
@@ -2099,7 +2110,6 @@ static void mld_clear_zeros(struct ip6_sf_list __rcu **ppsf, struct inet6_dev *i } } -/* called with mc_lock */ static void mld_send_cr (struct inet6_dev *idev) {struct ifmcaddr6 *pmc, *pmc_prev, *pmc_next;
@@ -2263,13 +2273,14 @@ static void igmp6_send(struct in6_addr *addr, struct net_device *dev, int type) goto out; } -/* called with mc_lock */ static void mld_send_initial_cr (struct inet6_dev *idev) { -struct sk_buff *skb;struct ifmcaddr6 *pmc; +struct sk_buff *skb;int type; +mc_assert_locked (idev); +if (mld_in_v1_mode (idev))return ;
@@ -2316,13 +2327,14 @@ static void mld_dad_work(struct work_struct *work)in6_dev_put (idev); } -/* called with mc_lock */ static int ip6_mc_del1_src (struct ifmcaddr6 *pmc,int sfmode, -const struct in6_addr *psfsrc) +const struct in6_addr *psfsrc) {struct ip6_sf_list *psf, *psf_prev;int rv =0 ; +mc_assert_locked (pmc->idev); + psf_prev =NULL ;for_each_psf_mclock (pmc, psf) {if (ipv6_addr_equal (&psf->sf_addr, psfsrc))
@@ -2359,7 +2371,6 @@ static int ip6_mc_del1_src(struct ifmcaddr6 *pmc, int sfmode,return rv; } -/* called with mc_lock */ static int ip6_mc_del_src (struct inet6_dev *idev,const struct in6_addr *pmca,int sfmode,int sfcount,const struct in6_addr *psfsrc,int delta)
@@ -2371,6 +2382,8 @@ static int ip6_mc_del_src(struct inet6_dev *idev, const struct in6_addr *pmca,if (!idev)return -ENODEV ; +mc_assert_locked (idev); +for_each_mc_mclock (idev, pmc) {if (ipv6_addr_equal (pmca, &pmc->mca_addr))break ;
@@ -2412,15 +2425,14 @@ static int ip6_mc_del_src(struct inet6_dev *idev, const struct in6_addr *pmca,return err; } -/* -* Add multicast single-source filter to the interface list -* called with mc_lock -*/ +/* Add multicast single-source filter to the interface list */ static int ip6_mc_add1_src (struct ifmcaddr6 *pmc,int sfmode, -const struct in6_addr *psfsrc) +const struct in6_addr *psfsrc) {struct ip6_sf_list *psf, *psf_prev; +mc_assert_locked (pmc->idev); + psf_prev =NULL ;for_each_psf_mclock (pmc, psf) {if (ipv6_addr_equal (&psf->sf_addr, psfsrc))
@@ -2443,11 +2455,12 @@ static int ip6_mc_add1_src(struct ifmcaddr6 *pmc, int sfmode,return 0 ; } -/* called with mc_lock */ static void sf_markstate (struct ifmcaddr6 *pmc) { -struct ip6_sf_list *psf;int mca_xcount = pmc->mca_sfcount[MCAST_EXCLUDE ]; +struct ip6_sf_list *psf; + +mc_assert_locked (pmc->idev);for_each_psf_mclock (pmc, psf) {if (pmc->mca_sfcount[MCAST_EXCLUDE ]) {
@@ -2460,14 +2473,15 @@ static void sf_markstate(struct ifmcaddr6 *pmc) } } -/* called with mc_lock */ static int sf_setstate (struct ifmcaddr6 *pmc) { -struct ip6_sf_list *psf, *dpsf;int mca_xcount = pmc->mca_sfcount[MCAST_EXCLUDE ]; +struct ip6_sf_list *psf, *dpsf;int qrv = pmc->idev->mc_qrv;int new_in, rv; +mc_assert_locked (pmc->idev); + rv =0 ;for_each_psf_mclock (pmc, psf) {if (pmc->mca_sfcount[MCAST_EXCLUDE ]) {
@@ -2526,10 +2540,7 @@ static int sf_setstate(struct ifmcaddr6 *pmc)return rv; } -/* -* Add multicast source filter list to the interface list -* called with mc_lock -*/ +/* Add multicast source filter list to the interface list */ static int ip6_mc_add_src (struct inet6_dev *idev,const struct in6_addr *pmca,int sfmode,int sfcount,const struct in6_addr *psfsrc,int delta)
@@ -2541,6 +2552,8 @@ static int ip6_mc_add_src(struct inet6_dev *idev, const struct in6_addr *pmca,if (!idev)return -ENODEV ; +mc_assert_locked (idev); +for_each_mc_mclock (idev, pmc) {if (ipv6_addr_equal (pmca, &pmc->mca_addr))break ;
@@ -2588,11 +2601,12 @@ static int ip6_mc_add_src(struct inet6_dev *idev, const struct in6_addr *pmca,return err; } -/* called with mc_lock */ static void ip6_mc_clear_src (struct ifmcaddr6 *pmc) {struct ip6_sf_list *psf, *nextpsf; +mc_assert_locked (pmc->idev); +for (psf =mc_dereference (pmc->mca_tomb, pmc->idev); psf; psf = nextpsf) {
@@ -2613,11 +2627,12 @@ static void ip6_mc_clear_src(struct ifmcaddr6 *pmc)WRITE_ONCE (pmc->mca_sfcount[MCAST_EXCLUDE ],1 ); } -/* called with mc_lock */ static void igmp6_join_group (struct ifmcaddr6 *ma) {unsigned long delay; +mc_assert_locked (ma->idev); +if (ma->mca_flags &MAF_NOREPORT )return ;
@@ -2664,9 +2679,10 @@ static int ip6_mc_leave_src(struct sock *sk, struct ipv6_mc_socklist *iml,return err; } -/* called with mc_lock */ static void igmp6_leave_group (struct ifmcaddr6 *ma) { +mc_assert_locked (ma->idev); +if (mld_in_v1_mode (ma->idev)) {if (ma->mca_flags &MAF_LAST_REPORTER ) { igmp6_send(&ma->mca_addr, ma->idev->dev,
@@ -2711,9 +2727,10 @@ static void mld_ifc_work(struct work_struct *work)in6_dev_put (idev); } -/* called with mc_lock */ static void mld_ifc_event (struct inet6_dev *idev) { +mc_assert_locked (idev); +if (mld_in_v1_mode (idev))return ;
Signature · 1 line
2.49.0
PATCH v3 3/15 ipv6: mcast: Check inet6_dev->dead under idev->mc_lock in __ipv6_dev_mc_inc().
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
Since commit 63ed8de4be81 ("mld: add mc_lock for protecting per-interface mld data"), every multicast resource is protected by inet6_dev->mc_lock.
RTNL is unnecessary in terms of protection but still needed for synchronisation between addrconf_ifdown() and __ipv6_dev_mc_inc().
Once we removed RTNL, there would be a race below, where we could add a multicast address to a dead inet6_dev.
CPU1 CPU2
==== ====
addrconf_ifdown() __ipv6_dev_mc_inc()
if (idev->dead) <-- false
dead = true return -ENODEV;
ipv6_mc_destroy_dev() / ipv6_mc_down()
mutex_lock(&idev->mc_lock)
...
mutex_unlock(&idev->mc_lock)
mutex_lock(&idev->mc_lock)
...
mutex_unlock(&idev->mc_lock)The race window can be easily closed by checking inet6_dev->dead under inet6_dev->mc_lock in __ipv6_dev_mc_inc() as addrconf_ifdown() will acquire it after marking inet6_dev dead.
Let's check inet6_dev->dead under mc_lock in __ipv6_dev_mc_inc().
Note that now __ipv6_dev_mc_inc() no longer depends on RTNL and we can remove ASSERT_RTNL() there and the RTNL comment above addrconf_join_solict().
--- net/ipv6/addrconf.c | 7 +++---- net/ipv6/mcast.c | 11 +++++------ 2 files changed, 8 insertions(+), 10 deletions(-)
net/ipv6/addrconf.c +3 -4
@@ -2229,13 +2229,12 @@ void addrconf_dad_failure(struct sk_buff *skb, struct inet6_ifaddr *ifp)in6_ifa_put (ifp); } -/* Join to solicited addr multicast group. -* caller must hold RTNL */ +/* Join to solicited addr multicast group. */ void addrconf_join_solict (struct net_device *dev,const struct in6_addr *addr) {struct in6_addr maddr; -if (dev->flags&(IFF_LOOPBACK |IFF_NOARP )) +if (READ_ONCE (dev->flags) & (IFF_LOOPBACK |IFF_NOARP ))return ;addrconf_addr_solict_mult (addr, &maddr);
@@ -3865,7 +3864,7 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister)* Do not dev_put! */ if (unregister) { - idev->dead =1 ; +WRITE_ONCE (idev->dead,1 );/* protected by rtnl_lock */ RCU_INIT_POINTER (dev->ip6_ptr,NULL );
net/ipv6/mcast.c +5 -6
@@ -952,23 +952,22 @@ static void inet6_ifmcaddr_notify(struct net_device *dev,static int __ipv6_dev_mc_inc (struct net_device *dev,const struct in6_addr *addr,unsigned int mode) { -struct ifmcaddr6 *mc;struct inet6_dev *idev; - -ASSERT_RTNL (); +struct ifmcaddr6 *mc;/* we need to take a reference on idev */ idev =in6_dev_get (dev); -if (!idev)return -EINVAL ; -if (idev->dead) { +mutex_lock (&idev->mc_lock); + +if (READ_ONCE (idev->dead)) { +mutex_unlock (&idev->mc_lock);in6_dev_put (idev);return -ENODEV ; } -mutex_lock (&idev->mc_lock);for_each_mc_mclock (idev, mc) {if (ipv6_addr_equal (&mc->mca_addr, addr)) { mc->mca_users++;
Signature · 1 line
2.49.0
PATCH v3 4/15 ipv6: mcast: Remove mca_get().
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
Since commit 63ed8de4be81 ("mld: add mc_lock for protecting per-interface mld data"), the newly allocated struct ifmcaddr6 cannot be removed until inet6_dev->mc_lock is released, so mca_get() and mc_put() are unnecessary.
Let's remove the extra refcounting.
Note that mca_get() was only used in __ipv6_dev_mc_inc().
--- net/ipv6/mcast.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
net/ipv6/mcast.c +1 -8
@@ -867,11 +867,6 @@ static void mld_clear_report(struct inet6_dev *idev)spin_unlock_bh (&idev->mc_report_lock); } -static void mca_get (struct ifmcaddr6 *mc) -{ -refcount_inc (&mc->mca_refcnt); -} -static void ma_put (struct ifmcaddr6 *mc) {if (refcount_dec_and_test (&mc->mca_refcnt)) {
@@ -988,13 +983,11 @@ static int __ipv6_dev_mc_inc(struct net_device *dev,rcu_assign_pointer (mc->next, idev->mc_list);rcu_assign_pointer (idev->mc_list, mc); -mca_get (mc); -mld_del_delrec (idev, mc);igmp6_group_added (mc);inet6_ifmcaddr_notify (dev, mc,RTM_NEWMULTICAST );mutex_unlock (&idev->mc_lock); -ma_put (mc); +return 0 ; }
Signature · 1 line
2.49.0
PATCH v3 5/15 ipv6: mcast: Use in6_dev_get() in ipv6_dev_mc_dec().
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
As well as __ipv6_dev_mc_inc(), all code in __ipv6_dev_mc_dec() are protected by inet6_dev->mc_lock, and RTNL is not needed.
Let's use in6_dev_get() in ipv6_dev_mc_dec() and remove ASSERT_RTNL() in __ipv6_dev_mc_dec().
Now, we can remove the RTNL comment above addrconf_leave_solict() too.
--- net/ipv6/addrconf.c | 3 +-- net/ipv6/mcast.c | 14 ++++++-------- 2 files changed, 7 insertions(+), 10 deletions(-)
net/ipv6/addrconf.c +1 -2
@@ -2241,12 +2241,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; -if (idev->dev->flags&(IFF_LOOPBACK |IFF_NOARP )) +if (READ_ONCE (idev->dev->flags) & (IFF_LOOPBACK |IFF_NOARP ))return ;addrconf_addr_solict_mult (addr, &maddr);
net/ipv6/mcast.c +6 -8
@@ -1004,9 +1004,8 @@ int __ipv6_dev_mc_dec(struct inet6_dev *idev, const struct in6_addr *addr) {struct ifmcaddr6 *ma,__rcu **map; -ASSERT_RTNL (); -mutex_lock (&idev->mc_lock); +for (map = &idev->mc_list; (ma =mc_dereference (*map, idev)); map = &ma->next) {
@@ -1037,13 +1036,12 @@ int ipv6_dev_mc_dec(struct net_device *dev, const struct in6_addr *addr)struct inet6_dev *idev;int err; -ASSERT_RTNL (); - - idev =__in6_dev_get (dev); + idev =in6_dev_get (dev);if (!idev) - err = -ENODEV ; -else - err =__ipv6_dev_mc_dec (idev, addr); +return -ENODEV ; + + err =__ipv6_dev_mc_dec (idev, addr); +in6_dev_put (idev);return err; }
Signature · 1 line
2.49.0
PATCH v3 6/15 ipv6: mcast: Don't hold RTNL for IPV6_ADD_MEMBERSHIP and MCAST_JOIN_GROUP.
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
In __ipv6_sock_mc_join(), per-socket mld data is protected by lock_sock(), and only __dev_get_by_index() requires RTNL.
Let's use dev_get_by_index() and drop RTNL for IPV6_ADD_MEMBERSHIP and MCAST_JOIN_GROUP.
Note that we must call rt6_lookup() and dev_hold() under RCU.
If rt6_lookup() returns an entry from the exception table, dst_dev_put() could change rt->dev.dst to loopback concurrently, and the original device could lose the refcount before dev_hold() and unblock device registration.
dst_dev_put() is called from NETDEV_UNREGISTER and synchronize_net() follows it, so as long as rt6_lookup() and dev_hold() are called within the same RCU critical section, the dev is alive.
Even if the race happens, they are synchronised by idev->dead and mcast addresses are cleaned up.
For the racy access to rt->dst.dev, we use dst_dev().
---
v3: Add dst_dev() for rt->dst.dev
v2: Hold rcu_read_lock() around rt6_lookup & dev_hold()
--- net/ipv6/ipv6_sockglue.c | 2 -- net/ipv6/mcast.c | 24 +++++++++++++----------- 2 files changed, 13 insertions(+), 13 deletions(-)
net/ipv6/ipv6_sockglue.c +0 -2
@@ -121,11 +121,9 @@ static bool setsockopt_needs_rtnl(int optname) {switch (optname) {case IPV6_ADDRFORM : -case IPV6_ADD_MEMBERSHIP :case IPV6_DROP_MEMBERSHIP :case IPV6_JOIN_ANYCAST :case IPV6_LEAVE_ANYCAST : -case MCAST_JOIN_GROUP :case MCAST_LEAVE_GROUP :case MCAST_JOIN_SOURCE_GROUP :case MCAST_LEAVE_SOURCE_GROUP :
net/ipv6/mcast.c +13 -11
@@ -175,14 +175,12 @@ static int unsolicited_report_interval(struct inet6_dev *idev)static int __ipv6_sock_mc_join (struct sock *sk,int ifindex,const struct in6_addr *addr,unsigned int mode) { -struct net_device *dev =NULL ; -struct ipv6_mc_socklist *mc_lst;struct ipv6_pinfo *np =inet6_sk (sk); +struct ipv6_mc_socklist *mc_lst;struct net *net =sock_net (sk); +struct net_device *dev =NULL ;int err; -ASSERT_RTNL (); -if (!ipv6_addr_is_multicast (addr))return -EINVAL ;
@@ -202,13 +200,18 @@ static int __ipv6_sock_mc_join(struct sock *sk, int ifindex,if (ifindex ==0 ) {struct rt6_info *rt; + +rcu_read_lock (); rt =rt6_lookup (net, addr,NULL ,0 ,NULL ,0 );if (rt) { - dev = rt->dst.dev; + dev =dst_dev (&rt->dst); +dev_hold (dev);ip6_rt_put (rt); } - }else - dev =__dev_get_by_index (net, ifindex); +rcu_read_unlock (); + }else { + dev =dev_get_by_index (net, ifindex); + }if (!dev) {sock_kfree_s (sk, mc_lst,sizeof (*mc_lst));
@@ -219,12 +222,11 @@ static int __ipv6_sock_mc_join(struct sock *sk, int ifindex, mc_lst->sfmode = mode;RCU_INIT_POINTER (mc_lst->sflist,NULL ); -/* -* now add/increase the group membership on the device -*/ - +/* now add/increase the group membership on the device */ err =__ipv6_dev_mc_inc (dev, addr, mode); +dev_put (dev); +if (err) {sock_kfree_s (sk, mc_lst,sizeof (*mc_lst));return err;
Signature · 1 line
2.49.0
PATCH v3 7/15 ipv6: mcast: Don't hold RTNL for IPV6_DROP_MEMBERSHIP and MCAST_LEAVE_GROUP.
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
In __ipv6_sock_mc_drop(), per-socket mld data is protected by lock_sock(), and only __dev_get_by_index() and __in6_dev_get() require RTNL.
Let's use dev_get_by_index() and in6_dev_get() and drop RTNL for IPV6_ADD_MEMBERSHIP and MCAST_JOIN_GROUP.
Note that __ipv6_sock_mc_drop() is factorised to reuse in the next patch.
--- net/ipv6/ipv6_sockglue.c | 2 -- net/ipv6/mcast.c | 47 +++++++++++++++++++++++----------------- 2 files changed, 27 insertions(+), 22 deletions(-)
net/ipv6/ipv6_sockglue.c +0 -2
@@ -121,10 +121,8 @@ static bool setsockopt_needs_rtnl(int optname) {switch (optname) {case IPV6_ADDRFORM : -case IPV6_DROP_MEMBERSHIP :case IPV6_JOIN_ANYCAST :case IPV6_LEAVE_ANYCAST : -case MCAST_LEAVE_GROUP :case MCAST_JOIN_SOURCE_GROUP :case MCAST_LEAVE_SOURCE_GROUP :case MCAST_BLOCK_SOURCE :
net/ipv6/mcast.c +27 -20
64 lines not on this page · raw message
Signature · 1 line
2.49.0
Re: [PATCH v3 net-next 07/15] ipv6: mcast: Don't hold RTNL for IPV6_DROP_MEMBERSHIP and MCAST_LEAVE_GROUP.
Hello Kuniyuki,
Quoted 92 lines · Kuniyuki Iwashima
From: Kuniyuki Iwashima <[email protected]>
In
__ipv6_sock_mc_drop(), per-socket mld data is protected bylock_sock(), and only__dev_get_by_index()and__in6_dev_get()require RTNL.Let's use
dev_get_by_index()andin6_dev_get()and drop RTNL forIPV6_ADD_MEMBERSHIPandMCAST_JOIN_GROUP.Note that
__ipv6_sock_mc_drop()is factorised to reuse in the next patch.Signed-off-by Kuniyuki Iwashima <[email protected]>Reviewed-by Eric Dumazet <[email protected]>--- net/ipv6/ipv6_sockglue.c | 2 -- net/ipv6/mcast.c | 47 +++++++++++++++++++++++----------------- 2 files changed, 27 insertions(+), 22 deletions(-)net/ipv6/ipv6_sockglue.c +0
-2@@ -121,10 +121,8 @@ static bool setsockopt_needs_rtnl(int optname) {switch (optname) {case IPV6_ADDRFORM : -case IPV6_DROP_MEMBERSHIP :case IPV6_JOIN_ANYCAST :case IPV6_LEAVE_ANYCAST : -case MCAST_LEAVE_GROUP :case MCAST_JOIN_SOURCE_GROUP :case MCAST_LEAVE_SOURCE_GROUP :case MCAST_BLOCK_SOURCE :net/ipv6/mcast.c +26
-14@@ -253,14 +253,36 @@ int ipv6_sock_mc_join_ssm(struct sock *sk, int ifindex,/* * socket leave on multicast group */ +static void __ipv6_sock_mc_drop (struct sock *sk,struct ipv6_mc_socklist *mc_lst) +{ +struct net *net =sock_net (sk); +struct net_device *dev; + + dev =dev_get_by_index (net, mc_lst->ifindex); +if (dev) { +struct inet6_dev *idev =in6_dev_get (dev); + +ip6_mc_leave_src (sk, mc_lst, idev); + +if (idev) { +__ipv6_dev_mc_dec (idev, &mc_lst->addr); +in6_dev_put (idev); + } + +dev_put (dev); + }else { +ip6_mc_leave_src (sk, mc_lst,NULL ); + } + +atomic_sub (sizeof (*mc_lst), &sk->sk_omem_alloc); +kfree_rcu (mc_lst, rcu); +} +int ipv6_sock_mc_drop (struct sock *sk,int ifindex,const struct in6_addr *addr) {struct ipv6_pinfo *np =inet6_sk (sk); -struct ipv6_mc_socklist *mc_lst;struct ipv6_mc_socklist __rcu **lnk; -struct net *net =sock_net (sk); - -ASSERT_RTNL (); +struct ipv6_mc_socklist *mc_lst;if (!ipv6_addr_is_multicast (addr))return -EINVAL ;@@ -270,23 +292,8 @@ int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr) lnk = &mc_lst->next) {if ((ifindex ==0 || mc_lst->ifindex == ifindex) &&ipv6_addr_equal (&mc_lst->addr, addr)) { -struct net_device *dev; - *lnk = mc_lst->next; - - dev =__dev_get_by_index (net, mc_lst->ifindex); -if (dev) { -struct inet6_dev *idev =__in6_dev_get (dev); - -ip6_mc_leave_src (sk, mc_lst, idev); -if (idev) -__ipv6_dev_mc_dec (idev, &mc_lst->addr);
net/ipv6/mcast.c int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr) · line 299
Quoted 10 lines
net/ipv6/mcast.c +1
-6- }else { -ip6_mc_leave_src (sk, mc_lst,NULL ); - } - -atomic_sub (sizeof (*mc_lst), &sk->sk_omem_alloc); -kfree_rcu (mc_lst, rcu); +__ipv6_sock_mc_drop (sk, mc_lst);return 0 ; } }
I'm getting the below stack, though unreliably, during kernel-selftest/drivers/net/dsa/local_termination.sh runs with different new-next revisions based on v6.18-rcX:
RTNL: assertion failed at git/net/core/dev.c (9477)
WARNING: CPU: 1 PID: 527 at git/net/core/dev.c:9477 __dev_set_promiscuity+0x1d0/0x1e0 pc : __dev_set_promiscuity+0x1d0/0x1e0 Call trace: __dev_set_promiscuity+0x1d0/0x1e0 (P) __dev_set_rx_mode+0xf8/0x118 igmp6_group_dropped+0x1e8/0x618 __ipv6_dev_mc_dec+0x164/0x1d0 ipv6_sock_mc_drop+0x1ac/0x1e0 do_ipv6_setsockopt+0x1990/0x1e58 ipv6_setsockopt+0x74/0x100 udpv6_setsockopt+0x28/0x58 sock_common_setsockopt+0x7c/0xa0 do_sock_setsockopt+0xf8/0x250 __sys_setsockopt+0xa8/0x130 __arm64_sys_setsockopt+0x70/0x98 invoke_syscall+0x68/0x190 el0_svc_common.constprop.0+0x11c/0x150 do_el0_svc+0x38/0x50 el0_svc+0x4c/0x1e8 el0t_64_sync_handler+0xa0/0xe8 el0t_64_sync+0x198/0x1a0 irq event stamp: 138641 hardirqs last enabled at (138640): [<ffff80008013da14>] __up_console_sem+0x74/0x90 hardirqs last disabled at (138641): [<ffff800081823ee8>] el1_brk64+0x20/0x58 softirqs last enabled at (138610): [<ffff800081144814>] lock_sock_nested+0x8c/0xb8
Do you have an idea what could be forgotten in the Subject patch?
Do we need to drop ASSERT_RTNL() from __dev_set_promiscuity() now, or am I too naive?
Signature · 3 lines
Alexander Sverdlin Siemens AG www.siemens.com
Re: [PATCH v3 net-next 07/15] ipv6: mcast: Don't hold RTNL for IPV6_DROP_MEMBERSHIP and MCAST_LEAVE_GROUP.
Quoted 129 lines · Alexander
Hello Kuniyuki,
Quoted 92 lines · Kuniyuki Iwashima
From: Kuniyuki Iwashima <[email protected]>
In
__ipv6_sock_mc_drop(), per-socket mld data is protected bylock_sock(), and only__dev_get_by_index()and__in6_dev_get()require RTNL.Let's use
dev_get_by_index()andin6_dev_get()and drop RTNL forIPV6_ADD_MEMBERSHIPandMCAST_JOIN_GROUP.Note that
__ipv6_sock_mc_drop()is factorised to reuse in the next patch.Signed-off-by Kuniyuki Iwashima <[email protected]>Reviewed-by Eric Dumazet <[email protected]>--- net/ipv6/ipv6_sockglue.c | 2 -- net/ipv6/mcast.c | 47 +++++++++++++++++++++++----------------- 2 files changed, 27 insertions(+), 22 deletions(-)net/ipv6/ipv6_sockglue.c +0
-2@@ -121,10 +121,8 @@ static bool setsockopt_needs_rtnl(int optname) {switch (optname) {case IPV6_ADDRFORM : -case IPV6_DROP_MEMBERSHIP :case IPV6_JOIN_ANYCAST :case IPV6_LEAVE_ANYCAST : -case MCAST_LEAVE_GROUP :case MCAST_JOIN_SOURCE_GROUP :case MCAST_LEAVE_SOURCE_GROUP :case MCAST_BLOCK_SOURCE :net/ipv6/mcast.c +26
-14@@ -253,14 +253,36 @@ int ipv6_sock_mc_join_ssm(struct sock *sk, int ifindex,/* * socket leave on multicast group */ +static void __ipv6_sock_mc_drop (struct sock *sk,struct ipv6_mc_socklist *mc_lst) +{ +struct net *net =sock_net (sk); +struct net_device *dev; + + dev =dev_get_by_index (net, mc_lst->ifindex); +if (dev) { +struct inet6_dev *idev =in6_dev_get (dev); + +ip6_mc_leave_src (sk, mc_lst, idev); + +if (idev) { +__ipv6_dev_mc_dec (idev, &mc_lst->addr); +in6_dev_put (idev); + } + +dev_put (dev); + }else { +ip6_mc_leave_src (sk, mc_lst,NULL ); + } + +atomic_sub (sizeof (*mc_lst), &sk->sk_omem_alloc); +kfree_rcu (mc_lst, rcu); +} +int ipv6_sock_mc_drop (struct sock *sk,int ifindex,const struct in6_addr *addr) {struct ipv6_pinfo *np =inet6_sk (sk); -struct ipv6_mc_socklist *mc_lst;struct ipv6_mc_socklist __rcu **lnk; -struct net *net =sock_net (sk); - -ASSERT_RTNL (); +struct ipv6_mc_socklist *mc_lst;if (!ipv6_addr_is_multicast (addr))return -EINVAL ;@@ -270,23 +292,8 @@ int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr) lnk = &mc_lst->next) {if ((ifindex ==0 || mc_lst->ifindex == ifindex) &&ipv6_addr_equal (&mc_lst->addr, addr)) { -struct net_device *dev; - *lnk = mc_lst->next; - - dev =__dev_get_by_index (net, mc_lst->ifindex); -if (dev) { -struct inet6_dev *idev =__in6_dev_get (dev); - -ip6_mc_leave_src (sk, mc_lst, idev); -if (idev) -__ipv6_dev_mc_dec (idev, &mc_lst->addr);net/ipv6/mcast.c int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr) · line 299
Quoted 10 lines
net/ipv6/mcast.c +1
-6- }else { -ip6_mc_leave_src (sk, mc_lst,NULL ); - } - -atomic_sub (sizeof (*mc_lst), &sk->sk_omem_alloc); -kfree_rcu (mc_lst, rcu); +__ipv6_sock_mc_drop (sk, mc_lst);return 0 ; } }I'm getting the below stack, though unreliably, during
kernel-selftest/drivers/net/dsa/local_termination.shruns with different new-next revisions based on v6.18-rcX:RTNL: assertion failed at
git/net/core/dev.c(9477)WARNING: CPU: 1 PID: 527 at git/net/core/dev.c:9477 __dev_set_promiscuity+0x1d0/0x1e0 pc : __dev_set_promiscuity+0x1d0/0x1e0 Call trace: __dev_set_promiscuity+0x1d0/0x1e0 (P) __dev_set_rx_mode+0xf8/0x118 igmp6_group_dropped+0x1e8/0x618 __ipv6_dev_mc_dec+0x164/0x1d0 ipv6_sock_mc_drop+0x1ac/0x1e0 do_ipv6_setsockopt+0x1990/0x1e58 ipv6_setsockopt+0x74/0x100 udpv6_setsockopt+0x28/0x58 sock_common_setsockopt+0x7c/0xa0 do_sock_setsockopt+0xf8/0x250 __sys_setsockopt+0xa8/0x130 __arm64_sys_setsockopt+0x70/0x98 invoke_syscall+0x68/0x190 el0_svc_common.constprop.0+0x11c/0x150
Quoted 9 lines
do_el0_svc+0x38/0x50 el0_svc+0x4c/0x1e8 el0t_64_sync_handler+0xa0/0xe8 el0t_64_sync+0x198/0x1a0 irq event stamp: 138641 hardirqs last enabled at (138640): [<ffff80008013da14>] __up_console_sem+0x74/0x90 hardirqs last disabled at (138641): [<ffff800081823ee8>] el1_brk64+0x20/0x58 softirqs last enabled at (138610): [<ffff800081144814>] lock_sock_nested+0x8c/0xb8
Thanks for the report!
Quoted 4 lines
Do you have an idea what could be forgotten in the Subject patch?
Do we need to drop
ASSERT_RTNL()from__dev_set_promiscuity()now, or am I too naive?
hmm.. ASSERT_RTNL() is still needed there given not all callers hold netdev_lock() and ndo_change_rx_flags() could nest the call.
But let me think if we can do something better than reverting it.
Re: [PATCH v3 net-next 07/15] ipv6: mcast: Don't hold RTNL for IPV6_DROP_MEMBERSHIP and MCAST_LEAVE_GROUP.
Hi Kuniyuki,
Quoted 139 lines · Kuniyuki Iwashima
Quoted 125 lines
Quoted 99 lines
From: Kuniyuki Iwashima <[email protected]>
In
__ipv6_sock_mc_drop(), per-socket mld data is protected bylock_sock(), and only__dev_get_by_index()and__in6_dev_get()require RTNL.Let's use
dev_get_by_index()andin6_dev_get()and drop RTNL forIPV6_ADD_MEMBERSHIPandMCAST_JOIN_GROUP.Note that
__ipv6_sock_mc_drop()is factorised to reuse in the next patch.Signed-off-by Kuniyuki Iwashima <[email protected]>Reviewed-by Eric Dumazet <[email protected]>---
net/ipv6/ipv6_sockglue.c| 2 --
net/ipv6/mcast.c | 47 +++++++++++++++++++++++-----------------2 files changed, 27 insertions(+), 22 deletions(-)net/ipv6/ipv6_sockglue.c +0
-0@@ -121,10 +121,8 @@ static bool setsockopt_needs_rtnl(int optname) { switch (optname) {case
IPV6_ADDRFORM:
- caseIPV6_DROP_MEMBERSHIP:
caseIPV6_JOIN_ANYCAST:
caseIPV6_LEAVE_ANYCAST:
- caseMCAST_LEAVE_GROUP:
caseMCAST_JOIN_SOURCE_GROUP:
caseMCAST_LEAVE_SOURCE_GROUP:
caseMCAST_BLOCK_SOURCE:net/ipv6/mcast.c +26
-4@@ -253,14 +253,36 @@ int ipv6_sock_mc_join_ssm(struct sock *sk, int ifindex, /** socket leave on multicast group */ +static void __ipv6_sock_mc_drop (struct sock *sk,struct ipv6_mc_socklist *mc_lst) +{ + struct net *net = sock_net(sk); + struct net_device *dev; + + dev = dev_get_by_index(net, mc_lst->ifindex); + if (dev) { + struct inet6_dev *idev = in6_dev_get(dev); + + ip6_mc_leave_src(sk, mc_lst, idev); + + if (idev) { + __ipv6_dev_mc_dec(idev, &mc_lst->addr); + in6_dev_put(idev); + } + + dev_put(dev); + } else { + ip6_mc_leave_src(sk, mc_lst, NULL); + } + + atomic_sub(sizeof(*mc_lst), &sk->sk_omem_alloc); + kfree_rcu(mc_lst, rcu); +} + int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr) { struct ipv6_pinfo *np = inet6_sk(sk); - struct ipv6_mc_socklist *mc_lst; struct ipv6_mc_socklist __rcu **lnk; - struct net *net = sock_net(sk); - - ASSERT_RTNL(); + struct ipv6_mc_socklist *mc_lst; if (!ipv6_addr_is_multicast(addr)) return -EINVAL;@@ -270,23 +292,8 @@ int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr) lnk = &mc_lst->next) {if ((ifindex == 0 ||
mc_lst->ifindex== ifindex) &&ipv6_addr_equal(&mc_lst->addr, addr)) { - structnet_device*dev;
-
*lnk =mc_lst->next;net/ipv6/mcast.c +1
-14- - dev = __dev_get_by_index(net, mc_lst->ifindex); - if (dev) { - struct inet6_dev *idev = __in6_dev_get(dev); - - ip6_mc_leave_src(sk, mc_lst, idev); - if (idev) - __ipv6_dev_mc_dec(idev, &mc_lst->addr); - } else { - ip6_mc_leave_src(sk, mc_lst, NULL); - } - - atomic_sub(sizeof(*mc_lst), &sk->sk_omem_alloc); - kfree_rcu(mc_lst, rcu); + __ipv6_sock_mc_drop(sk, mc_lst);Quoted 3 lines
return 0;
}
}I'm getting the below stack, though unreliably, during
kernel-selftest/drivers/net/dsa/local_termination.shruns with different new-next revisions based on v6.18-rcX:RTNL: assertion failed at
git/net/core/dev.c(9477)WARNING: CPU: 1 PID: 527 at git/net/core/dev.c:9477 __dev_set_promiscuity+0x1d0/0x1e0 pc : __dev_set_promiscuity+0x1d0/0x1e0 Call trace: __dev_set_promiscuity+0x1d0/0x1e0 (P) __dev_set_rx_mode+0xf8/0x118 igmp6_group_dropped+0x1e8/0x618 __ipv6_dev_mc_dec+0x164/0x1d0 ipv6_sock_mc_drop+0x1ac/0x1e0 do_ipv6_setsockopt+0x1990/0x1e58 ipv6_setsockopt+0x74/0x100 udpv6_setsockopt+0x28/0x58 sock_common_setsockopt+0x7c/0xa0 do_sock_setsockopt+0xf8/0x250 __sys_setsockopt+0xa8/0x130 __arm64_sys_setsockopt+0x70/0x98 invoke_syscall+0x68/0x190 el0_svc_common.constprop.0+0x11c/0x150Quoted 9 lines
do_el0_svc+0x38/0x50 el0_svc+0x4c/0x1e8 el0t_64_sync_handler+0xa0/0xe8 el0t_64_sync+0x198/0x1a0 irq event stamp: 138641 hardirqs last enabled at (138640): [<ffff80008013da14>] __up_console_sem+0x74/0x90 hardirqs last disabled at (138641): [<ffff800081823ee8>] el1_brk64+0x20/0x58 softirqs last enabled at (138610): [<ffff800081144814>] lock_sock_nested+0x8c/0xb8Thanks for the report!
Quoted 2 lines
Do you have an idea what could be forgotten in the Subject patch?
Quoted 7 lines
Quoted 2 lines
Do we need to drop
ASSERT_RTNL()from__dev_set_promiscuity()now, or am I too naive?hmm..
ASSERT_RTNL()is still needed there given not all callers holdnetdev_lock()andndo_change_rx_flags()could nest the call.But let me think if we can do something better than reverting it.
I'll be happy to test a patch, you definitely have more insights into the different locks in this area... I was looking into it yesterday, but the netdev_lock_ops() in dev_set_promiscuity() just doesn't look right as a replacement for __dev_set_promiscuity() because it's optional (for some reason) and it seems we need some dumb per-net_device lock here?
Signature · 3 lines
Alexander Sverdlin Siemens AG www.siemens.com
PATCH v3 8/15 ipv6: mcast: Don't hold RTNL in ipv6_sock_mc_close().
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
In __ipv6_sock_mc_close(), per-socket mld data is protected by lock_sock(), and only __dev_get_by_index() and __in6_dev_get() require RTNL.
Let's call __ipv6_sock_mc_drop() and drop RTNL in ipv6_sock_mc_close().
--- net/ipv6/mcast.c | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-)
net/ipv6/mcast.c +1 -21
@@ -334,28 +334,10 @@ void __ipv6_sock_mc_close(struct sock *sk) {struct ipv6_pinfo *np =inet6_sk (sk);struct ipv6_mc_socklist *mc_lst; -struct net *net =sock_net (sk); - -ASSERT_RTNL ();while ((mc_lst =sock_dereference (np->ipv6_mc_list, sk)) !=NULL ) { -struct net_device *dev; - np->ipv6_mc_list = mc_lst->next; - - dev =__dev_get_by_index (net, mc_lst->ifindex); -if (dev) { -struct inet6_dev *idev =__in6_dev_get (dev); - -ip6_mc_leave_src (sk, mc_lst, idev); -if (idev) -__ipv6_dev_mc_dec (idev, &mc_lst->addr); - }else { -ip6_mc_leave_src (sk, mc_lst,NULL ); - } - -atomic_sub (sizeof (*mc_lst), &sk->sk_omem_alloc); -kfree_rcu (mc_lst, rcu); +__ipv6_sock_mc_drop (sk, mc_lst); } }
@@ -366,11 +348,9 @@ void ipv6_sock_mc_close(struct sock *sk)if (!rcu_access_pointer (np->ipv6_mc_list))return ; -rtnl_lock ();lock_sock (sk);__ipv6_sock_mc_close (sk);release_sock (sk); -rtnl_unlock (); } int ip6_mc_source(int add, int omode, struct sock *sk,
Signature · 1 line
2.49.0
PATCH v3 9/15 ipv6: mcast: Don't hold RTNL for MCAST_ socket options.
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
In ip6_mc_source() and ip6_mc_msfilter(), per-socket mld data is protected by lock_sock() and inet6_dev->mc_lock is also held for some per-interface functions.
ip6_mc_find_dev_rtnl() only depends on RTNL. If we want to remove it, we need to check inet6_dev->dead under mc_lock to close the race with addrconf_ifdown(), as mentioned earlier.
Let's do that and drop RTNL for the rest of MCAST_ socket options.
Note that ip6_mc_msfilter() has unnecessary lock dances and they are integrated into one to avoid the last-minute error and simplify the error handling.
---
v3: Use dst_dev() for rt->dst.dev
v2: Hold rcu_read_lock() around rt6_lookup & dev_hold()
--- net/ipv6/ipv6_sockglue.c | 5 --- net/ipv6/mcast.c | 74 ++++++++++++++++++++++++---------------- 2 files changed, 45 insertions(+), 34 deletions(-)
net/ipv6/ipv6_sockglue.c +0 -5
@@ -123,11 +123,6 @@ static bool setsockopt_needs_rtnl(int optname)case IPV6_ADDRFORM :case IPV6_JOIN_ANYCAST :case IPV6_LEAVE_ANYCAST : -case MCAST_JOIN_SOURCE_GROUP : -case MCAST_LEAVE_SOURCE_GROUP : -case MCAST_BLOCK_SOURCE : -case MCAST_UNBLOCK_SOURCE : -case MCAST_MSFILTER :return true; }return false;
net/ipv6/mcast.c +45 -29
169 lines not on this page · raw message
Signature · 1 line
2.49.0
PATCH v3 10/15 ipv6: mcast: Remove unnecessary ASSERT_RTNL and comment.
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
Now, RTNL is not needed for mcast code, and what's commented in ip6_mc_msfget() is apparent by for_each_pmc_socklock(), which has lockdep annotation for lock_sock().
Let's remove the comment and ASSERT_RTNL() in ipv6_mc_rejoin_groups().
--- net/ipv6/mcast.c | 6 ------ 1 file changed, 6 deletions(-)
net/ipv6/mcast.c +0 -6
@@ -605,10 +605,6 @@ int ip6_mc_msfget(struct sock *sk, struct group_filter *gsf,if (!ipv6_addr_is_multicast (group))return -EINVAL ; -/* changes to the ipv6_mc_list require the socket lock and -* rtnl lock. We have the socket lock, so reading the list is safe. -*/ -for_each_pmc_socklock (inet6, sk, pmc) {if (pmc->ifindex != gsf->gf_interface)continue ;
@@ -2880,8 +2876,6 @@ static void ipv6_mc_rejoin_groups(struct inet6_dev *idev) {struct ifmcaddr6 *pmc; -ASSERT_RTNL (); -mutex_lock (&idev->mc_lock);if (mld_in_v1_mode (idev)) {for_each_mc_mclock (idev, pmc)
Signature · 1 line
2.49.0
PATCH v3 11/15 ipv6: anycast: Don't use rtnl_dereference().
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
inet6_dev->ac_list is protected by inet6_dev->lock, so rtnl_dereference() is a bit rough annotation.
As done in mcast.c, we can use ac_dereference() that checks if inet6_dev->lock is held.
Let's replace rtnl_dereference() with a new helper ac_dereference().
Note that now addrconf_join_solict() / addrconf_leave_solict() in __ipv6_dev_ac_inc() / __ipv6_dev_ac_dec() does not need RTNL, so we can remove ASSERT_RTNL() there.
--- net/ipv6/addrconf.c | 2 -- net/ipv6/anycast.c | 17 ++++++++--------- 2 files changed, 8 insertions(+), 11 deletions(-)
net/ipv6/addrconf.c +0 -2
@@ -2252,7 +2252,6 @@ 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;
@@ -2265,7 +2264,6 @@ static void addrconf_join_anycast(struct inet6_ifaddr *ifp)__ipv6_dev_ac_inc (ifp->idev, &addr); } -/* caller must hold RTNL */ static void addrconf_leave_anycast (struct inet6_ifaddr *ifp) {struct in6_addr addr;
net/ipv6/anycast.c +8 -9
@@ -47,6 +47,9 @@static struct hlist_head inet6_acaddr_lst[IN6_ADDR_HSIZE ];static DEFINE_SPINLOCK (acaddr_hash_lock); +#define ac_dereference (a, idev) \ + rcu_dereference_protected(a, lockdep_is_held(&(idev)->lock)) +static int ipv6_dev_ac_dec (struct net_device *dev,const struct in6_addr *addr); static u32 inet6_acaddr_hash(const struct net *net,
@@ -319,16 +322,14 @@ int __ipv6_dev_ac_inc(struct inet6_dev *idev, const struct in6_addr *addr)struct net *net;int err; -ASSERT_RTNL (); -write_lock_bh (&idev->lock);if (idev->dead) { err = -ENODEV ; goto out; } -for (aca =rtnl_dereference (idev->ac_list); aca; - aca =rtnl_dereference (aca->aca_next)) { +for (aca =ac_dereference (idev->ac_list, idev); aca; + aca =ac_dereference (aca->aca_next, idev)) {if (ipv6_addr_equal (&aca->aca_addr, addr)) { aca->aca_users++; err =0 ;
@@ -380,12 +381,10 @@ 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 =rtnl_dereference (idev->ac_list); aca; - aca =rtnl_dereference (aca->aca_next)) { +for (aca =ac_dereference (idev->ac_list, idev); aca; + aca =ac_dereference (aca->aca_next, idev)) {if (ipv6_addr_equal (&aca->aca_addr, addr))break ; prev_aca = aca;
@@ -429,7 +428,7 @@ void ipv6_ac_destroy_dev(struct inet6_dev *idev)struct ifacaddr6 *aca;write_lock_bh (&idev->lock); -while ((aca =rtnl_dereference (idev->ac_list)) !=NULL ) { +while ((aca =ac_dereference (idev->ac_list, idev)) !=NULL ) {rcu_assign_pointer (idev->ac_list, aca->aca_next);write_unlock_bh (&idev->lock);
Signature · 1 line
2.49.0
PATCH v3 12/15 ipv6: anycast: Don't hold RTNL for IPV6_LEAVE_ANYCAST and IPV6_ADDRFORM.
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
inet6_sk(sk)->ipv6_ac_list is protected by lock_sock().
In ipv6_sock_ac_drop() and ipv6_sock_ac_close(), only __dev_get_by_index() and __in6_dev_get() requrie RTNL.
Let's replace them with dev_get_by_index() and in6_dev_get() and drop RTNL from IPV6_LEAVE_ANYCAST and IPV6_ADDRFORM.
--- net/ipv6/anycast.c | 36 ++++++++++++++++++++---------------- net/ipv6/ipv6_sockglue.c | 2 -- 2 files changed, 20 insertions(+), 18 deletions(-)
net/ipv6/anycast.c +20 -16
@@ -158,12 +158,10 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)*/ int ipv6_sock_ac_drop (struct sock *sk,int ifindex,const struct in6_addr *addr) { -struct ipv6_pinfo *np =inet6_sk (sk); -struct net_device *dev;struct ipv6_ac_socklist *pac, *prev_pac; +struct ipv6_pinfo *np =inet6_sk (sk);struct net *net =sock_net (sk); - -ASSERT_RTNL (); +struct net_device *dev; prev_pac =NULL ;for (pac = np->ipv6_ac_list; pac; pac = pac->acl_next) {
@@ -179,9 +177,11 @@ int ipv6_sock_ac_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)else np->ipv6_ac_list = pac->acl_next; - dev =__dev_get_by_index (net, pac->acl_ifindex); -if (dev) + dev =dev_get_by_index (net, pac->acl_ifindex); +if (dev) {ipv6_dev_ac_dec (dev, &pac->acl_addr); +dev_put (dev); + }sock_kfree_s (sk, pac,sizeof (*pac));return 0 ;
@@ -190,21 +190,20 @@ int ipv6_sock_ac_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)void __ipv6_sock_ac_close (struct sock *sk) {struct ipv6_pinfo *np =inet6_sk (sk); +struct net *net =sock_net (sk);struct net_device *dev =NULL ;struct ipv6_ac_socklist *pac; -struct net *net =sock_net (sk); -int prev_index; +int prev_index =0 ; -ASSERT_RTNL (); pac = np->ipv6_ac_list; np->ipv6_ac_list =NULL ; - prev_index =0 ;while (pac) {struct ipv6_ac_socklist *next = pac->acl_next;if (pac->acl_ifindex != prev_index) { - dev =__dev_get_by_index (net, pac->acl_ifindex); +dev_put (dev); + dev =dev_get_by_index (net, pac->acl_ifindex); prev_index = pac->acl_ifindex; }if (dev)
@@ -212,6 +211,8 @@ void __ipv6_sock_ac_close(struct sock *sk)sock_kfree_s (sk, pac,sizeof (*pac)); pac = next; } + +dev_put (dev); }void ipv6_sock_ac_close (struct sock *sk)
@@ -220,9 +221,8 @@ void ipv6_sock_ac_close(struct sock *sk)if (!np->ipv6_ac_list)return ; -rtnl_lock (); +__ipv6_sock_ac_close (sk); -rtnl_unlock (); }static void ipv6_add_acaddr_hash (struct net *net,struct ifacaddr6 *aca)
@@ -413,14 +413,18 @@ int __ipv6_dev_ac_dec(struct inet6_dev *idev, const struct in6_addr *addr)return 0 ; } -/* called with rtnl_lock() */ static int ipv6_dev_ac_dec (struct net_device *dev,const struct in6_addr *addr) { -struct inet6_dev *idev =__in6_dev_get (dev); +struct inet6_dev *idev =in6_dev_get (dev); +int err;if (!idev)return -ENODEV ; -return __ipv6_dev_ac_dec (idev, addr); + + err =__ipv6_dev_ac_dec (idev, addr); +in6_dev_put (idev); + +return err; }void ipv6_ac_destroy_dev (struct inet6_dev *idev)
net/ipv6/ipv6_sockglue.c +0 -2
@@ -120,9 +120,7 @@ struct ipv6_txoptions *ipv6_update_options(struct sock *sk,static bool setsockopt_needs_rtnl (int optname) {switch (optname) { -case IPV6_ADDRFORM :case IPV6_JOIN_ANYCAST : -case IPV6_LEAVE_ANYCAST :return true; }return false;
Signature · 1 line
2.49.0
PATCH v3 13/15 ipv6: anycast: Unify two error paths in ipv6_sock_ac_join().
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
The next patch will replace __dev_get_by_index() and __dev_get_by_flags() to RCU + refcount version.
Then, we will need to call dev_put() in some error paths.
Let's unify two error paths to make the next patch cleaner.
Note that we add READ_ONCE() for net->ipv6.devconf_all->forwarding and idev->conf.forwarding as we will drop RTNL that protects them.
---
v3: Use READ_ONCE() for net->ipv6.devconf_all->forwarding and idev->conf.forwarding
--- net/ipv6/anycast.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
net/ipv6/anycast.c +16 -9
@@ -67,12 +67,11 @@ static u32 inet6_acaddr_hash(const struct net *net,int ipv6_sock_ac_join (struct sock *sk,int ifindex,const struct in6_addr *addr) {struct ipv6_pinfo *np =inet6_sk (sk); +struct ipv6_ac_socklist *pac =NULL ; +struct net *net =sock_net (sk);struct net_device *dev =NULL ;struct inet6_dev *idev; -struct ipv6_ac_socklist *pac; -struct net *net =sock_net (sk); -int ishost = !net->ipv6.devconf_all->forwarding; -int err =0 ; +int err =0 , ishost;ASSERT_RTNL ();
@@ -84,15 +83,22 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)if (ifindex) dev =__dev_get_by_index (net, ifindex); -if (ipv6_chk_addr_and_flags (net, addr, dev, true,0 ,IFA_F_TENTATIVE )) -return -EINVAL ; +if (ipv6_chk_addr_and_flags (net, addr, dev, true,0 ,IFA_F_TENTATIVE )) { + err = -EINVAL ; + goto error; + } pac =sock_kmalloc (sk,sizeof (struct ipv6_ac_socklist ),GFP_KERNEL ); -if (!pac) -return -ENOMEM ; +if (!pac) { + err = -ENOMEM ; + goto error; + } + pac->acl_next =NULL ; pac->acl_addr = *addr; + ishost = !READ_ONCE (net->ipv6.devconf_all->forwarding); +if (ifindex ==0 ) {struct rt6_info *rt;
@@ -123,8 +129,9 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr) err = -EADDRNOTAVAIL ; goto error; } +/* reset ishost, now that we have a specific device */ - ishost = !idev->cnf.forwarding; + ishost = !READ_ONCE (idev->cnf.forwarding); pac->acl_ifindex = dev->ifindex;
Signature · 1 line
2.49.0
PATCH v3 14/15 ipv6: anycast: Don't hold RTNL for IPV6_JOIN_ANYCAST.
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
inet6_sk(sk)->ipv6_ac_list is protected by lock_sock().
In ipv6_sock_ac_join(), only __dev_get_by_index(), __dev_get_by_flags(), and __in6_dev_get() require RTNL.
__dev_get_by_flags() is only used by ipv6_sock_ac_join() and can be converted to RCU version.
Let's replace RCU version helper and drop RTNL from IPV6_JOIN_ANYCAST.
setsockopt_needs_rtnl() will be removed in the next patch.
---
v3: Use dst_dev() for rt->dst.dev
v2: Hold rcu_read_lock() around rt6_lookup & dev_hold()
--- include/linux/netdevice.h | 4 ++-- net/core/dev.c | 38 ++++++++++++++++++-------------------- net/ipv6/anycast.c | 22 ++++++++++++++-------- net/ipv6/ipv6_sockglue.c | 4 ---- 4 files changed, 34 insertions(+), 34 deletions(-)
include/linux/netdevice.h +2 -2
@@ -3332,8 +3332,8 @@ int dev_get_iflink(const struct net_device *dev);int dev_fill_metadata_dst (struct net_device *dev,struct sk_buff *skb);int dev_fill_forward_path (const struct net_device *dev,const u8 *daddr,struct net_device_path_stack *stack); -struct net_device *__dev_get_by_flags (struct net *net,unsigned short flags, -unsigned short mask); +struct net_device *dev_get_by_flags_rcu (struct net *net,unsigned short flags, +unsigned short mask);struct net_device *dev_get_by_name (struct net *net,const char *name);struct net_device *dev_get_by_name_rcu (struct net *net,const char *name);struct net_device *__dev_get_by_name (struct net *net,const char *name);
net/core/dev.c +18 -20
@@ -1267,33 +1267,31 @@ struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type)EXPORT_SYMBOL (dev_getfirstbyhwtype);/** -* __dev_get_by_flags - find any device with given flags -* @net: the applicable net namespace -* @if_flags: IFF_* values -* @mask: bitmask of bits in if_flags to check +* dev_get_by_flags_rcu - find any device with given flags +* @net: the applicable net namespace +* @if_flags: IFF_* values +* @mask: bitmask of bits in if_flags to check * -* Search for any interface with the given flags. Returns NULL if a device -* is not found or a pointer to the device. Must be called inside -* rtnl_lock(), and result refcount is unchanged. +* Search for any interface with the given flags. +* +* Context: rcu_read_lock() must be held. +* Returns: NULL if a device is not found or a pointer to the device. */ - -struct net_device *__dev_get_by_flags (struct net *net,unsigned short if_flags, -unsigned short mask) +struct net_device *dev_get_by_flags_rcu (struct net *net,unsigned short if_flags, +unsigned short mask) { -struct net_device *dev, *ret; - -ASSERT_RTNL (); +struct net_device *dev; - ret =NULL ; -for_each_netdev (net, dev) { -if (((dev->flags ^ if_flags) & mask) ==0 ) { - ret = dev; -break ; +for_each_netdev_rcu (net, dev) { +if (((READ_ONCE (dev->flags) ^ if_flags) & mask) ==0 ) { +dev_hold (dev); +return dev; } } -return ret; + +return NULL ; } -EXPORT_SYMBOL (__dev_get_by_flags); +EXPORT_IPV6_MOD (dev_get_by_flags_rcu);/** * dev_valid_name - check if name is okay for network device
net/ipv6/anycast.c +14 -8
@@ -73,15 +73,13 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)struct inet6_dev *idev;int err =0 , ishost; -ASSERT_RTNL (); -if (!ns_capable (net->user_ns,CAP_NET_ADMIN ))return -EPERM ;if (ipv6_addr_is_multicast (addr))return -EINVAL ;if (ifindex) - dev =__dev_get_by_index (net, ifindex); + dev =dev_get_by_index (net, ifindex);if (ipv6_chk_addr_and_flags (net, addr, dev, true,0 ,IFA_F_TENTATIVE )) { err = -EINVAL ;
@@ -102,18 +100,22 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)if (ifindex ==0 ) {struct rt6_info *rt; +rcu_read_lock (); rt =rt6_lookup (net, addr,NULL ,0 ,NULL ,0 );if (rt) { - dev = rt->dst.dev; + dev =dst_dev (&rt->dst); +dev_hold (dev);ip6_rt_put (rt); }else if (ishost) { +rcu_read_unlock (); err = -EADDRNOTAVAIL ; goto error; }else {/* router, no matching interface: just pick one */ - dev =__dev_get_by_flags (net,IFF_UP , -IFF_UP |IFF_LOOPBACK ); + dev =dev_get_by_flags_rcu (net,IFF_UP , +IFF_UP |IFF_LOOPBACK ); } +rcu_read_unlock (); }if (!dev) {
@@ -121,7 +123,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr) goto error; } - idev =__in6_dev_get (dev); + idev =in6_dev_get (dev);if (!idev) {if (ifindex) err = -ENODEV ;
@@ -144,7 +146,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)if (ishost) err = -EADDRNOTAVAIL ;if (err) - goto error; + goto error_idev; } err =__ipv6_dev_ac_inc (idev, addr);
@@ -154,7 +156,11 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr) pac =NULL ; } +error_idev: +in6_dev_put (idev); error: +dev_put (dev); +if (pac)sock_kfree_s (sk, pac,sizeof (*pac));return err;
net/ipv6/ipv6_sockglue.c +0 -4
@@ -119,10 +119,6 @@ struct ipv6_txoptions *ipv6_update_options(struct sock *sk,static bool setsockopt_needs_rtnl (int optname) { -switch (optname) { -case IPV6_JOIN_ANYCAST : -return true; - }return false; }
Signature · 1 line
2.49.0
Jakub Kicinski
maintainerRe: [PATCH v3 net-next 14/15] ipv6: anycast: Don't hold RTNL for IPV6_JOIN_ANYCAST.
net/core/dev.c struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type) · line 1281
Quoted 4 lines · On Wed, 2 Jul 2025 16:01:31 -0700 Kuniyuki Iwashima wrote:
net/core/dev.c +2
-2-struct net_device *__dev_get_by_flags (struct net *net,unsigned short if_flags, -unsigned short mask) +struct net_device *dev_get_by_flags_rcu (struct net *net,unsigned short if_flags, +unsigned short mask)
Could you follow up and toss a netdev tracker in here?
Not sure how much it matters but looks trivial, and we shouldn't really add APIs that take a bare reference these days..
Re: [PATCH v3 net-next 14/15] ipv6: anycast: Don't hold RTNL for IPV6_JOIN_ANYCAST.
Quoted 10 lines · Jakub Kicinski
net/core/dev.c struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type) · line 1281
Quoted 4 lines · On Wed, 2 Jul 2025 16:01:31 -0700 Kuniyuki Iwashima wrote:
net/core/dev.c +2
-2-struct net_device *__dev_get_by_flags (struct net *net,unsigned short if_flags, -unsigned short mask) +struct net_device *dev_get_by_flags_rcu (struct net *net,unsigned short if_flags, +unsigned short mask)Could you follow up and toss a netdev tracker in here?
Not sure how much it matters but looks trivial, and we shouldn't really add APIs that take a bare reference these days..
Fair enough, will post a followup :)
PATCH v3 15/15 ipv6: Remove setsockopt_needs_rtnl().
In reply to Kuniyuki IwashimaFrom: Kuniyuki Iwashima <[email protected]>
We no longer need to hold RTNL for IPv6 socket options.
Let's remove setsockopt_needs_rtnl().
--- net/ipv6/ipv6_sockglue.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
net/ipv6/ipv6_sockglue.c +2 -11
@@ -117,11 +117,6 @@ struct ipv6_txoptions *ipv6_update_options(struct sock *sk,return opt; } -static bool setsockopt_needs_rtnl (int optname) -{ -return false; -} -static int copy_group_source_from_sockptr (struct group_source_req *greqs,sockptr_t optval,int optlen) {
@@ -380,9 +375,8 @@ int do_ipv6_setsockopt(struct sock *sk, int level, int optname, {struct ipv6_pinfo *np =inet6_sk (sk);struct net *net =sock_net (sk); -int val, valbool;int retv = -ENOPROTOOPT ; -bool needs_rtnl =setsockopt_needs_rtnl (optname); +int val, valbool;if (sockptr_is_null (optval)) val =0 ;
@@ -547,8 +541,7 @@ int do_ipv6_setsockopt(struct sock *sk, int level, int optname,return 0 ; } } -if (needs_rtnl) -rtnl_lock (); +sockopt_lock_sock (sk);/* Another thread has converted the socket into IPv4 with
@@ -954,8 +947,6 @@ int do_ipv6_setsockopt(struct sock *sk, int level, int optname, unlock:sockopt_release_sock (sk); -if (needs_rtnl) -rtnl_unlock ();return retv;
Signature · 1 line
2.49.0
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:
Hello:
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:
Quoted 10 lines · On Wed, 2 Jul 2025 16:01:17 -0700 you wrote:
From: Kuniyuki Iwashima <[email protected]>
This is a prep series for RCU conversion of
RTM_NEWNEIGH, which needs RTNL during neigh_table.{pconstructor,pdestructor}() touching IPv6 multicast code.Currently, IPv6 multicast code is protected by
lock_sock()andinet6_dev->mc_lock, and RTNL is not actually needed.[...]
Here is the summary with links:
- [v3,net-next,01/15] ipv6: ndisc: Remove __in6_dev_get() in pndisc_{constructor,destructor}().
https://git.kernel.org/netdev/net-next/c/fb60b74e4e5b - [v3,net-next,02/15] ipv6: mcast: Replace locking comments with lockdep annotations. https://git.kernel.org/netdev/net-next/c/818ae1a5ecb4 - [v3,net-next,03/15] ipv6: mcast: Check inet6_dev->dead under idev->mc_lock in __ipv6_dev_mc_inc(). https://git.kernel.org/netdev/net-next/c/dbd40f318cf2 - [v3,net-next,04/15] ipv6: mcast: Remove mca_get(). https://git.kernel.org/netdev/net-next/c/d22faae8c555 - [v3,net-next,05/15] ipv6: mcast: Use in6_dev_get() in ipv6_dev_mc_dec(). https://git.kernel.org/netdev/net-next/c/e01b193e0b50 - [v3,net-next,06/15] ipv6: mcast: Don't hold RTNL for IPV6_ADD_MEMBERSHIP and MCAST_JOIN_GROUP. https://git.kernel.org/netdev/net-next/c/1767bb2d47b7 - [v3,net-next,07/15] ipv6: mcast: Don't hold RTNL for IPV6_DROP_MEMBERSHIP and MCAST_LEAVE_GROUP. https://git.kernel.org/netdev/net-next/c/2ceb71ce7d34 - [v3,net-next,08/15] ipv6: mcast: Don't hold RTNL in ipv6_sock_mc_close(). https://git.kernel.org/netdev/net-next/c/1e589db3892e - [v3,net-next,09/15] ipv6: mcast: Don't hold RTNL for MCAST_ socket options. https://git.kernel.org/netdev/net-next/c/e6e14d582dd2 - [v3,net-next,10/15] ipv6: mcast: Remove unnecessary ASSERT_RTNL and comment. https://git.kernel.org/netdev/net-next/c/49b8223fa9c1 - [v3,net-next,11/15] ipv6: anycast: Don't use rtnl_dereference(). https://git.kernel.org/netdev/net-next/c/7b6b53a76fcc - [v3,net-next,12/15] ipv6: anycast: Don't hold RTNL for IPV6_LEAVE_ANYCAST and IPV6_ADDRFORM. https://git.kernel.org/netdev/net-next/c/f7fdf13bf103 - [v3,net-next,13/15] ipv6: anycast: Unify two error paths in ipv6_sock_ac_join(). https://git.kernel.org/netdev/net-next/c/976fa9b2054f - [v3,net-next,14/15] ipv6: anycast: Don't hold RTNL for IPV6_JOIN_ANYCAST. https://git.kernel.org/netdev/net-next/c/eb1ac9ff6c4a - [v3,net-next,15/15] ipv6: Remove setsockopt_needs_rtnl(). https://git.kernel.org/netdev/net-next/c/db38443dcd9f
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html