| Message ID | 20241001060005.418231-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 |
no longer applies, please respin On Tue, 1 Oct 2024 13:59:59 +0800 Menglong Dong wrote: > + enum skb_drop_reason drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; > const struct iphdr *iph = ip_hdr(skb); > - int err, drop_reason; > + int err; > struct rtable *rt; reverse xmas tree > > - drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; > - > if (ip_can_use_hint(skb, iph, hint)) { > err = ip_route_use_hint(skb, iph->daddr, iph->saddr, iph->tos, > dev, hint); > @@ -363,7 +362,7 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk, > */ > if (!skb_valid_dst(skb)) { > err = ip_route_input_noref(skb, iph->daddr, iph->saddr, > - iph->tos, dev); > + iph->tos, dev, &drop_reason); I find the extra output argument quite ugly. I can't apply this now to try to suggest something better, perhaps you can come up with a better solution..
On Fri, Oct 4, 2024 at 6:36 PM Jakub Kicinski <kuba@kernel.org> wrote: > > no longer applies, please respin > > On Tue, 1 Oct 2024 13:59:59 +0800 Menglong Dong wrote: > > + enum skb_drop_reason drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; > > const struct iphdr *iph = ip_hdr(skb); > > - int err, drop_reason; > > + int err; > > struct rtable *rt; > > reverse xmas tree > > > > > - drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; > > - > > if (ip_can_use_hint(skb, iph, hint)) { > > err = ip_route_use_hint(skb, iph->daddr, iph->saddr, iph->tos, > > dev, hint); > > @@ -363,7 +362,7 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk, > > */ > > if (!skb_valid_dst(skb)) { > > err = ip_route_input_noref(skb, iph->daddr, iph->saddr, > > - iph->tos, dev); > > + iph->tos, dev, &drop_reason); > > I find the extra output argument quite ugly. > I can't apply this now to try to suggest something better, perhaps you > can come up with a better solution.. Also, passing a local variable by address forces the compiler to emit more canary checks in more networking core functions. See : config STACKPROTECTOR_STRONG bool "Strong Stack Protector" depends on STACKPROTECTOR depends on $(cc-option,-fstack-protector-strong) default y help Functions will have the stack-protector canary logic added in any of the following conditions: - local variable's address used as part of the right hand side of an assignment or function argument - local variable is an array (or union containing an array), regardless of array type or length - uses register local variables This feature requires gcc version 4.9 or above, or a distribution gcc with the feature backported ("-fstack-protector-strong"). On an x86 "defconfig" build, this feature adds canary checks to about 20% of all kernel functions, which increases the kernel code size by about 2%.
On Sat, Oct 5, 2024 at 12:54 AM Eric Dumazet <edumazet@google.com> wrote: > > On Fri, Oct 4, 2024 at 6:36 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > no longer applies, please respin > > > > On Tue, 1 Oct 2024 13:59:59 +0800 Menglong Dong wrote: > > > + enum skb_drop_reason drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; > > > const struct iphdr *iph = ip_hdr(skb); > > > - int err, drop_reason; > > > + int err; > > > struct rtable *rt; > > > > reverse xmas tree > > > > > > > > - drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; > > > - > > > if (ip_can_use_hint(skb, iph, hint)) { > > > err = ip_route_use_hint(skb, iph->daddr, iph->saddr, iph->tos, > > > dev, hint); > > > @@ -363,7 +362,7 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk, > > > */ > > > if (!skb_valid_dst(skb)) { > > > err = ip_route_input_noref(skb, iph->daddr, iph->saddr, > > > - iph->tos, dev); > > > + iph->tos, dev, &drop_reason); > > > > I find the extra output argument quite ugly. > > I can't apply this now to try to suggest something better, perhaps you > > can come up with a better solution.. > > Also, passing a local variable by address forces the compiler to emit > more canary checks in more > networking core functions. > Yeah, passing the address of the drop reasons to a function looks not nice. The first glance for me is to make ip_route_input_noref() return drop reasons, but I'm afraid that the errno it returns is used by the caller. Let me dig it deeper, and make the functions in this series return drop reasons, instead of passing a local variable. Thanks! Menglong Dong > > See : > > > config STACKPROTECTOR_STRONG > bool "Strong Stack Protector" > depends on STACKPROTECTOR > depends on $(cc-option,-fstack-protector-strong) > default y > help > Functions will have the stack-protector canary logic added in any > of the following conditions: > > - local variable's address used as part of the right hand side of an > assignment or function argument > - local variable is an array (or union containing an array), > regardless of array type or length > - uses register local variables > > This feature requires gcc version 4.9 or above, or a distribution > gcc with the feature backported ("-fstack-protector-strong"). > > On an x86 "defconfig" build, this feature adds canary checks to > about 20% of all kernel functions, which increases the kernel code > size by about 2%.
diff --git a/drivers/net/ipvlan/ipvlan_l3s.c b/drivers/net/ipvlan/ipvlan_l3s.c index d5b05e803219..fbfdd8c00056 100644 --- a/drivers/net/ipvlan/ipvlan_l3s.c +++ b/drivers/net/ipvlan/ipvlan_l3s.c @@ -52,7 +52,7 @@ static struct sk_buff *ipvlan_l3_rcv(struct net_device *dev, int err; err = ip_route_input_noref(skb, ip4h->daddr, ip4h->saddr, - ip4h->tos, sdev); + ip4h->tos, sdev, NULL); if (unlikely(err)) goto out; break; diff --git a/include/net/route.h b/include/net/route.h index 1789f1e6640b..cb9f31080517 100644 --- a/include/net/route.h +++ b/include/net/route.h @@ -202,7 +202,8 @@ int ip_mc_validate_source(struct sk_buff *skb, __be32 daddr, __be32 saddr, u8 tos, struct net_device *dev, struct in_device *in_dev, u32 *itag); int ip_route_input_noref(struct sk_buff *skb, __be32 dst, __be32 src, - u8 tos, struct net_device *devin); + u8 tos, struct net_device *devin, + enum skb_drop_reason *reason); int ip_route_use_hint(struct sk_buff *skb, __be32 dst, __be32 src, u8 tos, struct net_device *devin, const struct sk_buff *hint); @@ -213,7 +214,7 @@ static inline int ip_route_input(struct sk_buff *skb, __be32 dst, __be32 src, int err; rcu_read_lock(); - err = ip_route_input_noref(skb, dst, src, tos, devin); + err = ip_route_input_noref(skb, dst, src, tos, devin, NULL); if (!err) { skb_dst_force(skb); if (!skb_dst(skb)) diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c index 1a14f915b7a4..df50f2977c90 100644 --- a/net/core/lwt_bpf.c +++ b/net/core/lwt_bpf.c @@ -96,7 +96,7 @@ static int bpf_lwt_input_reroute(struct sk_buff *skb) dev_hold(dev); skb_dst_drop(skb); err = ip_route_input_noref(skb, iph->daddr, iph->saddr, - iph->tos, dev); + iph->tos, dev, NULL); dev_put(dev); } else if (skb->protocol == htons(ETH_P_IPV6)) { skb_dst_drop(skb); diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c index 11c1519b3699..a9dac0ef2be6 100644 --- a/net/ipv4/arp.c +++ b/net/ipv4/arp.c @@ -835,7 +835,7 @@ static int arp_process(struct net *net, struct sock *sk, struct sk_buff *skb) } if (arp->ar_op == htons(ARPOP_REQUEST) && - ip_route_input_noref(skb, tip, sip, 0, dev) == 0) { + ip_route_input_noref(skb, tip, sip, 0, dev, NULL) == 0) { rt = skb_rtable(skb); addr_type = rt->rt_type; diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c index a92664a5ef2e..cdc75cfc1826 100644 --- a/net/ipv4/ip_fragment.c +++ b/net/ipv4/ip_fragment.c @@ -176,7 +176,7 @@ static void ip_expire(struct timer_list *t) /* skb has no dst, perform route lookup again */ iph = ip_hdr(head); err = ip_route_input_noref(head, iph->daddr, iph->saddr, - iph->tos, head->dev); + iph->tos, head->dev, NULL); if (err) goto out; diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c index b6e7d4921309..dc062ae49137 100644 --- a/net/ipv4/ip_input.c +++ b/net/ipv4/ip_input.c @@ -318,12 +318,11 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk, struct sk_buff *skb, struct net_device *dev, const struct sk_buff *hint) { + enum skb_drop_reason drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; const struct iphdr *iph = ip_hdr(skb); - int err, drop_reason; + int err; struct rtable *rt; - drop_reason = SKB_DROP_REASON_NOT_SPECIFIED; - if (ip_can_use_hint(skb, iph, hint)) { err = ip_route_use_hint(skb, iph->daddr, iph->saddr, iph->tos, dev, hint); @@ -363,7 +362,7 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk, */ if (!skb_valid_dst(skb)) { err = ip_route_input_noref(skb, iph->daddr, iph->saddr, - iph->tos, dev); + iph->tos, dev, &drop_reason); if (unlikely(err)) goto drop_error; } else { diff --git a/net/ipv4/route.c b/net/ipv4/route.c index 723ac9181558..f1767e0cc9d9 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -2465,7 +2465,8 @@ static int ip_route_input_rcu(struct sk_buff *skb, __be32 daddr, __be32 saddr, } int ip_route_input_noref(struct sk_buff *skb, __be32 daddr, __be32 saddr, - u8 tos, struct net_device *dev) + u8 tos, struct net_device *dev, + enum skb_drop_reason *reason) { struct fib_result res; int err; diff --git a/net/ipv4/xfrm4_input.c b/net/ipv4/xfrm4_input.c index a620618cc568..14990cc30c68 100644 --- a/net/ipv4/xfrm4_input.c +++ b/net/ipv4/xfrm4_input.c @@ -33,7 +33,7 @@ static inline int xfrm4_rcv_encap_finish(struct net *net, struct sock *sk, const struct iphdr *iph = ip_hdr(skb); if (ip_route_input_noref(skb, iph->daddr, iph->saddr, - iph->tos, skb->dev)) + iph->tos, skb->dev, NULL)) goto drop; } diff --git a/net/ipv4/xfrm4_protocol.c b/net/ipv4/xfrm4_protocol.c index b146ce88c5d0..9678ff876169 100644 --- a/net/ipv4/xfrm4_protocol.c +++ b/net/ipv4/xfrm4_protocol.c @@ -76,7 +76,7 @@ int xfrm4_rcv_encap(struct sk_buff *skb, int nexthdr, __be32 spi, const struct iphdr *iph = ip_hdr(skb); if (ip_route_input_noref(skb, iph->daddr, iph->saddr, - iph->tos, skb->dev)) + iph->tos, skb->dev, NULL)) goto drop; }
The errno which ip_route_input_noref() returns can be used and checked by the caller, so it's complex to make ip_route_input_noref() return drop reason. Instead, we add the pointer of the skb drop reason to the function arguments of ip_route_input_noref, and adjust all the callers of it. Then, we can pass the skb drop reasons to the caller. Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn> --- drivers/net/ipvlan/ipvlan_l3s.c | 2 +- include/net/route.h | 5 +++-- net/core/lwt_bpf.c | 2 +- net/ipv4/arp.c | 2 +- net/ipv4/ip_fragment.c | 2 +- net/ipv4/ip_input.c | 7 +++---- net/ipv4/route.c | 3 ++- net/ipv4/xfrm4_input.c | 2 +- net/ipv4/xfrm4_protocol.c | 2 +- 9 files changed, 14 insertions(+), 13 deletions(-)