All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] net: avoid a full fib lookup when rp_filter is disabled.
@ 2017-09-19 10:46 Paolo Abeni
  2017-09-19 12:00 ` Eric Dumazet
  0 siblings, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2017-09-19 10:46 UTC (permalink / raw)
  To: netdev; +Cc: David S. Miller, Hannes Frederic Sowa

Since commit 1dced6a85482 ("ipv4: Restore accept_local
behaviour in fib_validate_source()") a full fib lookup
is needed even if the rp_filter is disabled, if
accept_local is false - which is the default.

What we really need in the above scenario is just checking
that the source IP address is not local, and we can do
that is a cheaper way looking up the ifaddr hash table.

This commit adds an helper for such lookup and uses it
to validate the src address when rp_filter is disabled.
It also drops the checks to bail early from
__fib_validate_source, added by the commit 1dced6a85482
("ipv4: Restore accept_local behaviour in fib_validate_source()")
and now unneeded.

This improves UDP performances for unconnected sockets
when rp_filter is disabled by 5% and also gives small but
measurable performance improvement for TCP flood scenarios.

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/linux/inetdevice.h |  1 +
 net/ipv4/devinet.c         | 14 ++++++++++++++
 net/ipv4/fib_frontend.c    | 11 +++++++----
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index fb3f809e34e4..751d051f0bc7 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -179,6 +179,7 @@ __be32 inet_confirm_addr(struct net *net, struct in_device *in_dev, __be32 dst,
 			 __be32 local, int scope);
 struct in_ifaddr *inet_ifa_byprefix(struct in_device *in_dev, __be32 prefix,
 				    __be32 mask);
+struct in_ifaddr *inet_lookup_ifaddr_rcu(struct net *net, __be32 addr);
 static __inline__ bool inet_ifa_match(__be32 addr, struct in_ifaddr *ifa)
 {
 	return !((addr^ifa->ifa_address)&ifa->ifa_mask);
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index d7adc0616599..73bf09bcfe43 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -173,6 +173,20 @@ struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref)
 }
 EXPORT_SYMBOL(__ip_dev_find);
 
+/* called under RCU lock */
+struct in_ifaddr *inet_lookup_ifaddr_rcu(struct net *net, __be32 addr)
+{
+	u32 hash = inet_addr_hash(net, addr);
+	struct in_ifaddr *ifa;
+
+	hlist_for_each_entry_rcu(ifa, &inet_addr_lst[hash], hash)
+		if (ifa->ifa_local == addr &&
+		    net_eq(dev_net(ifa->ifa_dev->dev), net))
+			return ifa;
+
+	return NULL;
+}
+
 static void rtmsg_ifa(int event, struct in_ifaddr *, struct nlmsghdr *, u32);
 
 static BLOCKING_NOTIFIER_HEAD(inetaddr_chain);
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 37819ab4cc74..1470d265a357 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -345,9 +345,6 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
 	if (res.type != RTN_UNICAST &&
 	    (res.type != RTN_LOCAL || !IN_DEV_ACCEPT_LOCAL(idev)))
 		goto e_inval;
-	if (!rpf && !fib_num_tclassid_users(net) &&
-	    (dev->ifindex != oif || !IN_DEV_TX_REDIRECTS(idev)))
-		goto last_resort;
 	fib_combine_itag(itag, &res);
 	dev_match = false;
 
@@ -404,8 +401,14 @@ int fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
 	int r = secpath_exists(skb) ? 0 : IN_DEV_RPFILTER(idev);
 
 	if (!r && !fib_num_tclassid_users(dev_net(dev)) &&
-	    IN_DEV_ACCEPT_LOCAL(idev) &&
 	    (dev->ifindex != oif || !IN_DEV_TX_REDIRECTS(idev))) {
+		/* we need only to ensure that the src address is not a
+		 * local one
+		 */
+		if (!IN_DEV_ACCEPT_LOCAL(idev) &&
+		    inet_lookup_ifaddr_rcu(dev_net(dev), src))
+			return -EINVAL;
+
 		*itag = 0;
 		return 0;
 	}
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: avoid a full fib lookup when rp_filter is disabled.
  2017-09-19 10:46 [PATCH net-next] net: avoid a full fib lookup when rp_filter is disabled Paolo Abeni
@ 2017-09-19 12:00 ` Eric Dumazet
  2017-09-19 12:46   ` Paolo Abeni
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2017-09-19 12:00 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: netdev, David S. Miller, Hannes Frederic Sowa

On Tue, 2017-09-19 at 12:46 +0200, Paolo Abeni wrote:
..

> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---

> +/* called under RCU lock */
> +struct in_ifaddr *inet_lookup_ifaddr_rcu(struct net *net, __be32 addr)
> +{
> +	u32 hash = inet_addr_hash(net, addr);
> +	struct in_ifaddr *ifa;
> +
> +	hlist_for_each_entry_rcu(ifa, &inet_addr_lst[hash], hash)
> +		if (ifa->ifa_local == addr &&
> +		    net_eq(dev_net(ifa->ifa_dev->dev), net))
> +			return ifa;
> +
> +	return NULL;
> +}
> +

Any particular reason you do not use this helper to replace the lookup
in __ip_dev_find() ?

In this case, copy paste can be avoided, please avoid a future patch
consolidating the things...

Thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: avoid a full fib lookup when rp_filter is disabled.
  2017-09-19 12:00 ` Eric Dumazet
