netdev - Re: [PATCH net-next] net: icmp: use input address in traceroute

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]

Message-ID: <2706bf00-c156-a71e-01f8-be64de0dd32f@gmail.com>
Date:   Thu, 31 Oct 2019 21:11:24 -0600
From:   David Ahern <dsahern@...il.com>
To:     Francesco Ruggeri <fruggeri@...sta.com>, kuznet@....inr.ac.ru,
        yoshfuji@...ux-ipv6.org, davem@...emloft.net,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: icmp: use input address in traceroute

On 10/31/19 6:44 PM, Francesco Ruggeri wrote:
> Even with icmp_errors_use_inbound_ifaddr set, traceroute returns the
> primary address of the interface the packet was received on, even if
> the path goes through a secondary address. In the example:
> 
>                     1.0.3.1/24
>  ---- 1.0.1.3/24    1.0.1.1/24 ---- 1.0.2.1/24    1.0.2.4/24 ----
>  |H1|--------------------------|R1|--------------------------|H2|
>  ----            N1            ----            N2            ----
> 
> where 1.0.3.1/24 is R1's primary address on N1, traceroute from
> H1 to H2 returns:
> 
> traceroute to 1.0.2.4 (1.0.2.4), 30 hops max, 60 byte packets
>  1  1.0.3.1 (1.0.3.1)  0.018 ms  0.006 ms  0.006 ms
>  2  1.0.2.4 (1.0.2.4)  0.021 ms  0.007 ms  0.007 ms
> 
> After applying this patch, it returns:
> 
> traceroute to 1.0.2.4 (1.0.2.4), 30 hops max, 60 byte packets
>  1  1.0.1.1 (1.0.1.1)  0.033 ms  0.007 ms  0.006 ms
>  2  1.0.2.4 (1.0.2.4)  0.011 ms  0.007 ms  0.007 ms
> 
> Original-patch-by: Bill Fenner <fenner@...sta.com>
> Signed-off-by: Francesco Ruggeri <fruggeri@...sta.com>
> 
> ---
>  net/ipv4/icmp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 4298aae74e0e..a72fbdf1fb85 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -682,7 +682,8 @@ void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
>  			dev = dev_get_by_index_rcu(net, inet_iif(skb_in));
>  
>  		if (dev)
> -			saddr = inet_select_addr(dev, 0, RT_SCOPE_LINK);
> +			saddr = inet_select_addr(dev, iph->saddr,
> +						 RT_SCOPE_LINK);
>  		else
>  			saddr = 0;
>  		rcu_read_unlock();
> 

Change looks good to me, so for that
Reviewed-by: David Ahern <dsahern@...il.com>

In this case and your ipv6 patch you have a set of commands to show this
problem and verify the fix. Please submit both in a test script under
tools/testing/selftests/net/. Also, veth pairs is a better way to
connect namespaces than macvlan on a dummy device. See any of the fib*
tests in that directory. Those all serve as good templates for a
traceroute test script.