From: Lorenzo Colitti <hidden> Date: 2016-08-08 07:42:21
ping_v6_sendmsg never sets flowi6_oif, so it is not possible to
ping an IPv6 address on a different interface. Instead, it sets
flowi6_iif, which is incorrect but harmless. Also, it returns an
error if a passed-in scope ID doesn't match sk_bound_dev_if.
Get rid of the error, stop setting flowi6_iif, and support
various ways of setting oif in the same priority order used by
udpv6_sendmsg.
Tested: https://android-review.googlesource.com/#/c/254470/
Signed-off-by: Lorenzo Colitti <redacted>
---
net/ipv6/ping.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
From: Hannes Frederic Sowa <hidden> Date: 2016-08-08 14:26:49
On 08.08.2016 09:42, Lorenzo Colitti wrote:
quoted hunk
ping_v6_sendmsg never sets flowi6_oif, so it is not possible to
ping an IPv6 address on a different interface. Instead, it sets
flowi6_iif, which is incorrect but harmless. Also, it returns an
error if a passed-in scope ID doesn't match sk_bound_dev_if.
Get rid of the error, stop setting flowi6_iif, and support
various ways of setting oif in the same priority order used by
udpv6_sendmsg.
Tested: https://android-review.googlesource.com/#/c/254470/
Signed-off-by: Lorenzo Colitti <redacted>
---
net/ipv6/ping.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
Hmm, sk->sk_bound_dev_if always has highest prio for the selection of
the output interface. Thus this code made sense to me.
Removing it is consistent with the other sendmsg functions such as
udpv6_sendmsg or rawv6_sendmsg.
There is similar code in __ip6_datagram_connect, but that seems a bit
different because that code also *sets* sk_bound_dev_if.
Personally I think it's better for pingv6_sendmsg be consistent with
the other *_sendmsg functions than with ip6_datagram_connect, and thus
the code should be removed. But I don't feel particularly strongly
about it.
From: David Ahern <hidden> Date: 2016-08-08 15:27:20
On 8/8/16 1:42 AM, Lorenzo Colitti wrote:
quoted hunk
ping_v6_sendmsg never sets flowi6_oif, so it is not possible to
ping an IPv6 address on a different interface. Instead, it sets
flowi6_iif, which is incorrect but harmless. Also, it returns an
error if a passed-in scope ID doesn't match sk_bound_dev_if.
Get rid of the error, stop setting flowi6_iif, and support
various ways of setting oif in the same priority order used by
udpv6_sendmsg.
Tested: https://android-review.googlesource.com/#/c/254470/
Signed-off-by: Lorenzo Colitti <redacted>
---
net/ipv6/ping.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
Hmm, sk->sk_bound_dev_if always has highest prio for the selection of
the output interface. Thus this code made sense to me.
Removing it is consistent with the other sendmsg functions such as
udpv6_sendmsg or rawv6_sendmsg.
There is similar code in __ip6_datagram_connect, but that seems a bit
different because that code also *sets* sk_bound_dev_if.
Personally I think it's better for pingv6_sendmsg be consistent with
the other *_sendmsg functions than with ip6_datagram_connect, and thus
the code should be removed. But I don't feel particularly strongly
about it.
Following must be met, at least, IMHO.
- SO_BINDTODEVICE requires "root", which sets sk_bound_dev_if.
- sin6_scope_id and sk_bound_dev_if should match (if the address it
link-local address), or each or both should equal to 0.
I think it would make more sense if former setting wins...
--yoshfuji
From: David Ahern <hidden> Date: 2016-08-08 16:27:36
On 8/8/16 10:24 AM, Lorenzo Colitti wrote:
On Tue, Aug 9, 2016 at 12:27 AM, David Ahern [off-list ref] wrote:
quoted
quoted
- if (!fl6.flowi6_oif && ipv6_addr_is_multicast(&fl6.daddr))
- fl6.flowi6_oif = np->mcast_oif;
- else if (!fl6.flowi6_oif)
- fl6.flowi6_oif = np->ucast_oif;
-
That code removal is contrary to your patch description regarding flowi6_oif.
Which code removal? The one I quote above? That code wasn't removed,
it was moved to above the initialization of flowi6.
Your description states:
"ping_v6_sendmsg never sets flowi6_oif, so it is not possible to
ping an IPv6 address on a different interface."
That code snippet above contradicts that -- flowi6_oif is set in ping_v6_sendmsg.
You are making a different change than just setting flowi6_oif.
From: David Miller <davem@davemloft.net> Date: 2016-08-08 21:35:03
From: Lorenzo Colitti <redacted>
Date: Mon, 8 Aug 2016 16:42:07 +0900
ping_v6_sendmsg never sets flowi6_oif, so it is not possible to
ping an IPv6 address on a different interface. Instead, it sets
flowi6_iif, which is incorrect but harmless. Also, it returns an
error if a passed-in scope ID doesn't match sk_bound_dev_if.
Get rid of the error, stop setting flowi6_iif, and support
various ways of setting oif in the same priority order used by
udpv6_sendmsg.
Tested: https://android-review.googlesource.com/#/c/254470/
Signed-off-by: Lorenzo Colitti <redacted>
We should always give sk_bound_dev_if the highest priority.
Also, we should amend, not delete, the check against the scope
ID in the sockaddr. As explained by YOSHIFUJI Hideaki.
From: Lorenzo Colitti <hidden> Date: 2016-08-09 00:40:46
On Tue, Aug 9, 2016 at 1:27 AM, David Ahern [off-list ref] wrote:
Your description states:
"ping_v6_sendmsg never sets flowi6_oif, so it is not possible to
ping an IPv6 address on a different interface."
That code snippet above contradicts that -- flowi6_oif is set in ping_v6_sendmsg.
From: Lorenzo Colitti <hidden> Date: 2016-08-09 01:00:47
On Tue, Aug 9, 2016 at 6:35 AM, David Miller [off-list ref] wrote:
We should always give sk_bound_dev_if the highest priority.
Also, we should amend, not delete, the check against the scope
ID in the sockaddr. As explained by YOSHIFUJI Hideaki.
Sure, I can do that.
Note that pretty much every sendmsg codepath allows other data to take
precedence over sk_bound_dev_if:
- udpv6_sendmsg: if sin6_scope_id specified on a scoped address
- rawv6_sendmsg: if sin6_scope_id specified on a scoped address
- l2tp_ip6_sendmsg: if sin6_scope_id specified on a scoped address
- ip_cmsg_send: if IP_PKTINFO or IPV6_PKTINFO specified
What should I do about those? -EINVAL? Ignore the conflicting data? Leave as is?
From: David Miller <davem@davemloft.net> Date: 2016-08-09 05:20:37
From: Lorenzo Colitti <redacted>
Date: Tue, 9 Aug 2016 10:00:25 +0900
Note that pretty much every sendmsg codepath allows other data to take
precedence over sk_bound_dev_if:
- udpv6_sendmsg: if sin6_scope_id specified on a scoped address
- rawv6_sendmsg: if sin6_scope_id specified on a scoped address
- l2tp_ip6_sendmsg: if sin6_scope_id specified on a scoped address
- ip_cmsg_send: if IP_PKTINFO or IPV6_PKTINFO specified
What should I do about those? -EINVAL? Ignore the conflicting data? Leave as is?
That's a good point, I guess this needs some more thought.
From: Erik Kline <hidden> Date: 2016-08-09 07:01:27
On 9 August 2016 at 14:20, David Miller [off-list ref] wrote:
From: Lorenzo Colitti <redacted>
Date: Tue, 9 Aug 2016 10:00:25 +0900
quoted
Note that pretty much every sendmsg codepath allows other data to take
precedence over sk_bound_dev_if:
- udpv6_sendmsg: if sin6_scope_id specified on a scoped address
- rawv6_sendmsg: if sin6_scope_id specified on a scoped address
- l2tp_ip6_sendmsg: if sin6_scope_id specified on a scoped address
- ip_cmsg_send: if IP_PKTINFO or IPV6_PKTINFO specified
What should I do about those? -EINVAL? Ignore the conflicting data? Leave as is?
That's a good point, I guess this needs some more thought.
I could see a point of view that says when bound_if is in play sending
to destinations on/via other interfaces--by any mechanism--should
effectively get ENETUNREACH (or something).
That does seem like I would involve changing some existing behavior, though.
On 9 August 2016 at 14:20, David Miller [off-list ref] wrote:
quoted
From: Lorenzo Colitti <redacted>
Date: Tue, 9 Aug 2016 10:00:25 +0900
quoted
Note that pretty much every sendmsg codepath allows other data to take
precedence over sk_bound_dev_if:
- udpv6_sendmsg: if sin6_scope_id specified on a scoped address
- rawv6_sendmsg: if sin6_scope_id specified on a scoped address
- l2tp_ip6_sendmsg: if sin6_scope_id specified on a scoped address
- ip_cmsg_send: if IP_PKTINFO or IPV6_PKTINFO specified
What should I do about those? -EINVAL? Ignore the conflicting data? Leave as is?
That's a good point, I guess this needs some more thought.
I could see a point of view that says when bound_if is in play sending
to destinations on/via other interfaces--by any mechanism--should
effectively get ENETUNREACH (or something).
+1
That does seem like I would involve changing some existing behavior, though.
The use of sin6_scope_id and SO_BINDTODEVICE with different interfaces
is incorrect and should be rejected.
--
Hideaki Yoshifuji [off-list ref]
Technical Division, MIRACLE LINUX CORPORATION
From: Hannes Frederic Sowa <hidden> Date: 2016-08-09 09:36:13
On 09.08.2016 10:37, YOSHIFUJI Hideaki wrote:
Erik Kline wrote:
quoted
On 9 August 2016 at 14:20, David Miller [off-list ref] wrote:
quoted
From: Lorenzo Colitti <redacted>
Date: Tue, 9 Aug 2016 10:00:25 +0900
quoted
Note that pretty much every sendmsg codepath allows other data to take
precedence over sk_bound_dev_if:
- udpv6_sendmsg: if sin6_scope_id specified on a scoped address
- rawv6_sendmsg: if sin6_scope_id specified on a scoped address
- l2tp_ip6_sendmsg: if sin6_scope_id specified on a scoped address
- ip_cmsg_send: if IP_PKTINFO or IPV6_PKTINFO specified
What should I do about those? -EINVAL? Ignore the conflicting data? Leave as is?
That's a good point, I guess this needs some more thought.
I could see a point of view that says when bound_if is in play sending
to destinations on/via other interfaces--by any mechanism--should
effectively get ENETUNREACH (or something).
+1
quoted
That does seem like I would involve changing some existing behavior, though.
The use of sin6_scope_id and SO_BINDTODEVICE with different interfaces
is incorrect and should be rejected.
I agree, I would actually change the behavior at this point, as it also
could have security consequences from a network pov.
Bye,
Hannes
From: David Ahern <hidden> Date: 2016-08-09 13:21:39
On 8/9/16 1:01 AM, Erik Kline wrote:
On 9 August 2016 at 14:20, David Miller [off-list ref] wrote:
quoted
From: Lorenzo Colitti <redacted>
Date: Tue, 9 Aug 2016 10:00:25 +0900
quoted
Note that pretty much every sendmsg codepath allows other data to take
precedence over sk_bound_dev_if:
- udpv6_sendmsg: if sin6_scope_id specified on a scoped address
- rawv6_sendmsg: if sin6_scope_id specified on a scoped address
- l2tp_ip6_sendmsg: if sin6_scope_id specified on a scoped address
- ip_cmsg_send: if IP_PKTINFO or IPV6_PKTINFO specified
What should I do about those? -EINVAL? Ignore the conflicting data? Leave as is?
That's a good point, I guess this needs some more thought.
I could see a point of view that says when bound_if is in play sending
to destinations on/via other interfaces--by any mechanism--should
effectively get ENETUNREACH (or something).
VRF uses this capability to send on an enslaved interface. ie., socket is bound to VRF device to limit packets to that L3 domain and then uses PKTINFO to force a packet out a particular interface.
On 9 August 2016 at 14:20, David Miller [off-list ref] wrote:
quoted
From: Lorenzo Colitti <redacted>
Date: Tue, 9 Aug 2016 10:00:25 +0900
quoted
Note that pretty much every sendmsg codepath allows other data to take
precedence over sk_bound_dev_if:
- udpv6_sendmsg: if sin6_scope_id specified on a scoped address
- rawv6_sendmsg: if sin6_scope_id specified on a scoped address
- l2tp_ip6_sendmsg: if sin6_scope_id specified on a scoped address
- ip_cmsg_send: if IP_PKTINFO or IPV6_PKTINFO specified
What should I do about those? -EINVAL? Ignore the conflicting data? Leave as is?
That's a good point, I guess this needs some more thought.
I could see a point of view that says when bound_if is in play sending
to destinations on/via other interfaces--by any mechanism--should
effectively get ENETUNREACH (or something).
VRF uses this capability to send on an enslaved interface. ie., socket is bound to VRF device to limit packets to that L3 domain and then uses PKTINFO to force a packet out a particular interface.
We could extend our code to allow enslave devices, maybe.
--
Hideaki Yoshifuji [off-list ref]
Technical Division, MIRACLE LINUX CORPORATION
From: Lorenzo Colitti <hidden> Date: 2016-08-12 15:46:04
On Wed, Aug 10, 2016 at 7:44 AM, YOSHIFUJI Hideaki
[off-list ref] wrote:
quoted
quoted
I could see a point of view that says when bound_if is in play sending
to destinations on/via other interfaces--by any mechanism--should
effectively get ENETUNREACH (or something).
VRF uses this capability to send on an enslaved interface. ie., socket is bound to VRF device to limit packets to that L3 domain and then uses PKTINFO to force a packet out a particular interface.
We could extend our code to allow enslave devices, maybe.
So something like this, then?
static inline bool inet_check_bound_oif(const struct sock *sk, int oif)
{
if (!oif || !sk->sk_bound_dev_if || oif == sk->sk_bound_dev_if)
return true;
#ifdef CONFIG_NET_L3_MASTER_DEV
return l3mdev_master_ifindex_by_index(sock_net(sk), oif) ==
sk->sk_bound_dev_if;
#endif
return false;
}
and then in the various sendmsg functions:
if (!inet_check_bound_oif(sk, oif))
return -EINVAL;
From: David Ahern <hidden> Date: 2016-08-12 16:04:18
On 8/12/16 9:45 AM, Lorenzo Colitti wrote:
On Wed, Aug 10, 2016 at 7:44 AM, YOSHIFUJI Hideaki
[off-list ref] wrote:
quoted
quoted
quoted
I could see a point of view that says when bound_if is in play sending
to destinations on/via other interfaces--by any mechanism--should
effectively get ENETUNREACH (or something).
VRF uses this capability to send on an enslaved interface. ie., socket is bound to VRF device to limit packets to that L3 domain and then uses PKTINFO to force a packet out a particular interface.
We could extend our code to allow enslave devices, maybe.
So something like this, then?
static inline bool inet_check_bound_oif(const struct sock *sk, int oif)
{
if (!oif || !sk->sk_bound_dev_if || oif == sk->sk_bound_dev_if)
return true;
#ifdef CONFIG_NET_L3_MASTER_DEV
return l3mdev_master_ifindex_by_index(sock_net(sk), oif) ==
sk->sk_bound_dev_if;
#endif
return false;
}
The ifdef is not needed; it's already in the l3mdev macros.
int midx;
if (!oif || !sk->sk_bound_dev_if || oif == sk->sk_bound_dev_if)
return true;
midx = l3mdev_master_ifindex_by_index(sock_net(sk), oif);
if (midx && sk->sk_bound_dev_if == midx)
return true;
return false;
and then in the various sendmsg functions:
if (!inet_check_bound_oif(sk, oif))
return -EINVAL;
On Wed, Aug 10, 2016 at 7:44 AM, YOSHIFUJI Hideaki
[off-list ref] wrote:
quoted
quoted
quoted
I could see a point of view that says when bound_if is in play sending
to destinations on/via other interfaces--by any mechanism--should
effectively get ENETUNREACH (or something).
VRF uses this capability to send on an enslaved interface. ie., socket is bound to VRF device to limit packets to that L3 domain and then uses PKTINFO to force a packet out a particular interface.
We could extend our code to allow enslave devices, maybe.
So something like this, then?
static inline bool inet_check_bound_oif(const struct sock *sk, int oif)
{
if (!oif || !sk->sk_bound_dev_if || oif == sk->sk_bound_dev_if)
return true;
#ifdef CONFIG_NET_L3_MASTER_DEV
return l3mdev_master_ifindex_by_index(sock_net(sk), oif) ==
sk->sk_bound_dev_if;
#endif
return false;
}
and then in the various sendmsg functions:
if (!inet_check_bound_oif(sk, oif))
return -EINVAL;
From: Lorenzo Colitti <hidden> Date: 2016-08-16 15:22:07
On Mon, Aug 15, 2016 at 4:39 PM, YOSHIFUJI Hideaki
[off-list ref] wrote:
quoted
and then in the various sendmsg functions:
if (!inet_check_bound_oif(sk, oif))
return -EINVAL;
Yes, something like that.
There's another complication. inet6_bind and raw_bind take
sin6_scope_id and assign it to sk_bound_dev_if:
if (addr_len >= sizeof(struct sockaddr_in6) &&
addr->sin6_scope_id) {
if (addr->sin6_scope_id != sk
/* Override any existing binding, if another
* one is supplied by user.
*/
sk->sk_bound_dev_if = addr->sin6_scope_id;
}
The reason they do this is that the only place in the socket to
score the scope ID is sk_bound_dev_if. The scope ID has to be stored
in the socket, because it's the only way to ensure the semantics of
scoped addresses, where the address without the scope ID is not
unique, and thus the scope ID is effectively part of the address. For
example:
1. A socket bound to fe80::1%eth0 and a socket bound to fe80::1%wlan0
must never see each other's packets. This means that things like
udp6_lib_lookup must take the scope ID into account.
2. Calling getpeername() on a socket that's bound to fe80::1%eth0 must
return eth0's ifindex in sin6_scope_id.
Unless we add a scope ID field to the socket, changing this behaviour
would cause substantial breakage. It's perfectly legal to bind a
socket to fe80::1%eth0 and then fe80::2%wlan0, for example. So we
can't just say that sk_bound_dev_if must always take precedence on
sin6_scope_id.
I also don't see how the VRF behaviour where sk_bound_dev_if sets the
master interface and pktinto selects the slave interface" can be made
to work at all in the presence of scoped addresses. I don't see any
way to support a socket bound to fe80::1%eth0 and a socket bound to
fe80::1%wlan0 in the same VRF.
From: David Ahern <hidden> Date: 2016-08-16 16:22:32
On 8/16/16 9:21 AM, Lorenzo Colitti wrote:
I also don't see how the VRF behaviour where sk_bound_dev_if sets the
master interface and pktinto selects the slave interface" can be made
to work at all in the presence of scoped addresses. I don't see any
way to support a socket bound to fe80::1%eth0 and a socket bound to
fe80::1%wlan0 in the same VRF.
It does not work with sk_bound_dev_if set; works fine without it.