ipv6: fix sticky pktinfo behaviour

PATCH Superseded Paolo Abeni 3 participants 8 messages Jan 27, 2016 – Jan 28, 2016

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Paolo Abeni

author maintainer

2016-01-27 13:46 UTC #0 lore raw

cover ipv6: 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 leads 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.

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 |  2 ++
 net/ipv6/datagram.c     |  3 +++
 net/ipv6/ip6_output.c   |  6 +++++-
 net/ipv6/route.c        | 12 +++++++++---
 4 files changed, 19 insertions(+), 4 deletions(-)

Signature · 1 line

1.8.3.1

Paolo Abeni

author maintainer

2016-01-27 13:46 UTC #1 lore raw

PATCH 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 additional lookup flags, and modify ip6_dst_lookup_tail() to enforce the ifindex match on the second lookup via said helper.

Fixes d46a9d678e4c ("net: ipv6: Dont add RT6_LOOKUP_F_IFACE flag if saddr set")

Signed-off-by Paolo Abeni <[email protected]>

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

include/net/ip6_route.h +2 -0

index 877f682..c221e36 100644
@@ -66,6 +66,8 @@ 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);
 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 +9 -3

index 3c8834b..973cb73 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,6 +1207,13 @@ 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_GPL(ip6_route_output_flags);
+
+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);
+}
 EXPORT_SYMBOL(ip6_route_output);
 
 struct dst_entry *ip6_blackhole_route(struct net *net, struct dst_entry *dst_orig)

Signature · 1 line

1.8.3.1

Hannes Frederic Sowa

2016-01-27 18:38 UTC #2 lore raw

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

Quoted 20 lines · Paolo Abeni

net/ipv6/route.c +2 -3

index 3c8834b..973cb73 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,6 +1207,13 @@ struct dst_entry *ip6_route_output(struct net *net, const struct sock *sk,

net/ipv6/route.c struct dst_entry *ip6_route_output(struct net *net, const struct sock *sk, · line 1217

Quoted 10 lines

net/ipv6/route.c +7 -0

  	return fib6_rule_lookup(net, fl6, flags, ip6_pol_route_output);
  }
+EXPORT_SYMBOL_GPL(ip6_route_output_flags);
+
+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);
+}
  EXPORT_SYMBOL(ip6_route_output);

I think this can just be a static inline function.

Is it a lot of work to introduce the flags argument globally? Most other functions already have a flags parameter, maybe instead of just adding another wrapper just bite the bullet and add it everywhere?

Thanks,
Hannes

Paolo Abeni

author maintainer

2016-01-28 12:27 UTC #3 lore raw

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

Quoted 34 lines · Hannes Frederic Sowa

Quoted 20 lines · Paolo Abeni

net/ipv6/route.c +2 -3

index 3c8834b..973cb73 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,6 +1207,13 @@ struct dst_entry *ip6_route_output(struct net *net, const struct sock *sk,

net/ipv6/route.c struct dst_entry *ip6_route_output(struct net *net, const struct sock *sk, · line 1217

Quoted 10 lines

net/ipv6/route.c +7 -0

  	return fib6_rule_lookup(net, fl6, flags, ip6_pol_route_output);
  }
+EXPORT_SYMBOL_GPL(ip6_route_output_flags);
+
+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);
+}
  EXPORT_SYMBOL(ip6_route_output);

I think this can just be a static inline function.

Quoted 3 lines

Is it a lot of work to introduce the flags argument globally? Most other functions already have a flags parameter, maybe instead of just adding another wrapper just bite the bullet and add it everywhere?

There are ~20 call sites for ip6_route_output(). Replacing them with ip6_route_output_flags() should be trivial, but it sounds quite invasive. Moving the new ip6_route_output() definition into the header file as static inline function should be pretty much equivalent, may I go with the latter option ?

Cheers,

Paolo

Hannes Frederic Sowa

2016-01-28 13:02 UTC #4 lore raw

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

Quoted 39 lines · Paolo Abeni

Quoted 34 lines · Hannes Frederic Sowa

Quoted 20 lines · Paolo Abeni

net/ipv6/route.c +2 -3

index 3c8834b..973cb73 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,6 +1207,13 @@ struct dst_entry *ip6_route_output(struct net *net, const struct sock *sk,

net/ipv6/route.c struct dst_entry *ip6_route_output(struct net *net, const struct sock *sk, · line 1217

Quoted 10 lines

net/ipv6/route.c +7 -0

   	return fib6_rule_lookup(net, fl6, flags, ip6_pol_route_output);
   }
+EXPORT_SYMBOL_GPL(ip6_route_output_flags);
+
+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);
+}
   EXPORT_SYMBOL(ip6_route_output);

I think this can just be a static inline function.

Quoted 3 lines

Is it a lot of work to introduce the flags argument globally? Most other functions already have a flags parameter, maybe instead of just adding another wrapper just bite the bullet and add it everywhere?

Quoted 5 lines

There are ~20 call sites for ip6_route_output(). Replacing them with ip6_route_output_flags() should be trivial, but it sounds quite invasive. Moving the new ip6_route_output() definition into the header file as static inline function should be pretty much equivalent, may I go with the latter option ?

I am not really a fan of such static inline wrappers all over the place, it doesnt't really help to evolve the code base.

But as a net/stable fix, go with the static inline first, IMHO.

Thanks,
Hannes

David Ahern

2016-01-28 14:59 UTC #5 lore raw

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

In reply to Paolo Abeni

Quoted 8 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.

Quoted 7 lines

This commit adds an output route lookup function variant, which allows the caller to specify additional lookup flags, and modify ip6_dst_lookup_tail() to enforce the ifindex match on the second lookup via said helper.
Fixes d46a9d678e4c ("net: ipv6: Dont add RT6_LOOKUP_F_IFACE flag if saddr set")
Signed-off-by Paolo Abeni <[email protected]>

I don't agree with that 'Fixes:' tag.

ip6_route_output did not add the RT6_LOOKUP_F_IFACE flag until 741a11d9e410; d46a9d678e4c is a follow on to limit adding the flag only if no source address is given.

Since ip6_dst_lookup_tail never considered the flowi6_oif this is a general bug fix rather than a fix of d46a9d678e4c.

Paolo Abeni

author maintainer

2016-01-28 17:04 UTC #6 lore raw

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

Quoted 17 lines · David Ahern

Quoted 8 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.
Quoted 7 lines
This commit adds an output route lookup function variant, which allows the caller to specify additional lookup flags, and modify ip6_dst_lookup_tail() to enforce the ifindex match on the second lookup via said helper.
Fixes d46a9d678e4c ("net: ipv6: Dont add RT6_LOOKUP_F_IFACE flag if saddr set")
Signed-off-by Paolo Abeni <[email protected]>

Quoted 8 lines

I don't agree with that 'Fixes:' tag.
ip6_route_output did not add the RT6_LOOKUP_F_IFACE flag until 741a11d9e410; d46a9d678e4c is a follow on to limit adding the flag only if no source address is given.
Since ip6_dst_lookup_tail never considered the flowi6_oif this is a general bug fix rather than a fix of d46a9d678e4c.

Ok, I'll remove it in the v2, which will include Hannes feedback, too.

Regards,

Paolo

Paolo Abeni

author maintainer

2016-01-27 13:46 UTC #7 lore raw

PATCH 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