[PATCH net v2 0/2] pv6: fix sticky pktinfo behaviour

[PATCH net v2 0/2] pv6: fix sticky pktinfo behaviour

[Paolo Abeni]

Currently:

ip addr add dev eth0 2001:0010::1/64
ip addr add dev eth1 2001:0020::1/64
ping6 -I eth0 2001:0020::2

do not lead to the expected results, i.e. eth1 is used as the
egress interface.

This is due to two related issues in handling sticky pktinfo,
used by ping6 to enforce the device binding:

- ip6_dst_lookup_flow()/ip6_dst_lookup_tail() do not really enforce
flowi6_oif match
- ipv6 udp connect() just ignore flowi6_oif

These patches address each issue individually.

The kernel has never enforced the egress interface specified
via the sticky pktinfo, except briefly between the commits
741a11d9e410 ("net: ipv6: Add RT6_LOOKUP_F_IFACE flag if oif is set")
and
d46a9d678e4c ("net: ipv6: Dont add RT6_LOOKUP_F_IFACE flag if saddr set"),
but the ping6 tools was unaffected up to iputils-20100214,
since before it used SO_BINDTODEVICE to enforce the egress
interface.

Paolo Abeni (2):
  ipv6: enforce flowi6_oif usage in ip6_dst_lookup_tail()
  ipv6/udp: use sticky pktinfo egress ifindex on connect()

 include/net/ip6_route.h | 12 ++++++++++--
 net/ipv6/datagram.c     |  3 +++
 net/ipv6/ip6_output.c   |  6 +++++-
 net/ipv6/route.c        |  7 +++----
 4 files changed, 21 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[PATCH net v2 1/2] ipv6: enforce flowi6_oif usage in ip6_dst_lookup_tail()

[Paolo Abeni]

