Re: POSSIBLE BUG: selftests/net/fcnal-test.sh: [FAIL] in vrf "bind - ns-B IPv6 LLA" test

[Mirsad Todorovac]

On 6/6/23 15:46, Guillaume Nault wrote:
> On Tue, Jun 06, 2023 at 08:24:54AM +0200, Mirsad Goran Todorovac wrote:
> > On 5/31/23 20:11, Guillaume Nault wrote:
> > > I believe this condition should be relaxed to allow the case where
> > > ->sk_bound_dev_if is oif's master device (and maybe there are other
> > > VRF cases to also consider).
> >
> > I've tried something like this, but something makes the kernel stuck
> > here:
> >
> > TEST: ping out, blocked by route - ns-B loopback IPv6                         [ OK ]
> > TEST: ping out, device bind, blocked by route - ns-B loopback IPv6            [ OK ]
> > TEST: ping in, blocked by route - ns-A loopback IPv6                          [ OK ]
> > TEST: ping out, unreachable route - ns-B loopback IPv6                        [ OK ]
> > TEST: ping out, device bind, unreachable route - ns-B loopback IPv6           [ OK ]
> >
> > #################################################################
> > With VRF
> >
> > [hanged process and kernel won't shutdown]
> >
> > The code is:
> >
> > ---
> >   net/ipv6/ping.c | 12 +++++++++++-
> >   1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
> > index c4835dbdfcff..81293e902293 100644
> > --- a/net/ipv6/ping.c
> > +++ b/net/ipv6/ping.c
> > @@ -73,6 +73,9 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> >          struct rt6_info *rt;
> >          struct pingfakehdr pfh;
> >          struct ipcm6_cookie ipc6;
> > +       struct net *net = sock_net(sk);
> > +       struct net_device *dev = NULL;
> > +       struct net_device *mdev = NULL;
> >          err = ping_common_sendmsg(AF_INET6, msg, len, &user_icmph,
> >                                    sizeof(user_icmph));
> > @@ -111,10 +114,17 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> >          else if (!oif)
> >                  oif = np->ucast_oif;
> > +       if (oif) {
> > +               dev = dev_get_by_index(net, oif);
> > +               mdev = netdev_master_upper_dev_get(dev);
> > +       }
> > +
> >          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))
> > +           (oif && sk->sk_bound_dev_if && oif != sk->sk_bound_dev_if &&
> > +                   !(mdev && sk->sk_bound_dev_if &&
> > +                             mdev != dev_get_by_index(net, sk->sk_bound_dev_if))))
> >                  return -EINVAL;
> >          ipcm6_init_sk(&ipc6, np);
> >
> > I am obviously doing something very stupid.
>
> The problem is that dev_get_by_index() holds a reference on 'dev' which
> your code never releases. Also netdev_master_upper_dev_get() needs rtnl
> protection. These should have generated some kernel oops.
>
> You can try this instead:
>
> -------- >8 --------
>
> diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
> index c4835dbdfcff..f804c11e2146 100644
> --- a/net/ipv6/ping.c
> +++ b/net/ipv6/ping.c
> @@ -114,7 +114,8 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>   	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))
> +	    (oif && sk->sk_bound_dev_if && oif != sk->sk_bound_dev_if &&
> +	     l3mdev_master_ifindex_by_index(sock_net(sk), oif) != sk->sk_bound_dev_if))
>   		return -EINVAL;
>   
>   	ipcm6_init_sk(&ipc6, np);

Yes, I stupidly forgot that.

I came across a fixed version:

------
diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
index c4835dbdfcff..c1d81c49b775 100644
--- a/net/ipv6/ping.c
+++ b/net/ipv6/ping.c
@@ -73,6 +73,10 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
        struct rt6_info *rt;
        struct pingfakehdr pfh;
        struct ipcm6_cookie ipc6;
+       struct net *net = sock_net(sk);
+       struct net_device *dev = NULL;
+       struct net_device *mdev = NULL;
+       struct net_device *bdev = NULL;

        err = ping_common_sendmsg(AF_INET6, msg, len, &user_icmph,
                                  sizeof(user_icmph));
@@ -111,10 +115,26 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
        else if (!oif)
                oif = np->ucast_oif;

+       if (oif) {
+               rcu_read_lock();
+               dev = dev_get_by_index_rcu(net, oif);
+               rcu_read_unlock();
+               rtnl_lock();
+               mdev = netdev_master_upper_dev_get(dev);
+               rtnl_unlock();
+       }
+
+       if (sk->sk_bound_dev_if) {
+               rcu_read_lock();
+               bdev = dev_get_by_index_rcu(net, sk->sk_bound_dev_if);
+               rcu_read_unlock();
+       }
+
        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))
+           (oif && sk->sk_bound_dev_if && oif != sk->sk_bound_dev_if &&
+                   !(mdev && sk->sk_bound_dev_if && bdev && mdev == bdev)))
                return -EINVAL;

        ipcm6_init_sk(&ipc6, np);

However, this works by the test (888 passed) but your two liner is obviously
better :-)

Best regards,
Mirsad

--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu

System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia