netdev - Re: [PATCH net,v2] vrf: Use orig netdev to count Ip6InNoRoutes and a fresh route lookup when sending dest unreach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 Apr 2019 10:03:27 -0600
From:   David Ahern <dsahern@...il.com>
To:     Stephen Suryaputra <ssuryaextr@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net,v2] vrf: Use orig netdev to count Ip6InNoRoutes and a
 fresh route lookup when sending dest unreach

On 4/24/19 12:54 PM, Stephen Suryaputra wrote:
> When there is no route to an IPv6 dest addr, skb_dst(skb) points
> to loopback dev in the case of that the IP6CB(skb)->iif is
> enslaved to a vrf. This causes Ip6InNoRoutes to be incremented on the
> loopback dev. This also causes the lookup to fail on icmpv6_send() and
> the dest unreachable to not sent and Ip6OutNoRoutes gets incremented on
> the loopback dev.

The root cause is the 'special' dst entries ipv6 has. I want to convert
those to generated ones like IPv4 has in which case the vrf device is
used over the loopback. The changes for nexthops move the code in the
right direction for this to happen. But that is a future change.

> 
> To reproduce:
> * Gateway configuration:
> 	ip link add dev vrf_258 type vrf table 258
> 	ip link set dev enp0s9 master vrf_258
> 	ip addr add 66:1/64 dev enp0s9
> 	ip -6 route add unreachable default metric 8192 table 258
> 	sysctl -w net.ipv6.conf.all.forwarding=1
> 	sysctl -w net.ipv6.conf.enp0s9.forwarding=1
> * Sender configuration:
> 	ip addr add 66::2/64 dev enp0s9
> 	ip -6 route add default via 66::1
> and ping 67::1 for example from the sender.

here's another self contained test case:

ip netns add foo
ip -netns foo li set lo up
ip -netns foo link add dev red up type vrf table 258
ip -6 -netns foo ro add vrf red unreachable default

ip link add veth1 type veth peer name veth2
ip link set veth2 netns foo
ip -netns foo li set veth2 vrf red up
ip li set veth1 up

ip -6 addr add 2001:d8:66::1/64 dev veth1
ip -6 -netns foo addr add 2001:d8:66::2/64 dev veth2
ip -6 ro add 2001:db8:67::64/128 via 2001:d8:66::2
ip netns exec foo sysctl -w net.ipv6.conf.veth2.forwarding=1
ping6 -c1 -w1 -I veth1 2001:db8:67::64
# fails  Destination unreachable: No route

ip -netns foo -6 ru add pref 66 to 2001:db8:67::64 prohibit
ping6 -c1 -w1 -I veth1 2001:db8:67::64
# Destination unreachable: Administratively prohibited

I am adding the above to my vrf tests.

> 
> Fix this by counting on the original netdev and reset the skb dst to
> force a fresh lookup.
> 
> v2: Fix typo of destination address in the repro steps.
> Signed-off-by: Stephen Suryaputra <ssuryaextr@...il.com>
> ---
>  net/ipv6/route.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index e8c73b7782cd..3b026a310b3e 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3772,23 +3772,33 @@ int ipv6_route_ioctl(struct net *net, unsigned int cmd, void __user *arg)
>  
>  static int ip6_pkt_drop(struct sk_buff *skb, u8 code, int ipstats_mib_noroutes)
>  {
> -	int type;
>  	struct dst_entry *dst = skb_dst(skb);
> +	struct inet6_dev *idev = ip6_dst_idev(dst);
> +	struct net *net = dev_net(dst->dev);
> +	int type;
> +
> +	if (netif_is_l3_master(skb->dev) &&
> +	    idev == __in6_dev_get(net->loopback_dev)) {

dst->dev == net->loopback_dev
should work as well.

> +		idev = __in6_dev_get_safely(dev_get_by_index_rcu(net, IP6CB(skb)->iif));  

theoretically this function is called for the output path (dst->output)
as well in which case iif is not set. But, I was not able to create a
test case that hit this function for it; local traffic is failing much
sooner (connect, sendmsg).

Reviewed-by: David Ahern <dsahern@...il.com>
Tested-by: David Ahern <dsahern@...il.com>