[PATCH net-next v3 01/10] net: ip: refactor fib_validate_source/__fib_validate_source

Menglong Dong posted 10 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH net-next v3 01/10] net: ip: refactor fib_validate_source/__fib_validate_source
Posted by Menglong Dong 1 month, 1 week ago
The only caller of __fib_validate_source() is fib_validate_source(), so
we can combine fib_validate_source() into __fib_validate_source(), and
make fib_validate_source() an inline call to __fib_validate_source().

This will make it easier to make fib_validate_source() return drop reasons
in the next patch.

Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
---
 include/net/ip_fib.h    | 15 ++++++++--
 net/ipv4/fib_frontend.c | 64 +++++++++++++++++------------------------
 2 files changed, 38 insertions(+), 41 deletions(-)

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index b6e44f4eaa4c..90ff815f212b 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -448,9 +448,18 @@ int fib_gw_from_via(struct fib_config *cfg, struct nlattr *nla,
 		    struct netlink_ext_ack *extack);
 __be32 fib_compute_spec_dst(struct sk_buff *skb);
 bool fib_info_nh_uses_dev(struct fib_info *fi, const struct net_device *dev);
-int fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
-			dscp_t dscp, int oif, struct net_device *dev,
-			struct in_device *idev, u32 *itag);
+int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
+			  dscp_t dscp, int oif, struct net_device *dev,
+			  struct in_device *idev, u32 *itag);
+
+static inline int
+fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
+		    dscp_t dscp, int oif, struct net_device *dev,
+		    struct in_device *idev, u32 *itag)
+{
+	return __fib_validate_source(skb, src, dst, dscp, oif, dev, idev,
+				     itag);
+}
 
 #ifdef CONFIG_IP_ROUTE_CLASSID
 static inline int fib_num_tclassid_users(struct net *net)
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 8353518b110a..f74138f4d748 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -341,10 +341,11 @@ EXPORT_SYMBOL_GPL(fib_info_nh_uses_dev);
  * - check, that packet arrived from expected physical interface.
  * called with rcu_read_lock()
  */
-static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
-				 dscp_t dscp, int oif, struct net_device *dev,
-				 int rpf, struct in_device *idev, u32 *itag)
+int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
+			  dscp_t dscp, int oif, struct net_device *dev,
+			  struct in_device *idev, u32 *itag)
 {
+	int rpf = secpath_exists(skb) ? 0 : IN_DEV_RPFILTER(idev);
 	struct net *net = dev_net(dev);
 	struct flow_keys flkeys;
 	int ret, no_addr;
@@ -352,6 +353,28 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
 	struct flowi4 fl4;
 	bool dev_match;
 
+	/* Ignore rp_filter for packets protected by IPsec. */
+	if (!rpf && !fib_num_tclassid_users(net) &&
+	    (dev->ifindex != oif || !IN_DEV_TX_REDIRECTS(idev))) {
+		if (IN_DEV_ACCEPT_LOCAL(idev))
+			goto last_resort;
+		/* with custom local routes in place, checking local addresses
+		 * only will be too optimistic, with custom rules, checking
+		 * local addresses only can be too strict, e.g. due to vrf
+		 */
+		if (net->ipv4.fib_has_custom_local_routes ||
+		    fib4_has_custom_rules(net))
+			goto full_check;
+		/* Within the same container, it is regarded as a martian source,
+		 * and the same host but different containers are not.
+		 */
+		if (inet_lookup_ifaddr_rcu(net, src))
+			return -EINVAL;
+
+		goto last_resort;
+	}
+
+full_check:
 	fl4.flowi4_oif = 0;
 	fl4.flowi4_l3mdev = l3mdev_master_ifindex_rcu(dev);
 	fl4.flowi4_iif = oif ? : LOOPBACK_IFINDEX;
@@ -417,41 +440,6 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
 	return -EXDEV;
 }
 
-/* Ignore rp_filter for packets protected by IPsec. */
-int fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
-			dscp_t dscp, int oif, struct net_device *dev,
-			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(net) &&
-	    (dev->ifindex != oif || !IN_DEV_TX_REDIRECTS(idev))) {
-		if (IN_DEV_ACCEPT_LOCAL(idev))
-			goto ok;
-		/* with custom local routes in place, checking local addresses
-		 * only will be too optimistic, with custom rules, checking
-		 * local addresses only can be too strict, e.g. due to vrf
-		 */
-		if (net->ipv4.fib_has_custom_local_routes ||
-		    fib4_has_custom_rules(net))
-			goto full_check;
-		/* Within the same container, it is regarded as a martian source,
-		 * and the same host but different containers are not.
-		 */
-		if (inet_lookup_ifaddr_rcu(net, src))
-			return -EINVAL;
-
-ok:
-		*itag = 0;
-		return 0;
-	}
-
-full_check:
-	return __fib_validate_source(skb, src, dst, dscp, oif, dev, r, idev,
-				     itag);
-}
-
 static inline __be32 sk_extract_addr(struct sockaddr *addr)
 {
 	return ((struct sockaddr_in *) addr)->sin_addr.s_addr;
-- 
2.39.5
Re: [PATCH net-next v3 01/10] net: ip: refactor fib_validate_source/__fib_validate_source
Posted by Paolo Abeni 1 month ago
On 10/15/24 16:07, Menglong Dong wrote:
> @@ -352,6 +353,28 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
>  	struct flowi4 fl4;
>  	bool dev_match;
>  
> +	/* Ignore rp_filter for packets protected by IPsec. */
> +	if (!rpf && !fib_num_tclassid_users(net) &&
> +	    (dev->ifindex != oif || !IN_DEV_TX_REDIRECTS(idev))) {
> +		if (IN_DEV_ACCEPT_LOCAL(idev))
> +			goto last_resort;

IMHO the re-usage of the 'last_resort' macro makes the patch a little
hard to read, as this is an 'accept' condition. I think it would be
better to retain the original code. If you really want to avoid the
small duplication, you could instead introduce an 'ok' label towards the
end of this function:

last_resort:
        if (rpf)
                goto e_rpf;

ok:
        *itag = 0;
        return 0;

And jump there.

Thanks,

Paolo