| Message ID | 20241007074702.249543-2-dongml2@chinatelecom.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ip: add drop reasons to input route | expand |
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@chinatelecom.cn> 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? Thanks! Paolo
On Thu, Oct 10, 2024 at 4:25 PM Paolo Abeni <pabeni@redhat.com> 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@chinatelecom.cn> > > 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. Thanks! Menglong Dong > Thanks! > > Paolo >
On Thu, Oct 10, 2024 at 5:18 PM Menglong Dong <menglong8.dong@gmail.com> wrote: > > On Thu, Oct 10, 2024 at 4:25 PM Paolo Abeni <pabeni@redhat.com> 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@chinatelecom.cn> > > > > 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! Menglong Dong > > > Thanks! > > > > Paolo > >
On 10/11/24 08:42, Menglong Dong wrote: > On Thu, Oct 10, 2024 at 5:18 PM Menglong Dong <menglong8.dong@gmail.com> wrote: >> On Thu, Oct 10, 2024 at 4:25 PM Paolo Abeni <pabeni@redhat.com> 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@chinatelecom.cn> >>> >>> 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. Thanks! Paolo
On Fri, Oct 11, 2024 at 4:49 PM Paolo Abeni <pabeni@redhat.com> wrote: > > On 10/11/24 08:42, Menglong Dong wrote: > > On Thu, Oct 10, 2024 at 5:18 PM Menglong Dong <menglong8.dong@gmail.com> wrote: > >> On Thu, Oct 10, 2024 at 4:25 PM Paolo Abeni <pabeni@redhat.com> 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@chinatelecom.cn> > >>> > >>> 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 >
diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h index 4748680e8c88..76504e25d581 100644 --- a/include/net/dropreason-core.h +++ b/include/net/dropreason-core.h @@ -76,6 +76,8 @@ FN(INVALID_PROTO) \ FN(IP_INADDRERRORS) \ FN(IP_INNOROUTES) \ + FN(IP_LOCAL_SOURCE) \ + FN(IP_INVALID_SOURCE) \ FN(PKT_TOO_BIG) \ FN(DUP_FRAG) \ FN(FRAG_REASM_TIMEOUT) \ @@ -365,6 +367,14 @@ enum skb_drop_reason { * IPSTATS_MIB_INADDRERRORS */ SKB_DROP_REASON_IP_INNOROUTES, + /** @SKB_DROP_REASON_IP_LOCAL_SOURCE: the source ip is local */ + SKB_DROP_REASON_IP_LOCAL_SOURCE, + /** + * @SKB_DROP_REASON_IP_INVALID_SOURCE: the source ip is invalid: + * 1) source ip is multicast or limited broadcast + * 2) source ip is zero and not IGMP + */ + SKB_DROP_REASON_IP_INVALID_SOURCE, /** * @SKB_DROP_REASON_PKT_TOO_BIG: packet size is too big (maybe exceed the * MTU) diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c index 793e6781399a..779c90de3a54 100644 --- a/net/ipv4/fib_frontend.c +++ b/net/ipv4/fib_frontend.c @@ -346,6 +346,7 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst, int rpf, struct in_device *idev, u32 *itag) { struct net *net = dev_net(dev); + enum skb_drop_reason reason; struct flow_keys flkeys; int ret, no_addr; struct fib_result res; @@ -377,9 +378,15 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst, if (fib_lookup(net, &fl4, &res, 0)) goto last_resort; - if (res.type != RTN_UNICAST && - (res.type != RTN_LOCAL || !IN_DEV_ACCEPT_LOCAL(idev))) - goto e_inval; + if (res.type != RTN_UNICAST) { + if (res.type != RTN_LOCAL) { + reason = SKB_DROP_REASON_IP_INVALID_SOURCE; + goto e_inval; + } else if (!IN_DEV_ACCEPT_LOCAL(idev)) { + reason = SKB_DROP_REASON_IP_LOCAL_SOURCE; + goto e_inval; + } + } fib_combine_itag(itag, &res); dev_match = fib_info_nh_uses_dev(res.fi, dev); @@ -412,9 +419,9 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst, return 0; e_inval: - return -EINVAL; + return -reason; e_rpf: - return -EXDEV; + return -SKB_DROP_REASON_IP_RPFILTER; } /* Ignore rp_filter for packets protected by IPsec. */ @@ -440,7 +447,7 @@ int fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst, * and the same host but different containers are not. */ if (inet_lookup_ifaddr_rcu(net, src)) - return -EINVAL; + return -SKB_DROP_REASON_IP_LOCAL_SOURCE; ok: *itag = 0; diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c index c0a2490eb7c1..a6f5bfc274ee 100644 --- a/net/ipv4/ip_input.c +++ b/net/ipv4/ip_input.c @@ -425,10 +425,8 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk, return NET_RX_DROP; drop_error: - if (err == -EXDEV) { - drop_reason = SKB_DROP_REASON_IP_RPFILTER; + if (drop_reason == SKB_DROP_REASON_IP_RPFILTER) __NET_INC_STATS(net, LINUX_MIB_IPRPFILTER); - } goto drop; } diff --git a/net/ipv4/route.c b/net/ipv4/route.c index 6e1cd0065b87..e49b4ce1804a 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -1690,7 +1690,7 @@ int ip_mc_validate_source(struct sk_buff *skb, __be32 daddr, __be32 saddr, err = fib_validate_source(skb, saddr, 0, tos, 0, dev, in_dev, itag); if (err < 0) - return err; + return -EINVAL; } return 0; } @@ -1788,6 +1788,7 @@ static int __mkroute_input(struct sk_buff *skb, err = fib_validate_source(skb, saddr, daddr, tos, FIB_RES_OIF(*res), in_dev->dev, in_dev, &itag); if (err < 0) { + err = -EINVAL; ip_handle_martian_source(in_dev->dev, in_dev, skb, daddr, saddr); @@ -2162,8 +2163,10 @@ int ip_route_use_hint(struct sk_buff *skb, __be32 daddr, __be32 saddr, tos &= INET_DSCP_MASK; err = fib_validate_source(skb, saddr, daddr, tos, 0, dev, in_dev, &tag); - if (err < 0) + if (err < 0) { + err = -EINVAL; goto martian_source; + } skip_validate_source: skb_dst_copy(skb, hint); @@ -2302,8 +2305,10 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr, err = fib_validate_source(skb, saddr, daddr, inet_dscp_to_dsfield(dscp), 0, dev, in_dev, &itag); - if (err < 0) + if (err < 0) { + err = -EINVAL; goto martian_source; + } goto local_input; } @@ -2327,8 +2332,10 @@ out: return err; err = fib_validate_source(skb, saddr, 0, inet_dscp_to_dsfield(dscp), 0, dev, in_dev, &itag); - if (err < 0) + if (err < 0) { + err = -EINVAL; goto martian_source; + } } flags |= RTCF_BROADCAST; res->type = RTN_BROADCAST;
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@chinatelecom.cn> --- include/net/dropreason-core.h | 10 ++++++++++ net/ipv4/fib_frontend.c | 19 +++++++++++++------ net/ipv4/ip_input.c | 4 +--- net/ipv4/route.c | 15 +++++++++++---- 4 files changed, 35 insertions(+), 13 deletions(-)