Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-16 Thread David Ahern
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.


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-16 Thread Lorenzo Colitti
On Mon, Aug 15, 2016 at 4:39 PM, YOSHIFUJI Hideaki
 wrote:
>
> > 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.


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-15 Thread YOSHIFUJI Hideaki


Lorenzo Colitti wrote:
> On Wed, Aug 10, 2016 at 7:44 AM, YOSHIFUJI Hideaki
>  wrote:
>>
 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;
> 

Yes, something like that.

--yoshfuji


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-12 Thread David Ahern
On 8/12/16 9:45 AM, Lorenzo Colitti wrote:
> On Wed, Aug 10, 2016 at 7:44 AM, YOSHIFUJI Hideaki
>  wrote:
>>
 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;
> 




Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-12 Thread Lorenzo Colitti
On Wed, Aug 10, 2016 at 7:44 AM, YOSHIFUJI Hideaki
 wrote:
>
> >> 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;


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-09 Thread YOSHIFUJI Hideaki


David Ahern wrote:
> On 8/9/16 1:01 AM, Erik Kline wrote:
>> On 9 August 2016 at 14:20, David Miller  wrote:
>>> From: Lorenzo Colitti 
>>> 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.
>>
>> 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 
Technical Division, MIRACLE LINUX CORPORATION


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-09 Thread David Ahern
On 8/9/16 1:01 AM, Erik Kline wrote:
> On 9 August 2016 at 14:20, David Miller  wrote:
>> From: Lorenzo Colitti 
>> 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.
> 
> 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.



Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-09 Thread Lorenzo Colitti
On Tue, Aug 9, 2016 at 6:36 PM, Hannes Frederic Sowa
 wrote:
>> 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.

I can send out a patch to do that. Is it something that can be done in
net, or does it need to go into net-next?


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-09 Thread Hannes Frederic Sowa
On 09.08.2016 10:37, YOSHIFUJI Hideaki wrote:
> 
> 
> Erik Kline wrote:
>> On 9 August 2016 at 14:20, David Miller  wrote:
>>> From: Lorenzo Colitti 
>>> 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.
>>
>> 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.

I agree, I would actually change the behavior at this point, as it also
could have security consequences from a network pov.

Bye,
Hannes




Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-09 Thread YOSHIFUJI Hideaki


Erik Kline wrote:
> On 9 August 2016 at 14:20, David Miller  wrote:
>> From: Lorenzo Colitti 
>> 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.
> 
> 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 
Technical Division, MIRACLE LINUX CORPORATION


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-09 Thread Erik Kline
On 9 August 2016 at 14:20, David Miller  wrote:
> From: Lorenzo Colitti 
> 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.

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.


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-08 Thread David Miller
From: Lorenzo Colitti 
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.


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-08 Thread Lorenzo Colitti
On Tue, Aug 9, 2016 at 6:35 AM, David Miller  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?


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-08 Thread Lorenzo Colitti
On Tue, Aug 9, 2016 at 1:27 AM, David Ahern  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.

Ah, yes, thanks. Will fix the commit message.


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-08 Thread David Miller
From: Lorenzo Colitti 
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 

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.


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-08 Thread David Ahern
On 8/8/16 10:24 AM, Lorenzo Colitti wrote:
> On Tue, Aug 9, 2016 at 12:27 AM, David Ahern  wrote:
>>> - 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.


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-08 Thread Lorenzo Colitti
On Tue, Aug 9, 2016 at 12:27 AM, David Ahern  wrote:
> > - 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.


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-08 Thread 吉藤英明
Hi,

2016-08-08 23:45 GMT+09:00 Lorenzo Colitti :
> On Mon, Aug 8, 2016 at 11:26 PM, Hannes Frederic Sowa
>  wrote:
>>> - if (sk->sk_bound_dev_if &&
>>> - sk->sk_bound_dev_if != u->sin6_scope_id) {
>>> - return -EINVAL;
>>> - }
>>
>> 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


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-08 Thread David Ahern
On 8/8/16 1:42 AM, Lorenzo Colitti wrote:
> 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 
> ---
>  net/ipv6/ping.c | 29 +++--
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
> index fed40d1..eabf1ea 100644
> --- a/net/ipv6/ping.c
> +++ b/net/ipv6/ping.c

-8<-

> @@ -106,16 +111,12 @@ static int ping_v6_sendmsg(struct sock *sk, struct 
> msghdr *msg, size_t len)
>   fl6.flowi6_proto = IPPROTO_ICMPV6;
>   fl6.saddr = np->saddr;
>   fl6.daddr = *daddr;
> + fl6.flowi6_oif = oif;
>   fl6.flowi6_mark = sk->sk_mark;
>   fl6.fl6_icmp_type = user_icmph.icmp6_type;
>   fl6.fl6_icmp_code = user_icmph.icmp6_code;
>   security_sk_classify_flow(sk, flowi6_to_flowi(&fl6));
>  
> - 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;
> -
>   ipc6.tclass = np->tclass;
>   fl6.flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6.flowlabel);
>  
> 

