Re: [PATCH v2] net-next: Fix IP_UNICAST_IF option behavior for connected sockets

[David Ahern <dsahern@kernel.org>]

On 8/29/22 5:18 AM, Richard Gobert wrote:
> The IP_UNICAST_IF socket option is used to set the outgoing interface
> for outbound packets.
> 
> The IP_UNICAST_IF socket option was added as it was needed by the
> Wine project, since no other existing option (SO_BINDTODEVICE socket
> option, IP_PKTINFO socket option or the bind function) provided the
> needed characteristics needed by the IP_UNICAST_IF socket option. [1]
> The IP_UNICAST_IF socket option works well for unconnected sockets,
> that is, the interface specified by the IP_UNICAST_IF socket option
> is taken into consideration in the route lookup process when a packet
> is being sent. However, for connected sockets, the outbound interface
> is chosen when connecting the socket, and in the route lookup process
> which is done when a packet is being sent, the interface specified by
> the IP_UNICAST_IF socket option is being ignored.
> 
> This inconsistent behavior was reported and discussed in an issue
> opened on systemd's GitHub project [2]. Also, a bug report was
> submitted in the kernel's bugzilla [3].
> 
> To understand the problem in more detail, we can look at what happens
> for UDP packets over IPv4 (The same analysis was done separately in
> the referenced systemd issue).
> When a UDP packet is sent the udp_sendmsg function gets called and
> the following happens:
> 
> 1. The oif member of the struct ipcm_cookie ipc (which stores the
> output interface of the packet) is initialized by the ipcm_init_sk
> function to inet->sk.sk_bound_dev_if (the device set by the
> SO_BINDTODEVICE socket option).
> 
> 2. If the IP_PKTINFO socket option was set, the oif member gets
> overridden by the call to the ip_cmsg_send function.
> 
> 3. If no output interface was selected yet, the interface specified
> by the IP_UNICAST_IF socket option is used.
> 
> 4. If the socket is connected and no destination address is
> specified in the send function, the struct ipcm_cookie ipc is not
> taken into consideration and the cached route, that was calculated in
> the connect function is being used.
> 
> Thus, for a connected socket, the IP_UNICAST_IF sockopt isn't taken
> into consideration.
> 
> This patch corrects the behavior of the IP_UNICAST_IF socket option
> for connect()ed sockets by taking into consideration the
> IP_UNICAST_IF sockopt when connecting the socket.
> 
> In order to avoid reconnecting the socket, this option is still
> ignored when applied on an already connected socket until connect()
> is called again by the Richard Gobert.
> 
> Change the __ip4_datagram_connect function, which is called during
> socket connection, to take into consideration the interface set by
> the IP_UNICAST_IF socket option, in a similar way to what is done in
> the udp_sendmsg function.
> 
> [1] https://lore.kernel.org/netdev/1328685717.4736.4.camel@edumazet-laptop/T/
> [2] https://github.com/systemd/systemd/issues/11935#issuecomment-618691018
> [3] https://bugzilla.kernel.org/show_bug.cgi?id=210255
> 
> Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
> ---
> v1 -> v2: Added self-tests and targeted to net-next.
> 
>  net/ipv4/datagram.c                       |  2 ++
>  tools/testing/selftests/net/fcnal-test.sh | 30 +++++++++++++++++++++++
>  tools/testing/selftests/net/nettest.c     | 16 ++++++++++--
>  3 files changed, 46 insertions(+), 2 deletions(-)
> 

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

> diff --git a/tools/testing/selftests/net/fcnal-test.sh b/tools/testing/selftests/net/fcnal-test.sh
> index 03b586760164..31c3b6ebd388 100755
> --- a/tools/testing/selftests/net/fcnal-test.sh
> +++ b/tools/testing/selftests/net/fcnal-test.sh
> @@ -1466,6 +1466,13 @@ ipv4_udp_novrf()
>  		run_cmd nettest -D -r ${a} -d ${NSA_DEV} -S -0 ${NSA_IP}
>  		log_test_addr ${a} $? 0 "Client, device bind via IP_UNICAST_IF"
>  
> +		log_start
> +		run_cmd_nsb nettest -D -s &
> +		sleep 1
> +		run_cmd nettest -D -r ${a} -d ${NSA_DEV} -S -0 ${NSA_IP} -U
> +		log_test_addr ${a} $? 0 "Client, device bind via IP_UNICAST_IF, with connect()"
> +
> +
>  		log_start
>  		show_hint "Should fail 'Connection refused'"
>  		run_cmd nettest -D -r ${a}
> @@ -1525,6 +1532,13 @@ ipv4_udp_novrf()
>  	run_cmd nettest -D -d ${NSA_DEV} -S -r ${a}
>  	log_test_addr ${a} $? 0 "Global server, device client via IP_UNICAST_IF, local connection"
>  
> +	log_start
> +	run_cmd nettest -s -D &
> +	sleep 1
> +	run_cmd nettest -D -d ${NSA_DEV} -S -r ${a} -U
> +	log_test_addr ${a} $? 0 "Global server, device client via IP_UNICAST_IF, local connection, with connect()"
> +
> +
>  	# IPv4 with device bind has really weird behavior - it overrides the
>  	# fib lookup, generates an rtable and tries to send the packet. This
>  	# causes failures for local traffic at different places
> @@ -1550,6 +1564,15 @@ ipv4_udp_novrf()
>  		sleep 1
>  		run_cmd nettest -D -r ${a} -d ${NSA_DEV} -S
>  		log_test_addr ${a} $? 1 "Global server, device client via IP_UNICAST_IF, local connection"
> +
> +		log_start
> +		show_hint "Should fail since addresses on loopback are out of device scope"
> +		run_cmd nettest -D -s &
> +		sleep 1
> +		run_cmd nettest -D -r ${a} -d ${NSA_DEV} -S -U
> +		log_test_addr ${a} $? 1 "Global server, device client via IP_UNICAST_IF, local connection, with connect()"
> +
> +
>  	done
>  
>  	a=${NSA_IP}
> @@ -3157,6 +3180,13 @@ ipv6_udp_novrf()
>  		sleep 1
>  		run_cmd nettest -6 -D -r ${a} -d ${NSA_DEV} -S
>  		log_test_addr ${a} $? 1 "Global server, device client via IP_UNICAST_IF, local connection"
> +
> +		log_start
> +		show_hint "Should fail 'No route to host' since addresses on loopback are out of device scope"
> +		run_cmd nettest -6 -D -s &
> +		sleep 1
> +		run_cmd nettest -6 -D -r ${a} -d ${NSA_DEV} -S -U
> +		log_test_addr ${a} $? 1 "Global server, device client via IP_UNICAST_IF, local connection, with connect()"
>  	done
>  
>  	a=${NSA_IP6}

All of the tests are 'novrf' but this should work with VRF too. The
device is in a VRF, so binding it to should. Please add these tests
again for the VRF cases. It can be a followup patch.