On Tue, Jun 06, 2023 at 04:28:02PM +0200, Mirsad Todorovac wrote:
On 6/6/23 16:11, Guillaume Nault wrote:
On Tue, Jun 06, 2023 at 03:57:35PM +0200, Mirsad Todorovac wrote:
if (oif) {rcu_read_lock();dev = dev_get_by_index_rcu(net, oif);rcu_read_unlock();You can't assume '*dev' is still valid after rcu_read_unlock() unless you hold a reference on it.
rtnl_lock();mdev = netdev_master_upper_dev_get(dev);rtnl_unlock();Because of that, 'dev' might have already disappeared at the time netdev_master_upper_dev_get() is called. So it may dereference an invalid pointer here.
Good point, thanks. I didn't expect those to change.
This can be fixed, provided that RCU and RTNL locks can be nested:
Well, yes and no. You can call rcu_read_{lock,unlock}() while under the rtnl protection, but not the other way around.
rcu_read_lock(); if (oif) { dev = dev_get_by_index_rcu(net, oif); rtnl_lock(); mdev = netdev_master_upper_dev_get(dev); rtnl_unlock(); }
This is invalid: rtnl_lock() uses a mutex, so it can sleep and that's forbidden inside an RCU critical section.
if (sk->sk_bound_dev_if) { bdev = dev_get_by_index_rcu(net, sk->sk_bound_dev_if); } addr_type = ipv6_addr_type(daddr); if ((__ipv6_addr_needs_scope_id(addr_type) && !oif) || (addr_type & IPV6_ADDR_MAPPED) || (oif && sk->sk_bound_dev_if && oif != sk->sk_bound_dev_if && !(mdev && sk->sk_bound_dev_if && bdev && mdev == bdev))) { rcu_read_unlock(); return -EINVAL;} rcu_read_unlock();
But again this is still probably not race-free (bdev might also disappear before the mdev == bdev test), even if it passed fcnal-test.sh, there is much duplication of code, so your one-line solution is obviously by far better. :-)
The real problem is choosing the right function for getting the master device. In particular netdev_master_upper_dev_get() was a bad choice. It forces you to take the rtnl, which is unnatural here and obliges you to add extra code, while all this shouldn't be necessary in the first place.
Much obliged.
Best regards, Mirsad