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

[David Ahern <dsahern@kernel.org>]

On 7/6/22 10:21 AM, Paolo Abeni wrote:
> On Wed, 2022-07-06 at 09:14 -0600, David Ahern wrote:
>> On 6/27/22 2:52 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 user.
>>>
>>> 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>
>>> ---
>>>  net/ipv4/datagram.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>
>> Reviewed-by: David Ahern <dsahern@kernel.org>
>>
>> if the maintainers decide to pick it up.
> 
> I think your reasoning is correct, and I'm now ok with the patch. Jakub
> noted it does not apply cleanly, so a repost will be needed.
> Additionally it would be great to include some self-tests.

Agreed. nettest.c has '-S' option that sets IP_UNICAST_IF after the
socket() call and before connect() so it is already doing what is wanted
by this patch. Just need positive and negative test cases added to
tools/testing/selftests/net.


> 
> It looks like the feature (even the original one, I mean) is IPv4
> specific, don't you need an IPv6 counter-part?
> 
> Thanks!
> 
> Paolo
>