| Message ID | 00ecfc1804b58d8dbb23b8a6e7e5c0646f0100e1.1600770261.git.sd@queasysnail.net |
|---|---|
| State | New |
| Headers | show |
| Series | net: iflink and link-netnsid fixes | expand |
Le 01/10/2020 à 09:59, Sabrina Dubroca a écrit : > Currently, we're not advertising link-netnsid when dumping IPv6 > addresses, so the "ip -6 addr" command will not correctly interpret > the value of the IFLA_LINK attribute. > > For example, we'll get: > 9: macvlan0@macvlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP qlen 1000 > <snip> > > Instead of: > 9: macvlan0@if9: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP qlen 1000 link-netns main > <snip> > > ndisc_ifinfo_sysctl_change calls inet6_fill_ifinfo without rcu or > rtnl, so I'm adding rcu_read_lock around rtnl_fill_link_netnsid. I don't think this is needed. ndisc_ifinfo_sysctl_change() takes a reference on the idev (with in6_dev_get(dev)).
2020-10-01, 17:58:40 +0200, Nicolas Dichtel wrote: > Le 01/10/2020 à 09:59, Sabrina Dubroca a écrit : > > Currently, we're not advertising link-netnsid when dumping IPv6 > > addresses, so the "ip -6 addr" command will not correctly interpret > > the value of the IFLA_LINK attribute. > > > > For example, we'll get: > > 9: macvlan0@macvlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP qlen 1000 > > <snip> > > > > Instead of: > > 9: macvlan0@if9: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP qlen 1000 link-netns main > > <snip> > > > > ndisc_ifinfo_sysctl_change calls inet6_fill_ifinfo without rcu or > > rtnl, so I'm adding rcu_read_lock around rtnl_fill_link_netnsid. > I don't think this is needed. > ndisc_ifinfo_sysctl_change() takes a reference on the idev (with in6_dev_get(dev)). The problem is veth's get_link_net implementation, even after my change in patch 6: static struct net *veth_get_link_net(const struct net_device *dev) { struct veth_priv *priv = netdev_priv(dev); struct net_device *peer = rcu_dereference_rtnl(priv->peer); return peer ? dev_net(peer) : dev_net(dev); } These commands: ip link add type veth sysctl net.ipv6.neigh.veth0.retrans_time_ms=2000 cause this splat: [ 91.426764] ============================= [ 91.427445] WARNING: suspicious RCU usage [ 91.428129] 5.9.0-rc6-net-00331-gae48bef8808b-dirty #266 Not tainted [ 91.429209] ----------------------------- [ 91.433898] drivers/net/veth.c:1436 suspicious rcu_dereference_check() usage! [ 91.435127] other info that might help us debug this: [ 91.436515] rcu_scheduler_active = 2, debug_locks = 1 [ 91.437636] 1 lock held by sysctl/3718: [ 91.438310] #0: ffff88806488c430 (sb_writers#5){.+.+}-{0:0}, at: vfs_write+0x2a7/0x350 [ 91.439769] stack backtrace: [ 91.440552] CPU: 2 PID: 3718 Comm: sysctl Not tainted 5.9.0-rc6-net-00331-gae48bef8808b-dirty #266 [ 91.442132] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ArchLinux 1.14.0-1 04/01/2014 [ 91.443742] Call Trace: [ 91.444204] dump_stack+0x9a/0xd0 [ 91.444810] veth_get_link_net+0xa6/0xb0 [ 91.445534] rtnl_fill_link_netnsid+0xa2/0x130 [ 91.446330] ? rtnl_put_cacheinfo+0x190/0x190 [ 91.447120] ? memcpy+0x39/0x60 [ 91.447717] inet6_fill_ifinfo+0x2f7/0x480 I guess I could push the rcu_read_lock down into veth and vxcan's handlers instead of the rcu_dereference_rtnl change in patch 6 and adding this rcu_read_lock.
Le 02/10/2020 à 11:03, Sabrina Dubroca a écrit : [snip] > I guess I could push the rcu_read_lock down into veth and vxcan's > handlers instead of the rcu_dereference_rtnl change in patch 6 and > adding this rcu_read_lock. > Yes, I think it would avoid having this problem later, when someone else will use this helper.
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 688e441a8699..fb95c0227dfe 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -5533,6 +5533,7 @@ static inline size_t inet6_if_nlmsg_size(void) + nla_total_size(MAX_ADDR_LEN) /* IFLA_ADDRESS */ + nla_total_size(4) /* IFLA_MTU */ + nla_total_size(4) /* IFLA_LINK */ + + nla_total_size(4) /* IFLA_LINK_NETNSID */ + nla_total_size(1) /* IFLA_OPERSTATE */ + nla_total_size(inet6_ifla6_size()); /* IFLA_PROTINFO */ } @@ -5840,6 +5841,14 @@ static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev, nla_put_u8(skb, IFLA_OPERSTATE, netif_running(dev) ? dev->operstate : IF_OPER_DOWN)) goto nla_put_failure; + + rcu_read_lock(); + if (rtnl_fill_link_netnsid(skb, dev, dev_net(dev), GFP_ATOMIC)) { + rcu_read_unlock(); + goto nla_put_failure; + } + rcu_read_unlock(); + protoinfo = nla_nest_start_noflag(skb, IFLA_PROTINFO); if (!protoinfo) goto nla_put_failure;
Currently, we're not advertising link-netnsid when dumping IPv6 addresses, so the "ip -6 addr" command will not correctly interpret the value of the IFLA_LINK attribute. For example, we'll get: 9: macvlan0@macvlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP qlen 1000 <snip> Instead of: 9: macvlan0@if9: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP qlen 1000 link-netns main <snip> ndisc_ifinfo_sysctl_change calls inet6_fill_ifinfo without rcu or rtnl, so I'm adding rcu_read_lock around rtnl_fill_link_netnsid. Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> --- net/ipv6/addrconf.c | 9 +++++++++ 1 file changed, 9 insertions(+)