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(-)
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;
--
2.39.5On 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
>
© 2016 - 2024 Red Hat, Inc.