pv6: fix sticky pktinfo behaviour

PATCH In review Paolo Abeni 5 participants 12 messages Jan 29, 2016 – Feb 2, 2016

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Paolo Abeni

author maintainer

2016-01-29 11:30 UTC #0 lore raw

v2 cover pv6: fix sticky pktinfo behaviour

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(-)

Signature · 1 line

1.8.3.1

Paolo Abeni

author maintainer

2016-01-29 11:30 UTC #1 lore raw

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

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 <[email protected]>

--
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(-)

include/net/ip6_route.h +10 -2

index 877f682..295d291 100644
@@ -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);

net/ipv6/ip6_output.c +5 -1

index 23de98f..a163102 100644
@@ -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)

net/ipv6/route.c +3 -4

index 3c8834b..ed44663 100644
@@ -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)
 {

Signature · 1 line

1.8.3.1

Hannes Frederic Sowa

Acked-by on v2 1/2

2016-01-29 12:33 UTC #2 lore raw

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

Quoted 13 lines · 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.

Quoted 8 lines

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 <[email protected]>
--
v1 -> v2 move the ip6_route_output() implementation into the header

Acked-by Hannes Frederic Sowa <[email protected]>

YOSHIFUJI Hideaki

Acked-by

2016-02-01 02:24 UTC #3 lore raw

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

Quoted 19 lines · Hannes Frederic Sowa

Quoted 18 lines · 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.

Quoted 6 lines

Quoted 3 lines
Signed-off-by Paolo Abeni <[email protected]>
Signature · 1 line
v1 -> v2 move the ip6_route_output() implementation into the header
Acked-by Hannes Frederic Sowa <[email protected]>

Acked-by YOSHIFUJI Hideaki <[email protected]>

Signature · 2 lines

Hideaki Yoshifuji <[email protected]>
Technical Division, MIRACLE LINUX CORPORATION

David Ahern

Acked-by on v2 1/2

2016-01-29 16:00 UTC #4 lore raw

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

In reply to Paolo Abeni

Quoted 13 lines · 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.

Quoted 7 lines

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 <[email protected]>
--

Acked-by David Ahern <[email protected]>

Paolo Abeni

author maintainer

2016-01-29 11:30 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 net/ipv6/datagram.c | 3 +++
 1 file changed, 3 insertions(+)

net/ipv6/datagram.c +3 -0

index 517c55b..4281621 100644
@@ -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;

Signature · 1 line

1.8.3.1

Hannes Frederic Sowa

Acked-by on v2 2/2

2016-01-29 12:34 UTC #6 lore raw

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

Quoted 8 lines · 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 <[email protected]>

Acked-by Hannes Frederic Sowa <[email protected]>

YOSHIFUJI Hideaki

Acked-by

2016-02-01 02:23 UTC #7 lore raw

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

Quoted 8 lines · Hannes Frederic Sowa

Quoted 7 lines · 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.

Quoted 5 lines

Quoted 1 line
Signed-off-by Paolo Abeni <[email protected]>
Acked-by Hannes Frederic Sowa <[email protected]>

Acked-by YOSHIFUJI Hideaki <[email protected]>

Signature · 2 lines

Hideaki Yoshifuji <[email protected]>
Technical Division, MIRACLE LINUX CORPORATION

David Ahern

2016-02-02 02:06 UTC #8 lore raw

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

Quoted 11 lines · YOSHIFUJI Hideaki

Quoted 8 lines · Hannes Frederic Sowa
Quoted 7 lines · 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.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Signed-off-by Paolo Abeni <[email protected]>
Acked-by Hannes Frederic Sowa <[email protected]>
Acked-by YOSHIFUJI Hideaki <[email protected]>

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

David Miller

maintainer

2016-02-02 02:18 UTC #9 lore raw

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

From: David Ahern <[email protected]>
Date: Mon, 1 Feb 2016 19:06:15 -0700

Quoted 2 lines

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.

Hannes Frederic Sowa

2016-01-29 12:36 UTC #10 lore raw

In reply to Paolo Abeni

Quoted 8 lines · Paolo Abeni

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

David Miller

maintainer

2016-01-30 04:31 UTC #11 lore raw

In reply to Paolo Abeni

From: Paolo Abeni <[email protected]>
Date: Fri, 29 Jan 2016 12:30:18 +0100

Quoted 18 lines

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.

Quoted 8 lines

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.