The current implementation of ip6_dst_lookup_tail basically
ignore the egress ifindex match: if the saddr is set,
ip6_route_output() purposefully ignores flowi6_oif, due
to the commit d46a9d678e4c ("net: ipv6: Dont add RT6_LOOKUP_F_IFACE
flag if saddr set"), if the saddr is 'any' the first route lookup
in ip6_dst_lookup_tail fails, but upon failure a second lookup will
be performed with saddr set, thus ignoring the ifindex constraint.

This commit adds an output route lookup function variant, which
allows the caller to specify lookup flags, and modify
ip6_dst_lookup_tail() to enforce the ifindex match on the second
lookup via said helper.

ip6_route_output() becames now a static inline function build on
top of ip6_route_output_flags(); as a side effect, out-of-tree
modules need now a GPL license to access the output route lookup
functionality.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
--
v1 -> v2 move the ip6_route_output() implementation into the header
---
 include/net/ip6_route.h | 12 ++++++++++--
 net/ipv6/ip6_output.c   |  6 +++++-
 net/ipv6/route.c        |  7 +++----
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index 877f682..295d291 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -64,8 +64,16 @@ static inline bool rt6_need_strict(const struct in6_addr *daddr)
 
 void ip6_route_input(struct sk_buff *skb);
 
-struct dst_entry *ip6_route_output(struct net *net, const struct sock *sk,
-				   struct flowi6 *fl6);
+struct dst_entry *ip6_route_output_flags(struct net *net, const struct sock *sk,
+					 struct flowi6 *fl6, int flags);
+
+static inline struct dst_entry *ip6_route_output(struct net *net,
+						 const struct sock *sk,
+						 struct flowi6 *fl6)
+{
+	return ip6_route_output_flags(net, sk, fl6, 0);
+}
+
 struct dst_entry *ip6_route_lookup(struct net *net, struct flowi6 *fl6,
 				   int flags);
 
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 23de98f..a163102 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -909,6 +909,7 @@ static int ip6_dst_lookup_tail(struct net *net, const struct sock *sk,
 	struct rt6_info *rt;
 #endif
 	int err;
+	int flags = 0;
 
 	/* The correct way to handle this would be to do
 	 * ip6_route_get_saddr, and then ip6_route_output; however,
@@ -940,10 +941,13 @@ static int ip6_dst_lookup_tail(struct net *net, const struct sock *sk,
 			dst_release(*dst);
 			*dst = NULL;
 		}
+
+		if (fl6->flowi6_oif)
+			flags |= RT6_LOOKUP_F_IFACE;
 	}
 
 	if (!*dst)
-		*dst = ip6_route_output(net, sk, fl6);
+		*dst = ip6_route_output_flags(net, sk, fl6, flags);
 
 	err = (*dst)->error;
 	if (err)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 3c8834b..ed44663 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1183,11 +1183,10 @@ static struct rt6_info *ip6_pol_route_output(struct net *net, struct fib6_table
 	return ip6_pol_route(net, table, fl6->flowi6_oif, fl6, flags);
 }
 
-struct dst_entry *ip6_route_output(struct net *net, const struct sock *sk,
-				    struct flowi6 *fl6)
+struct dst_entry *ip6_route_output_flags(struct net *net, const struct sock *sk,
+					 struct flowi6 *fl6, int flags)
 {
 	struct dst_entry *dst;
-	int flags = 0;
 	bool any_src;
 
 	dst = l3mdev_rt6_dst_by_oif(net, fl6);
@@ -1208,7 +1207,7 @@ struct dst_entry *ip6_route_output(struct net *net, const struct sock *sk,
 
 	return fib6_rule_lookup(net, fl6, flags, ip6_pol_route_output);
 }
-EXPORT_SYMBOL(ip6_route_output);
+EXPORT_SYMBOL_GPL(ip6_route_output_flags);
 
 struct dst_entry *ip6_blackhole_route(struct net *net, struct dst_entry *dst_orig)
 {
-- 
1.8.3.1

Re: [PATCH net v2 1/2] ipv6: enforce flowi6_oif usage in ip6_dst_lookup_tail()

[Hannes Frederic Sowa]

On 29.01.2016 12:30, Paolo Abeni wrote:
> The current implementation of ip6_dst_lookup_tail basically
> ignore the egress ifindex match: if the saddr is set,
> ip6_route_output() purposefully ignores flowi6_oif, due
> to the commit d46a9d678e4c ("net: ipv6: Dont add RT6_LOOKUP_F_IFACE
> flag if saddr set"), if the saddr is 'any' the first route lookup
> in ip6_dst_lookup_tail fails, but upon failure a second lookup will
> be performed with saddr set, thus ignoring the ifindex constraint.
>
> This commit adds an output route lookup function variant, which
> allows the caller to specify lookup flags, and modify
> ip6_dst_lookup_tail() to enforce the ifindex match on the second
> lookup via said helper.
>
> ip6_route_output() becames now a static inline function build on
> top of ip6_route_output_flags(); as a side effect, out-of-tree
> modules need now a GPL license to access the output route lookup
> functionality.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> --
> v1 -> v2 move the ip6_route_output() implementation into the header

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Re: [PATCH net v2 1/2] ipv6: enforce flowi6_oif usage in ip6_dst_lookup_tail()

[YOSHIFUJI Hideaki]

Hannes Frederic Sowa wrote:
> On 29.01.2016 12:30, Paolo Abeni wrote:
>> The current implementation of ip6_dst_lookup_tail basically
>> ignore the egress ifindex match: if the saddr is set,
>> ip6_route_output() purposefully ignores flowi6_oif, due
>> to the commit d46a9d678e4c ("net: ipv6: Dont add RT6_LOOKUP_F_IFACE
>> flag if saddr set"), if the saddr is 'any' the first route lookup
>> in ip6_dst_lookup_tail fails, but upon failure a second lookup will
>> be performed with saddr set, thus ignoring the ifindex constraint.
>>
>> This commit adds an output route lookup function variant, which
>> allows the caller to specify lookup flags, and modify
>> ip6_dst_lookup_tail() to enforce the ifindex match on the second
>> lookup via said helper.
>>
>> ip6_route_output() becames now a static inline function build on
>> top of ip6_route_output_flags(); as a side effect, out-of-tree
>> modules need now a GPL license to access the output route lookup
>> functionality.
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> -- 
>> v1 -> v2 move the ip6_route_output() implementation into the header
> 
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> 
Acked-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>

-- 
Hideaki Yoshifuji <hideaki.yoshifuji@miraclelinux.com>
Technical Division, MIRACLE LINUX CORPORATION

Re: [PATCH net v2 1/2] ipv6: enforce flowi6_oif usage in ip6_dst_lookup_tail()

[David Ahern]

On 1/29/16 4:30 AM, Paolo Abeni wrote:
> The current implementation of ip6_dst_lookup_tail basically
> ignore the egress ifindex match: if the saddr is set,
> ip6_route_output() purposefully ignores flowi6_oif, due
> to the commit d46a9d678e4c ("net: ipv6: Dont add RT6_LOOKUP_F_IFACE
> flag if saddr set"), if the saddr is 'any' the first route lookup
> in ip6_dst_lookup_tail fails, but upon failure a second lookup will
> be performed with saddr set, thus ignoring the ifindex constraint.
>
> This commit adds an output route lookup function variant, which
> allows the caller to specify lookup flags, and modify
> ip6_dst_lookup_tail() to enforce the ifindex match on the second
> lookup via said helper.
>
> ip6_route_output() becames now a static inline function build on
> top of ip6_route_output_flags(); as a side effect, out-of-tree
> modules need now a GPL license to access the output route lookup
> functionality.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> --

Acked-by: David Ahern <dsa@cumulusnetworks.com>

[PATCH net v2 2/2] ipv6/udp: use sticky pktinfo egress ifindex on connect()

[Paolo Abeni]

Currently, the egress interface index specified via IPV6_PKTINFO
is ignored by __ip6_datagram_connect(), so that RFC 3542 section 6.7
can be subverted when the user space application calls connect()
before sendmsg().
Fix it by initializing properly flowi6_oif in connect() before
performing the route lookup.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/ipv6/datagram.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 517c55b..4281621 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -162,6 +162,9 @@ ipv4_connected:
 	fl6.fl6_dport = inet->inet_dport;
 	fl6.fl6_sport = inet->inet_sport;
 
+	if (!fl6.flowi6_oif)
+		fl6.flowi6_oif = np->sticky_pktinfo.ipi6_ifindex;
+
 	if (!fl6.flowi6_oif && (addr_type&IPV6_ADDR_MULTICAST))
 		fl6.flowi6_oif = np->mcast_oif;
 
-- 
1.8.3.1

Re: [PATCH net v2 2/2] ipv6/udp: use sticky pktinfo egress ifindex on connect()

[Hannes Frederic Sowa]

On 29.01.2016 12:30, Paolo Abeni wrote:
> Currently, the egress interface index specified via IPV6_PKTINFO
> is ignored by __ip6_datagram_connect(), so that RFC 3542 section 6.7
> can be subverted when the user space application calls connect()
> before sendmsg().
> Fix it by initializing properly flowi6_oif in connect() before
> performing the route lookup.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Re: [PATCH net v2 2/2] ipv6/udp: use sticky pktinfo egress ifindex on connect()

[YOSHIFUJI Hideaki]

Hannes Frederic Sowa wrote:
> On 29.01.2016 12:30, Paolo Abeni wrote:
>> Currently, the egress interface index specified via IPV6_PKTINFO
>> is ignored by __ip6_datagram_connect(), so that RFC 3542 section 6.7
>> can be subverted when the user space application calls connect()
>> before sendmsg().
>> Fix it by initializing properly flowi6_oif in connect() before
>> performing the route lookup.
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> 
> 

Acked-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>

-- 
Hideaki Yoshifuji <hideaki.yoshifuji@miraclelinux.com>
Technical Division, MIRACLE LINUX CORPORATION

Re: [PATCH net v2 2/2] ipv6/udp: use sticky pktinfo egress ifindex on connect()

[David Ahern]

On 1/31/16 7:23 PM, YOSHIFUJI Hideaki wrote:
>
>
> Hannes Frederic Sowa wrote:
>> On 29.01.2016 12:30, Paolo Abeni wrote:
>>> Currently, the egress interface index specified via IPV6_PKTINFO
>>> is ignored by __ip6_datagram_connect(), so that RFC 3542 section 6.7
>>> can be subverted when the user space application calls connect()
>>> before sendmsg().
>>> Fix it by initializing properly flowi6_oif in connect() before
>>> performing the route lookup.
>>>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>
>> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>>
>>
>
> Acked-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
>

Dave:

Can you queue these up for 4.4 stable? Fixes 'ping6 -I <interface>' for 
VRFs and 4.4 is the first release with VRF for IPv6.

Thanks,

David

Re: [PATCH net v2 2/2] ipv6/udp: use sticky pktinfo egress ifindex on connect()

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Mon, 1 Feb 2016 19:06:15 -0700

> Can you queue these up for 4.4 stable? Fixes 'ping6 -I <interface>'
> for VRFs and 4.4 is the first release with VRF for IPv6.

Sure, done.

Re: [PATCH net v2 0/2] pv6: fix sticky pktinfo behaviour

[Hannes Frederic Sowa]

On 29.01.2016 12:30, Paolo Abeni wrote:
> The kernel has never enforced the egress interface specified
> via the sticky pktinfo, except briefly between the commits
> 741a11d9e410 ("net: ipv6: Add RT6_LOOKUP_F_IFACE flag if oif is set")
> and
> d46a9d678e4c ("net: ipv6: Dont add RT6_LOOKUP_F_IFACE flag if saddr set"),
> but the ping6 tools was unaffected up to iputils-20100214,
> since before it used SO_BINDTODEVICE to enforce the egress
> interface.

I think because of the bad situation in terms of uapi and that recently 
behavior switched multiple times in kernel and iputils, enforcing the 
outgoing interface seems to be the right thing to do for me.

Also it matches expected behavior by ping6. Treating the interface as 
enforced is from a standards PoV the correct thing to do.

Thanks,
Hannes

Re: [PATCH net v2 0/2] pv6: fix sticky pktinfo behaviour

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Fri, 29 Jan 2016 12:30:18 +0100

> Currently:
> 
> ip addr add dev eth0 2001:0010::1/64
> ip addr add dev eth1 2001:0020::1/64
> ping6 -I eth0 2001:0020::2
> 
> do not lead to the expected results, i.e. eth1 is used as the
> egress interface.
> 
> This is due to two related issues in handling sticky pktinfo,
> used by ping6 to enforce the device binding:
> 
> - ip6_dst_lookup_flow()/ip6_dst_lookup_tail() do not really enforce
> flowi6_oif match
> - ipv6 udp connect() just ignore flowi6_oif
> 
> These patches address each issue individually.
> 
> The kernel has never enforced the egress interface specified
> via the sticky pktinfo, except briefly between the commits
> 741a11d9e410 ("net: ipv6: Add RT6_LOOKUP_F_IFACE flag if oif is set")
> and
> d46a9d678e4c ("net: ipv6: Dont add RT6_LOOKUP_F_IFACE flag if saddr set"),
> but the ping6 tools was unaffected up to iputils-20100214,
> since before it used SO_BINDTODEVICE to enforce the egress
> interface.

Series applied, thanks.