[PATCH net] ping: fix address binding wrt vrf

[PATCH net] ping: fix address binding wrt vrf

[Nicolas Dichtel]

When ping_group_range is updated, 'ping' uses the DGRAM ICMP socket,
instead of an IP raw socket. In this case, 'ping' is unable to bind its
socket to a local address owned by a vrflite.

Before the patch:
$ sysctl -w net.ipv4.ping_group_range='0  2147483647'
$ ip link add blue type vrf table 10
$ ip link add foo type dummy
$ ip link set foo master blue
$ ip link set foo up
$ ip addr add 192.168.1.1/24 dev foo
$ ip vrf exec blue ping -c1 -I 192.168.1.1 192.168.1.2
ping: bind: Cannot assign requested address

Fixes: 1b69c6d0ae90 ("net: Introduce L3 Master device abstraction")
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv4/ping.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 3ee947557b88..9ea326b50775 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -305,6 +305,7 @@ static int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk,
 	struct net *net = sock_net(sk);
 	if (sk->sk_family == AF_INET) {
 		struct sockaddr_in *addr = (struct sockaddr_in *) uaddr;
+		u32 tb_id = RT_TABLE_LOCAL;
 		int chk_addr_ret;
 
 		if (addr_len < sizeof(*addr))
@@ -318,7 +319,8 @@ static int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk,
 		pr_debug("ping_check_bind_addr(sk=%p,addr=%pI4,port=%d)\n",
 			 sk, &addr->sin_addr.s_addr, ntohs(addr->sin_port));
 
-		chk_addr_ret = inet_addr_type(net, addr->sin_addr.s_addr);
+		tb_id = l3mdev_fib_table_by_index(net, sk->sk_bound_dev_if) ? : tb_id;
+		chk_addr_ret = inet_addr_type_table(net, addr->sin_addr.s_addr, tb_id);
 
 		if (!inet_addr_valid_or_nonlocal(net, inet_sk(sk),
 					         addr->sin_addr.s_addr,
-- 
2.33.0

[PATCH net v2] ping: fix address binding wrt vrf

[Nicolas Dichtel]

When ping_group_range is updated, 'ping' uses the DGRAM ICMP socket,
instead of an IP raw socket. In this case, 'ping' is unable to bind its
socket to a local address owned by a vrflite.

Before the patch:
$ sysctl -w net.ipv4.ping_group_range='0  2147483647'
$ ip link add blue type vrf table 10
$ ip link add foo type dummy
$ ip link set foo master blue
$ ip link set foo up
$ ip addr add 192.168.1.1/24 dev foo
$ ip vrf exec blue ping -c1 -I 192.168.1.1 192.168.1.2
ping: bind: Cannot assign requested address

CC: stable@vger.kernel.org
Fixes: 1b69c6d0ae90 ("net: Introduce L3 Master device abstraction")
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

v1 -> v2:
 add the tag "Cc: stable@vger.kernel.org" for correct stable submission

 net/ipv4/ping.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 3ee947557b88..9ea326b50775 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -305,6 +305,7 @@ static int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk,
 	struct net *net = sock_net(sk);
 	if (sk->sk_family == AF_INET) {
 		struct sockaddr_in *addr = (struct sockaddr_in *) uaddr;
+		u32 tb_id = RT_TABLE_LOCAL;
 		int chk_addr_ret;
 
 		if (addr_len < sizeof(*addr))
@@ -318,7 +319,8 @@ static int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk,
 		pr_debug("ping_check_bind_addr(sk=%p,addr=%pI4,port=%d)\n",
 			 sk, &addr->sin_addr.s_addr, ntohs(addr->sin_port));
 
-		chk_addr_ret = inet_addr_type(net, addr->sin_addr.s_addr);
+		tb_id = l3mdev_fib_table_by_index(net, sk->sk_bound_dev_if) ? : tb_id;
+		chk_addr_ret = inet_addr_type_table(net, addr->sin_addr.s_addr, tb_id);
 
 		if (!inet_addr_valid_or_nonlocal(net, inet_sk(sk),
 					         addr->sin_addr.s_addr,
-- 
2.33.0

Re: [PATCH net v2] ping: fix address binding wrt vrf

[David Ahern]

On 4/29/22 2:20 AM, Nicolas Dichtel wrote:
> When ping_group_range is updated, 'ping' uses the DGRAM ICMP socket,
> instead of an IP raw socket. In this case, 'ping' is unable to bind its
> socket to a local address owned by a vrflite.
> 
> Before the patch:
> $ sysctl -w net.ipv4.ping_group_range='0  2147483647'
> $ ip link add blue type vrf table 10
> $ ip link add foo type dummy
> $ ip link set foo master blue
> $ ip link set foo up
> $ ip addr add 192.168.1.1/24 dev foo
> $ ip vrf exec blue ping -c1 -I 192.168.1.1 192.168.1.2
> ping: bind: Cannot assign requested address
> 
> CC: stable@vger.kernel.org
> Fixes: 1b69c6d0ae90 ("net: Introduce L3 Master device abstraction")
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
> 
> v1 -> v2:
>  add the tag "Cc: stable@vger.kernel.org" for correct stable submission
> 
>  net/ipv4/ping.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

please add a test case to fcnal-test.sh. Does ipv6 work ok?

Re: [PATCH net v2] ping: fix address binding wrt vrf

[Nicolas Dichtel]

Le 29/04/2022 à 16:31, David Ahern a écrit :
> On 4/29/22 2:20 AM, Nicolas Dichtel wrote:
>> When ping_group_range is updated, 'ping' uses the DGRAM ICMP socket,
>> instead of an IP raw socket. In this case, 'ping' is unable to bind its
>> socket to a local address owned by a vrflite.
>>
>> Before the patch:
>> $ sysctl -w net.ipv4.ping_group_range='0  2147483647'
>> $ ip link add blue type vrf table 10
>> $ ip link add foo type dummy
>> $ ip link set foo master blue
>> $ ip link set foo up
>> $ ip addr add 192.168.1.1/24 dev foo
>> $ ip vrf exec blue ping -c1 -I 192.168.1.1 192.168.1.2
>> ping: bind: Cannot assign requested address
>>
>> CC: stable@vger.kernel.org
>> Fixes: 1b69c6d0ae90 ("net: Introduce L3 Master device abstraction")
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>>
>> v1 -> v2:
>>  add the tag "Cc: stable@vger.kernel.org" for correct stable submission
>>
>>  net/ipv4/ping.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
> 
> please add a test case to fcnal-test.sh. Does ipv6 work ok?
Indeed, ipv6 is missing.

I will add some test cases.
Modifying the sysctl before the vrf tests produce a lot of failures:

With VRF

SYSCTL: net.ipv4.raw_l3mdev_accept=1

SYSCTL: net.ipv4.ping_group_range=0 2147483647

TEST: ping out, VRF bind - ns-B IP                                        [ OK ]
TEST: ping out, device bind - ns-B IP                                     [FAIL]
TEST: ping out, vrf device + dev address bind - ns-B IP                   [FAIL]
TEST: ping out, vrf device + dev address bind - ns-B IP                   [FAIL]
TEST: ping out, vrf device + vrf address bind - ns-B IP                   [FAIL]
TEST: ping out, VRF bind - ns-B loopback IP                               [ OK ]
TEST: ping out, device bind - ns-B loopback IP                            [FAIL]
TEST: ping out, vrf device + dev address bind - ns-B loopback IP          [FAIL]
TEST: ping out, vrf device + dev address bind - ns-B loopback IP          [FAIL]
TEST: ping out, vrf device + vrf address bind - ns-B loopback IP          [FAIL]


Regards,
Nicolas

[PATCH net v3 0/2] vrf: fix address binding with icmp socket

[Nicolas Dichtel]

The first patch fixes the issue.
The second patch adds related tests in selftests.

v2 -> v3:
 update seltests
 fix ipv6

v1 -> v2:
 add the tag "Cc: stable@vger.kernel.org" for correct stable submission

 net/ipv4/ping.c                           | 12 +++++++++++-
 tools/testing/selftests/net/fcnal-test.sh | 12 ++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

Comments are welcome,
Nicolas

[PATCH net v3 1/2] ping: fix address binding wrt vrf

[Nicolas Dichtel]

When ping_group_range is updated, 'ping' uses the DGRAM ICMP socket,
instead of an IP raw socket. In this case, 'ping' is unable to bind its
socket to a local address owned by a vrflite.

Before the patch:
$ sysctl -w net.ipv4.ping_group_range='0  2147483647'
$ ip link add blue type vrf table 10
$ ip link add foo type dummy
$ ip link set foo master blue
$ ip link set foo up
$ ip addr add 192.168.1.1/24 dev foo
$ ip addr add 2001::1/64 dev foo
$ ip vrf exec blue ping -c1 -I 192.168.1.1 192.168.1.2
ping: bind: Cannot assign requested address
$ ip vrf exec blue ping6 -c1 -I 2001::1 2001::2
ping6: bind icmp socket: Cannot assign requested address

CC: stable@vger.kernel.org
Fixes: 1b69c6d0ae90 ("net: Introduce L3 Master device abstraction")
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv4/ping.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 3ee947557b88..aa9a11b20d18 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -305,6 +305,7 @@ static int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk,
 	struct net *net = sock_net(sk);
 	if (sk->sk_family == AF_INET) {
 		struct sockaddr_in *addr = (struct sockaddr_in *) uaddr;
+		u32 tb_id = RT_TABLE_LOCAL;
 		int chk_addr_ret;
 
 		if (addr_len < sizeof(*addr))
@@ -318,7 +319,8 @@ static int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk,
 		pr_debug("ping_check_bind_addr(sk=%p,addr=%pI4,port=%d)\n",
 			 sk, &addr->sin_addr.s_addr, ntohs(addr->sin_port));
 
-		chk_addr_ret = inet_addr_type(net, addr->sin_addr.s_addr);
+		tb_id = l3mdev_fib_table_by_index(net, sk->sk_bound_dev_if) ? : tb_id;
+		chk_addr_ret = inet_addr_type_table(net, addr->sin_addr.s_addr, tb_id);
 
 		if (!inet_addr_valid_or_nonlocal(net, inet_sk(sk),
 					         addr->sin_addr.s_addr,
@@ -355,6 +357,14 @@ static int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk,
 				return -ENODEV;
 			}
 		}
+
+		if (!dev && sk->sk_bound_dev_if) {
+			dev = dev_get_by_index_rcu(net, sk->sk_bound_dev_if);
+			if (!dev) {
+				rcu_read_unlock();
+				return -ENODEV;
+			}
+		}
 		has_addr = pingv6_ops.ipv6_chk_addr(net, &addr->sin6_addr, dev,
 						    scoped);
 		rcu_read_unlock();
-- 
2.33.0

[PATCH net v3 2/2] selftests: add ping test with ping_group_range tuned

[Nicolas Dichtel]

The 'ping' utility is able to manage two kind of sockets (raw or icmp),
depending on the sysctl ping_group_range. By default, ping_group_range is
set to '1 0', which forces ping to use an ip raw socket.

Let's replay the ping tests by allowing 'ping' to use the ip icmp socket.
After the previous patch, ipv4 tests results are the same with both kinds
of socket. For ipv6, there are a lot a new failures (the previous patch
fixes only two cases).

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 tools/testing/selftests/net/fcnal-test.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/testing/selftests/net/fcnal-test.sh b/tools/testing/selftests/net/fcnal-test.sh
index 47c4d4b4a44a..54701c8b0cd7 100755
--- a/tools/testing/selftests/net/fcnal-test.sh
+++ b/tools/testing/selftests/net/fcnal-test.sh
@@ -810,10 +810,16 @@ ipv4_ping()
 	setup
 	set_sysctl net.ipv4.raw_l3mdev_accept=1 2>/dev/null
 	ipv4_ping_novrf
+	setup
+	set_sysctl net.ipv4.ping_group_range='0 2147483647' 2>/dev/null
+	ipv4_ping_novrf
 
 	log_subsection "With VRF"
 	setup "yes"
 	ipv4_ping_vrf
+	setup "yes"
+	set_sysctl net.ipv4.ping_group_range='0 2147483647' 2>/dev/null
+	ipv4_ping_vrf
 }
 
 ################################################################################
@@ -2348,10 +2354,16 @@ ipv6_ping()
 	log_subsection "No VRF"
 	setup
 	ipv6_ping_novrf
+	setup
+	set_sysctl net.ipv4.ping_group_range='0 2147483647' 2>/dev/null
+	ipv6_ping_novrf
 
 	log_subsection "With VRF"
 	setup "yes"
 	ipv6_ping_vrf
+	setup "yes"
+	set_sysctl net.ipv4.ping_group_range='0 2147483647' 2>/dev/null
+	ipv6_ping_vrf
 }
 
 ################################################################################
-- 
2.33.0

Re: [PATCH net v3 1/2] ping: fix address binding wrt vrf

[David Ahern]

On 5/4/22 2:07 AM, Nicolas Dichtel wrote:
> When ping_group_range is updated, 'ping' uses the DGRAM ICMP socket,
> instead of an IP raw socket. In this case, 'ping' is unable to bind its
> socket to a local address owned by a vrflite.
> 
> Before the patch:
> $ sysctl -w net.ipv4.ping_group_range='0  2147483647'
> $ ip link add blue type vrf table 10
> $ ip link add foo type dummy
> $ ip link set foo master blue
> $ ip link set foo up
> $ ip addr add 192.168.1.1/24 dev foo
> $ ip addr add 2001::1/64 dev foo
> $ ip vrf exec blue ping -c1 -I 192.168.1.1 192.168.1.2
> ping: bind: Cannot assign requested address
> $ ip vrf exec blue ping6 -c1 -I 2001::1 2001::2
> ping6: bind icmp socket: Cannot assign requested address
> 
> CC: stable@vger.kernel.org
> Fixes: 1b69c6d0ae90 ("net: Introduce L3 Master device abstraction")
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  net/ipv4/ping.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net v3 2/2] selftests: add ping test with ping_group_range tuned

[David Ahern]

On 5/4/22 2:07 AM, Nicolas Dichtel wrote:
> The 'ping' utility is able to manage two kind of sockets (raw or icmp),
> depending on the sysctl ping_group_range. By default, ping_group_range is
> set to '1 0', which forces ping to use an ip raw socket.
> 
> Let's replay the ping tests by allowing 'ping' to use the ip icmp socket.
> After the previous patch, ipv4 tests results are the same with both kinds
> of socket. For ipv6, there are a lot a new failures (the previous patch

I'll take a look at those when I get a few minutes.

> fixes only two cases).
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  tools/testing/selftests/net/fcnal-test.sh | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net v3 0/2] vrf: fix address binding with icmp socket

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  4 May 2022 11:07:37 +0200 you wrote:
> The first patch fixes the issue.
> The second patch adds related tests in selftests.
> 
> v2 -> v3:
>  update seltests
>  fix ipv6
> 
> [...]

Here is the summary with links:
  - [net,v3,1/2] ping: fix address binding wrt vrf
    https://git.kernel.org/netdev/net/c/e1a7ac6f3ba6
  - [net,v3,2/2] selftests: add ping test with ping_group_range tuned
    https://git.kernel.org/netdev/net/c/e71b7f1f44d3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html