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 >