Re: [PATCH net-next v2 1/7] net: ip: make fib_validate_source() return drop reason

[Menglong Dong]

On Fri, Oct 11, 2024 at 4:49 PM Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
>
> On 10/11/24 08:42, Menglong Dong wrote:
> > On Thu, Oct 10, 2024 at 5:18 PM Menglong Dong <menglong8.dong@xxxxxxxxx> wrote:
> >> On Thu, Oct 10, 2024 at 4:25 PM Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
> >>> On 10/7/24 09:46, Menglong Dong wrote:
> >>>> In this commit, we make fib_validate_source/__fib_validate_source return
> >>>> -reason instead of errno on error. As the return value of them can be
> >>>> -errno, 0, and 1, we can't make it return enum skb_drop_reason directly.
> >>>>
> >>>> In the origin logic, if __fib_validate_source() return -EXDEV,
> >>>> LINUX_MIB_IPRPFILTER will be counted. And now, we need to adjust it by
> >>>> checking "reason == SKB_DROP_REASON_IP_RPFILTER". However, this will take
> >>>> effect only after the patch "net: ip: make ip_route_input_noref() return
> >>>> drop reasons", as we can't pass the drop reasons from
> >>>> fib_validate_source() to ip_rcv_finish_core() in this patch.
> >>>>
> >>>> We set the errno to -EINVAL when fib_validate_source() is called and the
> >>>> validation fails, as the errno can be checked in the caller and now its
> >>>> value is -reason, which can lead misunderstand.
> >>>>
> >>>> Following new drop reasons are added in this patch:
> >>>>
> >>>>     SKB_DROP_REASON_IP_LOCAL_SOURCE
> >>>>     SKB_DROP_REASON_IP_INVALID_SOURCE
> >>>>
> >>>> Signed-off-by: Menglong Dong <dongml2@xxxxxxxxxxxxxxx>
> >>>
> >>> Looking at the next patches, I'm under the impression that the overall
> >>> code will be simpler if you let __fib_validate_source() return directly
> >>> a drop reason, and fib_validate_source(), too. Hard to be sure without
> >>> actually do the attempt... did you try such patch by any chance?
> >>>
> >>
> >> I analysed the usages of fib_validate_source() before. The
> >> return value of fib_validate_source() can be -errno, "0", and "1".
> >> And the value "1" can be used by the caller, such as
> >> __mkroute_input(). Making it return drop reasons can't cover this
> >> case.
> >>
> >> It seems that __mkroute_input() is the only case that uses the
> >> positive returning value of fib_validate_source(). Let me think
> >> about it more in this case.
> >
> > Hello,
> >
> > After digging into the code of __fib_validate_source() and __mkroute_input(),
> > I think it's hard to make __fib_validate_source() return drop reasons
> > directly.
> >
> > The __fib_validate_source() will return 1 if the scope of the
> > source(revert) route is HOST. And the __mkroute_input()
> > will mark the skb with IPSKB_DOREDIRECT in this
> > case (combine with some other conditions). And then, a REDIRECT
> > ICMP will be sent in ip_forward() if this flag exists.
> >
> > I don't find a way to pass this information to __mkroute_input
> > if we make __fib_validate_source() return drop reasons. Can we?
> >
> > An option is to add a wrapper for fib_validate_source(), such as
> > fib_validate_source_reason(), which returns drop reasons. And in
> > __mkroute_input(), we still call fib_validate_source().
> >
> > What do you think?
>
> Thanks for the investigation. I see that let __fib_validate_source()
> returning drop reasons does not look like a good design.
>
> I think the additional helper will not help much, so I guess you can
> retain the current implementation here, but please expand the commit
> message with the above information.

Hello,

I have implemented a new version just now like this:

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

Then, we can make fib_validate_source() return drop reasons. And
we call __fib_validate_source() in __mkroute_input(), which makes
the logic here remains unchanged.

What do you think? Or do we retain the current implementation here?

Following is the part patch that refactor
fib_validate_source/__fib_validate_source:

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index 06130933542d..ea51cae24fad 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;


Thanks!
Menglong Dong

>
> Thanks!
>
> Paolo
>