That code removal is contrary to your patch description regarding flowi6_oif.


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-08 Thread Lorenzo Colitti
On Mon, Aug 8, 2016 at 11:26 PM, Hannes Frederic Sowa
 wrote:
>> - if (sk->sk_bound_dev_if &&
>> - sk->sk_bound_dev_if != u->sin6_scope_id) {
>> - return -EINVAL;
>> - }
>
> 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.


Re: [PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-08 Thread Hannes Frederic Sowa
On 08.08.2016 09:42, Lorenzo Colitti wrote:
> 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 
> ---
>  net/ipv6/ping.c | 29 +++--
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
> index fed40d1..eabf1ea 100644
> --- a/net/ipv6/ping.c
> +++ b/net/ipv6/ping.c
> @@ -55,7 +55,7 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr 
> *msg, size_t len)
>   struct icmp6hdr user_icmph;
>   int addr_type;
>   struct in6_addr *daddr;
> - int iif = 0;
> + int oif = 0;
>   struct flowi6 fl6;
>   int err;
>   struct dst_entry *dst;
> @@ -78,23 +78,28 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr 
> *msg, size_t len)
>   if (u->sin6_family != AF_INET6) {
>   return -EAFNOSUPPORT;
>   }
> - if (sk->sk_bound_dev_if &&
> - sk->sk_bound_dev_if != u->sin6_scope_id) {
> - return -EINVAL;
> - }

Hmm, sk->sk_bound_dev_if always has highest prio for the selection of
the output interface. Thus this code made sense to me.

Bye,
Hannes



[PATCH net] net: ipv6: Fix ping to link-local addresses.

2016-08-08 Thread Lorenzo Colitti
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 
---
 net/ipv6/ping.c | 29 +++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/net/ipv6/ping.c b/net/ipv6/ping.c
index fed40d1..eabf1ea 100644
--- a/net/ipv6/ping.c
+++ b/net/ipv6/ping.c
@@ -55,7 +55,7 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr 
*msg, size_t len)
struct icmp6hdr user_icmph;
int addr_type;
struct in6_addr *daddr;
-   int iif = 0;
+   int oif = 0;
struct flowi6 fl6;
int err;
struct dst_entry *dst;
@@ -78,23 +78,28 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr 
*msg, size_t len)
if (u->sin6_family != AF_INET6) {
return -EAFNOSUPPORT;
}
-   if (sk->sk_bound_dev_if &&
-   sk->sk_bound_dev_if != u->sin6_scope_id) {
-   return -EINVAL;
-   }
daddr = &(u->sin6_addr);
-   iif = u->sin6_scope_id;
+   if (__ipv6_addr_needs_scope_id(ipv6_addr_type(daddr)))
+   oif = u->sin6_scope_id;
} else {
if (sk->sk_state != TCP_ESTABLISHED)
return -EDESTADDRREQ;
daddr = &sk->sk_v6_daddr;
}
 
-   if (!iif)
-   iif = sk->sk_bound_dev_if;
+   if (!oif)
+   oif = sk->sk_bound_dev_if;
+
+   if (!oif)
+   oif = np->sticky_pktinfo.ipi6_ifindex;
+
+   if (!oif && ipv6_addr_is_multicast(daddr))
+   fl6.flowi6_oif = np->mcast_oif;
+   else if (!oif)
+   oif = np->ucast_oif;
 
addr_type = ipv6_addr_type(daddr);
-   if (__ipv6_addr_needs_scope_id(addr_type) && !iif)
+   if (__ipv6_addr_needs_scope_id(addr_type) && !oif)
return -EINVAL;
if (addr_type & IPV6_ADDR_MAPPED)
return -EINVAL;
@@ -106,16 +111,12 @@ static int ping_v6_sendmsg(struct sock *sk, struct msghdr 
*msg, size_t len)
fl6.flowi6_proto = IPPROTO_ICMPV6;
fl6.saddr = np->saddr;
fl6.daddr = *daddr;
+   fl6.flowi6_oif = oif;
fl6.flowi6_mark = sk->sk_mark;
fl6.fl6_icmp_type = user_icmph.icmp6_type;
fl6.fl6_icmp_code = user_icmph.icmp6_code;
security_sk_classify_flow(sk, flowi6_to_flowi(&fl6));
 
-   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;
-
ipc6.tclass = np->tclass;
fl6.flowlabel = ip6_make_flowinfo(ipc6.tclass, fl6.flowlabel);
 
-- 
2.8.0.rc3.226.g39d4020