@ 2017-09-19 12:46   ` Paolo Abeni
  0 siblings, 0 replies; 6+ messages in thread
From: Paolo Abeni @ 2017-09-19 12:46 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, David S. Miller, Hannes Frederic Sowa

Hi,

On Tue, 2017-09-19 at 05:00 -0700, Eric Dumazet wrote:
> On Tue, 2017-09-19 at 12:46 +0200, Paolo Abeni wrote:
> ..
> 
> > Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > +/* called under RCU lock */
> > +struct in_ifaddr *inet_lookup_ifaddr_rcu(struct net *net, __be32 addr)
> > +{
> > +	u32 hash = inet_addr_hash(net, addr);
> > +	struct in_ifaddr *ifa;
> > +
> > +	hlist_for_each_entry_rcu(ifa, &inet_addr_lst[hash], hash)
> > +		if (ifa->ifa_local == addr &&
> > +		    net_eq(dev_net(ifa->ifa_dev->dev), net))
> > +			return ifa;
> > +
> > +	return NULL;
> > +}
> > +

Thank you for reviewing this!

> Any particular reason you do not use this helper to replace the lookup
> in __ip_dev_find() ?

uh, I just missed that opportunity. I'll send a v2, thanks!

Paolo

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: avoid a full fib lookup when rp_filter is disabled.
  2017-09-20 16:26 Paolo Abeni
  2017-09-20 16:37 ` Paolo Abeni
