On 6/6/23 20:50, Guillaume Nault wrote:
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.
Obviously, that's bad. Mea culpa.
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.
Thank you for the additional insight. I had poor luck with Googling on
these.
I made a blunder after blunder. But it was insightful and brainstorming.
Good exercise for my little grey cells.
However, learning without making any errors appears to be simply a lot
of blunt memorising. :-/
It's good to be in an environment when one can learn from errors.
:-)
Regards,
Mirsad
