| Message ID | 20220627085219.GA9597@debian (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Fix IP_UNICAST_IF option behavior for connected sockets | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Mon, 2022-06-27 at 10:52 +0200, 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. This also changes a long-established behavior for such socket option. It can break existing application assuming connect() is not affected by IP_UNICAST_IF. I'm unsure we can accept it. Cheers, Paolo
On Tue, Jun 28, 2022 at 03:08:48PM +0200, Paolo Abeni wrote: > This also changes a long-established behavior for such socket option. > It can break existing application assuming connect() is not affected by > IP_UNICAST_IF. I'm unsure we can accept it. The IP_UNICAST_IF option was initially introduced for better compatibility with the matching Windows socket-option. Its goal was better support for wine applications. This patch improves the compatibility even further since Windows behaves this way for connect()ed sockets. Also, I have not been able to find any examples of Linux applications that use IP_UNICAST_IF with connect(). It would be quite confusing to use this sockopt and expect that it would not affect your socket. I think that unless someone finds an example of such a use case, then it is better to accept this patch to improve compatibility for applications that run with wine. What are your thoughts on this? Regards, Richard
On 7/5/22 9:50 AM, Richard Gobert wrote: > On Tue, Jun 28, 2022 at 03:08:48PM +0200, Paolo Abeni wrote: >> This also changes a long-established behavior for such socket option. >> It can break existing application assuming connect() is not affected by >> IP_UNICAST_IF. I'm unsure we can accept it. > > The IP_UNICAST_IF option was initially introduced for better compatibility > with the matching Windows socket-option. Its goal was better support for > wine applications. > This patch improves the compatibility even further since Windows behaves > this way for connect()ed sockets. > > Also, I have not been able to find any examples of Linux applications > that use IP_UNICAST_IF with connect(). It would be quite confusing to use > this sockopt and expect that it would not affect your socket. > I think that unless someone finds an example of such a use case, then it > is better to accept this patch to improve compatibility for applications > that run with wine. > > What are your thoughts on this? > I can't imagine how a 'connected' socket would propelry work if connect path does not consider oif and then per message does. i.e, i think the patch has some risk but is the right thing to do.
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.
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. It looks like the feature (even the original one, I mean) is IPv4 specific, don't you need an IPv6 counter-part? Thanks! Paolo
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 >
On Wed, Jul 06, 2022 at 06:21:05PM +0200, Paolo Abeni wrote: > 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. Will include self-tests and submit V2. The patch applies cleanly in my local setup. Do you have an idea as to why this might happen? I missed the email where Jakub mentioned this. Thanks, Richard
On Wed, 13 Jul 2022 14:45:11 +0200 Richard Gobert wrote: > On Wed, Jul 06, 2022 at 06:21:05PM +0200, Paolo Abeni wrote: > > 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. > > Will include self-tests and submit V2. > The patch applies cleanly in my local setup. Do you have an idea as to why > this might happen? I missed the email where Jakub mentioned this. Jakub noted it in a private conversation with Paolo :) If it does indeed apply cleanly to net-next [1] please repost with the tree name explicitly stated in the subject line, ie. [PATCH net-next] to make sure our bots don't make any mistakes in tree selection for testing. [1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/
diff --git a/net/ipv4/datagram.c b/net/ipv4/datagram.c index ffd57523331f..405a8c2aea64 100644 --- a/net/ipv4/datagram.c +++ b/net/ipv4/datagram.c @@ -42,6 +42,8 @@ int __ip4_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len oif = inet->mc_index; if (!saddr) saddr = inet->mc_addr; + } else if (!oif) { + oif = inet->uc_index; } fl4 = &inet->cork.fl.u.ip4; rt = ip_route_connect(fl4, usin->sin_addr.s_addr, saddr,
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(+)