@ 2017-09-21 22:15 ` David Miller
  1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2017-09-21 22:15 UTC (permalink / raw)
  To: pabeni; +Cc: netdev, edumazet, hannes

From: Paolo Abeni <pabeni@redhat.com>
Date: Wed, 20 Sep 2017 18:26:53 +0200

> Since commit 1dced6a85482 ("ipv4: Restore accept_local behaviour
> in fib_validate_source()") a full fib lookup is needed even if
> the rp_filter is disabled, if accept_local is false - which is
> the default.
> 
> What we really need in the above scenario is just checking
> that the source IP address is not local, and in most case we
> can do that is a cheaper way looking up the ifaddr hash table.
> 
> This commit adds a helper for such lookup, and uses it to
> validate the src address when rp_filter is disabled and no
> 'local' routes are created by the user space in the relevant
> namespace.
> 
> A new ipv4 netns flag is added to account for such routes.
> We need that to preserve the same behavior we had before this
> patch.
> 
> It also drops the checks to bail early from __fib_validate_source,
> added by the commit 1dced6a85482 ("ipv4: Restore accept_local
> behaviour in fib_validate_source()") they do not give any
> measurable performance improvement: if we do the lookup with are
> on a slower path.
> 
> This improves UDP performances for unconnected sockets
> when rp_filter is disabled by 5% and also gives small but
> measurable performance improvement for TCP flood scenarios.
> 
> v1 -> v2:
>  - use the ifaddr lookup helper in __ip_dev_find(), as suggested
>    by Eric
>  - fall-back to full lookup if custom local routes are present
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Looks good, applied, thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: avoid a full fib lookup when rp_filter is disabled.
  2017-09-20 16:26 Paolo Abeni
@ 2017-09-20 16:37 ` Paolo Abeni
  2017-09-21 22:15 ` David Miller
  1 sibling, 0 replies; 6+ messages in thread
From: Paolo Abeni @ 2017-09-20 16:37 UTC (permalink / raw)
  To: David S. Miller; +Cc: Eric Dumazet, Hannes Frederic Sowa, netdev

Dumb me and dumb my scripts. 

This is actually a v2, v1 was at:

https://patchwork.ozlabs.org/project/netdev/list/?series=3835

David, please let me know if you prefer I'll repost with a more
appropriate subject line.

Sorry for the noise,

Paolo

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH net-next] net: avoid a full fib lookup when rp_filter is disabled.
@ 2017-09-20 16:26 Paolo Abeni
  2017-09-20 16:37 ` Paolo Abeni
  2017-09-21 22:15 ` David Miller
  0 siblings, 2 replies; 6+ messages in thread
From: Paolo Abeni @ 2017-09-20 16:26 UTC (permalink / raw)
  To: netdev; +Cc: David S. Miller, Eric Dumazet, Hannes Frederic Sowa

Since commit 1dced6a85482 ("ipv4: Restore accept_local behaviour
in fib_validate_source()") a full fib lookup is needed even if
the rp_filter is disabled, if accept_local is false - which is
the default.

What we really need in the above scenario is just checking
that the source IP address is not local, and in most case we
can do that is a cheaper way looking up the ifaddr hash table.

This commit adds a helper for such lookup, and uses it to
validate the src address when rp_filter is disabled and no
'local' routes are created by the user space in the relevant
namespace.

A new ipv4 netns flag is added to account for such routes.
We need that to preserve the same behavior we had before this
patch.

It also drops the checks to bail early from __fib_validate_source,
added by the commit 1dced6a85482 ("ipv4: Restore accept_local
behaviour in fib_validate_source()") they do not give any
measurable performance improvement: if we do the lookup with are
on a slower path.

This improves UDP performances for unconnected sockets
when rp_filter is disabled by 5% and also gives small but
measurable performance improvement for TCP flood scenarios.

v1 -> v2:
 - use the ifaddr lookup helper in __ip_dev_find(), as suggested
   by Eric
 - fall-back to full lookup if custom local routes are present

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/linux/inetdevice.h |  1 +
 include/net/netns/ipv4.h   |  1 +
 net/ipv4/devinet.c         | 30 ++++++++++++++++++------------
 net/ipv4/fib_frontend.c    | 22 +++++++++++++++++-----
 4 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index fb3f809e34e4..751d051f0bc7 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -179,6 +179,7 @@ __be32 inet_confirm_addr(struct net *net, struct in_device *in_dev, __be32 dst,
 			 __be32 local, int scope);
 struct in_ifaddr *inet_ifa_byprefix(struct in_device *in_dev, __be32 prefix,
 				    __be32 mask);
+struct in_ifaddr *inet_lookup_ifaddr_rcu(struct net *net, __be32 addr);
 static __inline__ bool inet_ifa_match(__be32 addr, struct in_ifaddr *ifa)
 {
 	return !((addr^ifa->ifa_address)&ifa->ifa_mask);
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index 20d061c805e3..20720721da4b 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -49,6 +49,7 @@ struct netns_ipv4 {
 #ifdef CONFIG_IP_MULTIPLE_TABLES
 	struct fib_rules_ops	*rules_ops;
 	bool			fib_has_custom_rules;
+	bool			fib_has_custom_local_routes;
 	struct fib_table __rcu	*fib_main;
 	struct fib_table __rcu	*fib_default;
 #endif
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index d7adc0616599..7ce22a2c07ce 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -137,22 +137,12 @@ static void inet_hash_remove(struct in_ifaddr *ifa)
  */
 struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref)
 {
-	u32 hash = inet_addr_hash(net, addr);
 	struct net_device *result = NULL;
 	struct in_ifaddr *ifa;
 
 	rcu_read_lock();
-	hlist_for_each_entry_rcu(ifa, &inet_addr_lst[hash], hash) {
-		if (ifa->ifa_local == addr) {
-			struct net_device *dev = ifa->ifa_dev->dev;
-
-			if (!net_eq(dev_net(dev), net))
-				continue;
-			result = dev;
-			break;
-		}
-	}
-	if (!result) {
+	ifa = inet_lookup_ifaddr_rcu(net, addr);
+	if (!ifa) {
 		struct flowi4 fl4 = { .daddr = addr };
 		struct fib_result res = { 0 };
 		struct fib_table *local;
@@ -165,6 +155,8 @@ struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref)
 		    !fib_table_lookup(local, &fl4, &res, FIB_LOOKUP_NOREF) &&
 		    res.type == RTN_LOCAL)
 			result = FIB_RES_DEV(res);
+	} else {
+		result = ifa->ifa_dev->dev;
 	}
 	if (result && devref)
 		dev_hold(result);
@@ -173,6 +165,20 @@ struct net_device *__ip_dev_find(struct net *net, __be32 addr, bool devref)
 }
 EXPORT_SYMBOL(__ip_dev_find);
 
+/* called under RCU lock */
+struct in_ifaddr *inet_lookup_ifaddr_rcu(struct net *net, __be32 addr)
+{
+	u32 hash = inet_addr_hash(net, addr);
+	struct in_ifaddr *ifa;
+
+	hlist_for_each_entry_rcu(ifa, &inet_addr_lst[hash], hash)
+		if (ifa->ifa_local == addr &&
+		    net_eq(dev_net(ifa->ifa_dev->dev), net))
+			return ifa;
+
+	return NULL;
+}
+
 static void rtmsg_ifa(int event, struct in_ifaddr *, struct nlmsghdr *, u32);
 
 static BLOCKING_NOTIFIER_HEAD(inetaddr_chain);
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 37819ab4cc74..f02819134ba2 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -345,9 +345,6 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
 	if (res.type != RTN_UNICAST &&
 	    (res.type != RTN_LOCAL || !IN_DEV_ACCEPT_LOCAL(idev)))
 		goto e_inval;
-	if (!rpf && !fib_num_tclassid_users(net) &&
-	    (dev->ifindex != oif || !IN_DEV_TX_REDIRECTS(idev)))
-		goto last_resort;
 	fib_combine_itag(itag, &res);
 	dev_match = false;
 
@@ -402,13 +399,26 @@ int fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
 			struct in_device *idev, u32 *itag)
 {
 	int r = secpath_exists(skb) ? 0 : IN_DEV_RPFILTER(idev);
+	struct net *net = dev_net(dev);
 
-	if (!r && !fib_num_tclassid_users(dev_net(dev)) &&
-	    IN_DEV_ACCEPT_LOCAL(idev) &&
+	if (!r && !fib_num_tclassid_users(net) &&
 	    (dev->ifindex != oif || !IN_DEV_TX_REDIRECTS(idev))) {
+		if (IN_DEV_ACCEPT_LOCAL(idev))
+			goto ok;
+		/* if no local routes are added from user space we can check
+		 * for local addresses looking-up the ifaddr table
+		 */
+		if (net->ipv4.fib_has_custom_local_routes)
+			goto full_check;
+		if (inet_lookup_ifaddr_rcu(net, src))
+			return -EINVAL;
+
+ok:
 		*itag = 0;
 		return 0;
 	}
+
+full_check:
 	return __fib_validate_source(skb, src, dst, tos, oif, dev, r, idev, itag);
 }
 
@@ -759,6 +769,8 @@ static int inet_rtm_newroute(struct sk_buff *skb, struct nlmsghdr *nlh,
 	}
 
 	err = fib_table_insert(net, tb, &cfg, extack);
+	if (!err && cfg.fc_type == RTN_LOCAL)
+		net->ipv4.fib_has_custom_local_routes = true;
 errout:
 	return err;
 }
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-21 22:15 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-19 10:46 [PATCH net-next] net: avoid a full fib lookup when rp_filter is disabled Paolo Abeni
2017-09-19 12:00 ` Eric Dumazet
2017-09-19 12:46   ` Paolo Abeni
2017-09-20 16:26 Paolo Abeni
2017-09-20 16:37 ` Paolo Abeni
2017-09-21 22:15 ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.