| Message ID | 20250312103246.16206-1-justin.iurman@uliege.be (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: lwtunnel: fix recursion loops | expand |
On 3/12/25 11:32, Justin Iurman wrote: > Different kind of loops in most of lwtunnel users were fixed by some > recent patches. This patch acts as a parachute, catch all solution, by > detecting any use cases with recursion and taking care of them (e.g., a > loop between routes). This is applied to lwtunnel_input(), > lwtunnel_output(), and lwtunnel_xmit(). > > Fixes: ffce41962ef6 ("lwtunnel: support dst output redirect function") > Fixes: 2536862311d2 ("lwt: Add support to redirect dst.input") > Fixes: 14972cbd34ff ("net: lwtunnel: Handle fragmentation") > Closes: https://lore.kernel.org/netdev/2bc9e2079e864a9290561894d2a602d6@akamai.com/ > Cc: Roopa Prabhu <roopa@nvidia.com> > Cc: Andrea Mayer <andrea.mayer@uniroma2.it> > Cc: Stefano Salsano <stefano.salsano@uniroma2.it> > Cc: Ahmed Abdelsalam <ahabdels.dev@gmail.com> > Cc: Ido Schimmel <idosch@nvidia.com> > Signed-off-by: Justin Iurman <justin.iurman@uliege.be> > --- > net/core/lwtunnel.c | 65 ++++++++++++++++++++++++++++++++++++--------- > net/core/lwtunnel.h | 42 +++++++++++++++++++++++++++++ > 2 files changed, 95 insertions(+), 12 deletions(-) > create mode 100644 net/core/lwtunnel.h > > diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c > index 711cd3b4347a..0954783e36ce 100644 > --- a/net/core/lwtunnel.c > +++ b/net/core/lwtunnel.c > @@ -23,6 +23,8 @@ > #include <net/ip6_fib.h> > #include <net/rtnh.h> > > +#include "lwtunnel.h" > + > DEFINE_STATIC_KEY_FALSE(nf_hooks_lwtunnel_enabled); > EXPORT_SYMBOL_GPL(nf_hooks_lwtunnel_enabled); > > @@ -325,13 +327,23 @@ EXPORT_SYMBOL_GPL(lwtunnel_cmp_encap); > > int lwtunnel_output(struct net *net, struct sock *sk, struct sk_buff *skb) > { > - struct dst_entry *dst = skb_dst(skb); > const struct lwtunnel_encap_ops *ops; > struct lwtunnel_state *lwtstate; > - int ret = -EINVAL; > + struct dst_entry *dst; > + int ret; > + > + if (lwtunnel_recursion()) { > + net_crit_ratelimited("%s(): recursion limit reached on datapath\n", > + __func__); > + ret = -ENETDOWN; > + goto drop; > + } > > - if (!dst) > + dst = skb_dst(skb); > + if (!dst) { > + ret = -EINVAL; > goto drop; > + } > lwtstate = dst->lwtstate; > > if (lwtstate->type == LWTUNNEL_ENCAP_NONE || > @@ -341,8 +353,11 @@ int lwtunnel_output(struct net *net, struct sock *sk, struct sk_buff *skb) > ret = -EOPNOTSUPP; > rcu_read_lock(); > ops = rcu_dereference(lwtun_encaps[lwtstate->type]); > - if (likely(ops && ops->output)) > + if (likely(ops && ops->output)) { > + lwtunnel_recursion_inc(); > ret = ops->output(net, sk, skb); > + lwtunnel_recursion_dec(); > + } > rcu_read_unlock(); > > if (ret == -EOPNOTSUPP) > @@ -359,13 +374,23 @@ EXPORT_SYMBOL_GPL(lwtunnel_output); > > int lwtunnel_xmit(struct sk_buff *skb) > { > - struct dst_entry *dst = skb_dst(skb); > const struct lwtunnel_encap_ops *ops; > struct lwtunnel_state *lwtstate; > - int ret = -EINVAL; > + struct dst_entry *dst; > + int ret; > + > + if (lwtunnel_recursion()) { > + net_crit_ratelimited("%s(): recursion limit reached on datapath\n", > + __func__); > + ret = -ENETDOWN; > + goto drop; > + } > > - if (!dst) > + dst = skb_dst(skb); > + if (!dst) { > + ret = -EINVAL; > goto drop; > + } > > lwtstate = dst->lwtstate; > > @@ -376,8 +401,11 @@ int lwtunnel_xmit(struct sk_buff *skb) > ret = -EOPNOTSUPP; > rcu_read_lock(); > ops = rcu_dereference(lwtun_encaps[lwtstate->type]); > - if (likely(ops && ops->xmit)) > + if (likely(ops && ops->xmit)) { > + lwtunnel_recursion_inc(); > ret = ops->xmit(skb); > + lwtunnel_recursion_dec(); > + } > rcu_read_unlock(); > > if (ret == -EOPNOTSUPP) > @@ -394,13 +422,23 @@ EXPORT_SYMBOL_GPL(lwtunnel_xmit); > > int lwtunnel_input(struct sk_buff *skb) > { > - struct dst_entry *dst = skb_dst(skb); > const struct lwtunnel_encap_ops *ops; > struct lwtunnel_state *lwtstate; > - int ret = -EINVAL; > + struct dst_entry *dst; > + int ret; > > - if (!dst) > + if (lwtunnel_recursion()) { > + net_crit_ratelimited("%s(): recursion limit reached on datapath\n", > + __func__); > + ret = -ENETDOWN; > goto drop; > + } > + > + dst = skb_dst(skb); > + if (!dst) { > + ret = -EINVAL; > + goto drop; > + } > lwtstate = dst->lwtstate; > > if (lwtstate->type == LWTUNNEL_ENCAP_NONE || > @@ -410,8 +448,11 @@ int lwtunnel_input(struct sk_buff *skb) > ret = -EOPNOTSUPP; > rcu_read_lock(); > ops = rcu_dereference(lwtun_encaps[lwtstate->type]); > - if (likely(ops && ops->input)) > + if (likely(ops && ops->input)) { > + lwtunnel_recursion_inc(); > ret = ops->input(skb); > + lwtunnel_recursion_dec(); > + } > rcu_read_unlock(); > > if (ret == -EOPNOTSUPP) > diff --git a/net/core/lwtunnel.h b/net/core/lwtunnel.h > new file mode 100644 > index 000000000000..32880ecdd8bb > --- /dev/null > +++ b/net/core/lwtunnel.h > @@ -0,0 +1,42 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +#ifndef _NET_CORE_LWTUNNEL_H > +#define _NET_CORE_LWTUNNEL_H > + > +#include <linux/netdevice.h> > + > +#define LWTUNNEL_RECURSION_LIMIT 8 > + > +#ifndef CONFIG_PREEMPT_RT > +static inline bool lwtunnel_recursion(void) > +{ > + return unlikely(__this_cpu_read(softnet_data.xmit.recursion) > > + LWTUNNEL_RECURSION_LIMIT); > +} > + > +static inline void lwtunnel_recursion_inc(void) > +{ > + __this_cpu_inc(softnet_data.xmit.recursion); > +} > + > +static inline void lwtunnel_recursion_dec(void) > +{ > + __this_cpu_dec(softnet_data.xmit.recursion); > +} > +#else > +static inline bool lwtunnel_recursion(void) > +{ > + return unlikely(current->net_xmit.recursion > LWTUNNEL_RECURSION_LIMIT); > +} > + > +static inline void lwtunnel_recursion_inc(void) > +{ > + current->net_xmit.recursion++; > +} > + > +static inline void lwtunnel_recursion_dec(void) > +{ > + current->net_xmit.recursion--; > +} > +#endif > + > +#endif /* _NET_CORE_LWTUNNEL_H */ Wondering what folks think about the above idea to reuse fields that dev_xmit_recursion() currently uses. IMO, it seems OK considering the use case and context. If not, I guess we'd need to add a new field to both softnet_data and task_struct. Also, with Andrea, we discussed the choice to either keep and send packets, or drop them, in case of pathological configurations. If we were to drop them, then only this patch would suffice, making my series "net: fix lwtunnel reentry loops" not needed anymore (at least for most of lwtunnel users, not for ioam for example where it is needed anyway).
Justin Iurman <justin.iurman@uliege.be> writes: >> --- /dev/null >> +++ b/net/core/lwtunnel.h >> @@ -0,0 +1,42 @@ >> +/* SPDX-License-Identifier: GPL-2.0+ */ >> +#ifndef _NET_CORE_LWTUNNEL_H >> +#define _NET_CORE_LWTUNNEL_H >> + >> +#include <linux/netdevice.h> >> + >> +#define LWTUNNEL_RECURSION_LIMIT 8 >> + >> +#ifndef CONFIG_PREEMPT_RT >> +static inline bool lwtunnel_recursion(void) >> +{ >> + return unlikely(__this_cpu_read(softnet_data.xmit.recursion) > >> + LWTUNNEL_RECURSION_LIMIT); >> +} >> + >> +static inline void lwtunnel_recursion_inc(void) >> +{ >> + __this_cpu_inc(softnet_data.xmit.recursion); >> +} >> + >> +static inline void lwtunnel_recursion_dec(void) >> +{ >> + __this_cpu_dec(softnet_data.xmit.recursion); >> +} >> +#else >> +static inline bool lwtunnel_recursion(void) >> +{ >> + return unlikely(current->net_xmit.recursion > LWTUNNEL_RECURSION_LIMIT); >> +} >> + >> +static inline void lwtunnel_recursion_inc(void) >> +{ >> + current->net_xmit.recursion++; >> +} >> + >> +static inline void lwtunnel_recursion_dec(void) >> +{ >> + current->net_xmit.recursion--; >> +} >> +#endif >> + >> +#endif /* _NET_CORE_LWTUNNEL_H */ > > Wondering what folks think about the above idea to reuse fields that > dev_xmit_recursion() currently uses. IMO, it seems OK considering the > use case and context. If not, I guess we'd need to add a new field to > both softnet_data and task_struct. Why not just reuse the dev_xmit_recursion*() helpers directly? -Toke
On 3/12/25 13:29, Toke Høiland-Jørgensen wrote: > Justin Iurman <justin.iurman@uliege.be> writes: > >>> --- /dev/null >>> +++ b/net/core/lwtunnel.h >>> @@ -0,0 +1,42 @@ >>> +/* SPDX-License-Identifier: GPL-2.0+ */ >>> +#ifndef _NET_CORE_LWTUNNEL_H >>> +#define _NET_CORE_LWTUNNEL_H >>> + >>> +#include <linux/netdevice.h> >>> + >>> +#define LWTUNNEL_RECURSION_LIMIT 8 >>> + >>> +#ifndef CONFIG_PREEMPT_RT >>> +static inline bool lwtunnel_recursion(void) >>> +{ >>> + return unlikely(__this_cpu_read(softnet_data.xmit.recursion) > >>> + LWTUNNEL_RECURSION_LIMIT); >>> +} >>> + >>> +static inline void lwtunnel_recursion_inc(void) >>> +{ >>> + __this_cpu_inc(softnet_data.xmit.recursion); >>> +} >>> + >>> +static inline void lwtunnel_recursion_dec(void) >>> +{ >>> + __this_cpu_dec(softnet_data.xmit.recursion); >>> +} >>> +#else >>> +static inline bool lwtunnel_recursion(void) >>> +{ >>> + return unlikely(current->net_xmit.recursion > LWTUNNEL_RECURSION_LIMIT); >>> +} >>> + >>> +static inline void lwtunnel_recursion_inc(void) >>> +{ >>> + current->net_xmit.recursion++; >>> +} >>> + >>> +static inline void lwtunnel_recursion_dec(void) >>> +{ >>> + current->net_xmit.recursion--; >>> +} >>> +#endif >>> + >>> +#endif /* _NET_CORE_LWTUNNEL_H */ >> >> Wondering what folks think about the above idea to reuse fields that >> dev_xmit_recursion() currently uses. IMO, it seems OK considering the >> use case and context. If not, I guess we'd need to add a new field to >> both softnet_data and task_struct. > > Why not just reuse the dev_xmit_recursion*() helpers directly? > > -Toke > It was my initial idea, but I'm not sure I can. Looks like they're not exposed (it's a local .h in net/core/).
Justin Iurman <justin.iurman@uliege.be> writes: > On 3/12/25 13:29, Toke Høiland-Jørgensen wrote: >> Justin Iurman <justin.iurman@uliege.be> writes: >> >>>> --- /dev/null >>>> +++ b/net/core/lwtunnel.h >>>> @@ -0,0 +1,42 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0+ */ >>>> +#ifndef _NET_CORE_LWTUNNEL_H >>>> +#define _NET_CORE_LWTUNNEL_H >>>> + >>>> +#include <linux/netdevice.h> >>>> + >>>> +#define LWTUNNEL_RECURSION_LIMIT 8 >>>> + >>>> +#ifndef CONFIG_PREEMPT_RT >>>> +static inline bool lwtunnel_recursion(void) >>>> +{ >>>> + return unlikely(__this_cpu_read(softnet_data.xmit.recursion) > >>>> + LWTUNNEL_RECURSION_LIMIT); >>>> +} >>>> + >>>> +static inline void lwtunnel_recursion_inc(void) >>>> +{ >>>> + __this_cpu_inc(softnet_data.xmit.recursion); >>>> +} >>>> + >>>> +static inline void lwtunnel_recursion_dec(void) >>>> +{ >>>> + __this_cpu_dec(softnet_data.xmit.recursion); >>>> +} >>>> +#else >>>> +static inline bool lwtunnel_recursion(void) >>>> +{ >>>> + return unlikely(current->net_xmit.recursion > LWTUNNEL_RECURSION_LIMIT); >>>> +} >>>> + >>>> +static inline void lwtunnel_recursion_inc(void) >>>> +{ >>>> + current->net_xmit.recursion++; >>>> +} >>>> + >>>> +static inline void lwtunnel_recursion_dec(void) >>>> +{ >>>> + current->net_xmit.recursion--; >>>> +} >>>> +#endif >>>> + >>>> +#endif /* _NET_CORE_LWTUNNEL_H */ >>> >>> Wondering what folks think about the above idea to reuse fields that >>> dev_xmit_recursion() currently uses. IMO, it seems OK considering the >>> use case and context. If not, I guess we'd need to add a new field to >>> both softnet_data and task_struct. >> >> Why not just reuse the dev_xmit_recursion*() helpers directly? >> >> -Toke >> > > It was my initial idea, but I'm not sure I can. Looks like they're not > exposed (it's a local .h in net/core/). Well, code can be moved :) Certainly, if you're going to re-implement the helpers with the same underlying data structure, it's better to just move the existing ones and reuse them. -Toke
On 3/12/25 13:32, Justin Iurman wrote: > On 3/12/25 13:29, Toke Høiland-Jørgensen wrote: >> Justin Iurman <justin.iurman@uliege.be> writes: >> >>>> --- /dev/null >>>> +++ b/net/core/lwtunnel.h >>>> @@ -0,0 +1,42 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0+ */ >>>> +#ifndef _NET_CORE_LWTUNNEL_H >>>> +#define _NET_CORE_LWTUNNEL_H >>>> + >>>> +#include <linux/netdevice.h> >>>> + >>>> +#define LWTUNNEL_RECURSION_LIMIT 8 >>>> + >>>> +#ifndef CONFIG_PREEMPT_RT >>>> +static inline bool lwtunnel_recursion(void) >>>> +{ >>>> + return unlikely(__this_cpu_read(softnet_data.xmit.recursion) > >>>> + LWTUNNEL_RECURSION_LIMIT); >>>> +} >>>> + >>>> +static inline void lwtunnel_recursion_inc(void) >>>> +{ >>>> + __this_cpu_inc(softnet_data.xmit.recursion); >>>> +} >>>> + >>>> +static inline void lwtunnel_recursion_dec(void) >>>> +{ >>>> + __this_cpu_dec(softnet_data.xmit.recursion); >>>> +} >>>> +#else >>>> +static inline bool lwtunnel_recursion(void) >>>> +{ >>>> + return unlikely(current->net_xmit.recursion > >>>> LWTUNNEL_RECURSION_LIMIT); >>>> +} >>>> + >>>> +static inline void lwtunnel_recursion_inc(void) >>>> +{ >>>> + current->net_xmit.recursion++; >>>> +} >>>> + >>>> +static inline void lwtunnel_recursion_dec(void) >>>> +{ >>>> + current->net_xmit.recursion--; >>>> +} >>>> +#endif >>>> + >>>> +#endif /* _NET_CORE_LWTUNNEL_H */ >>> >>> Wondering what folks think about the above idea to reuse fields that >>> dev_xmit_recursion() currently uses. IMO, it seems OK considering the >>> use case and context. If not, I guess we'd need to add a new field to >>> both softnet_data and task_struct. >> >> Why not just reuse the dev_xmit_recursion*() helpers directly? >> >> -Toke >> > > It was my initial idea, but I'm not sure I can. Looks like they're not > exposed (it's a local .h in net/core/). Sorry, forget what I just said. We could indeed reuse it directly by including "dev.h" in lwtunnel.c.
diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c index 711cd3b4347a..0954783e36ce 100644 --- a/net/core/lwtunnel.c +++ b/net/core/lwtunnel.c @@ -23,6 +23,8 @@ #include <net/ip6_fib.h> #include <net/rtnh.h> +#include "lwtunnel.h" + DEFINE_STATIC_KEY_FALSE(nf_hooks_lwtunnel_enabled); EXPORT_SYMBOL_GPL(nf_hooks_lwtunnel_enabled); @@ -325,13 +327,23 @@ EXPORT_SYMBOL_GPL(lwtunnel_cmp_encap); int lwtunnel_output(struct net *net, struct sock *sk, struct sk_buff *skb) { - struct dst_entry *dst = skb_dst(skb); const struct lwtunnel_encap_ops *ops; struct lwtunnel_state *lwtstate; - int ret = -EINVAL; + struct dst_entry *dst; + int ret; + + if (lwtunnel_recursion()) { + net_crit_ratelimited("%s(): recursion limit reached on datapath\n", + __func__); + ret = -ENETDOWN; + goto drop; + } - if (!dst) + dst = skb_dst(skb); + if (!dst) { + ret = -EINVAL; goto drop; + } lwtstate = dst->lwtstate; if (lwtstate->type == LWTUNNEL_ENCAP_NONE || @@ -341,8 +353,11 @@ int lwtunnel_output(struct net *net, struct sock *sk, struct sk_buff *skb) ret = -EOPNOTSUPP; rcu_read_lock(); ops = rcu_dereference(lwtun_encaps[lwtstate->type]); - if (likely(ops && ops->output)) + if (likely(ops && ops->output)) { + lwtunnel_recursion_inc(); ret = ops->output(net, sk, skb); + lwtunnel_recursion_dec(); + } rcu_read_unlock(); if (ret == -EOPNOTSUPP) @@ -359,13 +374,23 @@ EXPORT_SYMBOL_GPL(lwtunnel_output); int lwtunnel_xmit(struct sk_buff *skb) { - struct dst_entry *dst = skb_dst(skb); const struct lwtunnel_encap_ops *ops; struct lwtunnel_state *lwtstate; - int ret = -EINVAL; + struct dst_entry *dst; + int ret; + + if (lwtunnel_recursion()) { + net_crit_ratelimited("%s(): recursion limit reached on datapath\n", + __func__); + ret = -ENETDOWN; + goto drop; + } - if (!dst) + dst = skb_dst(skb); + if (!dst) { + ret = -EINVAL; goto drop; + } lwtstate = dst->lwtstate; @@ -376,8 +401,11 @@ int lwtunnel_xmit(struct sk_buff *skb) ret = -EOPNOTSUPP; rcu_read_lock(); ops = rcu_dereference(lwtun_encaps[lwtstate->type]); - if (likely(ops && ops->xmit)) + if (likely(ops && ops->xmit)) { + lwtunnel_recursion_inc(); ret = ops->xmit(skb); + lwtunnel_recursion_dec(); + } rcu_read_unlock(); if (ret == -EOPNOTSUPP) @@ -394,13 +422,23 @@ EXPORT_SYMBOL_GPL(lwtunnel_xmit); int lwtunnel_input(struct sk_buff *skb) { - struct dst_entry *dst = skb_dst(skb); const struct lwtunnel_encap_ops *ops; struct lwtunnel_state *lwtstate; - int ret = -EINVAL; + struct dst_entry *dst; + int ret; - if (!dst) + if (lwtunnel_recursion()) { + net_crit_ratelimited("%s(): recursion limit reached on datapath\n", + __func__); + ret = -ENETDOWN; goto drop; + } + + dst = skb_dst(skb); + if (!dst) { + ret = -EINVAL; + goto drop; + } lwtstate = dst->lwtstate; if (lwtstate->type == LWTUNNEL_ENCAP_NONE || @@ -410,8 +448,11 @@ int lwtunnel_input(struct sk_buff *skb) ret = -EOPNOTSUPP; rcu_read_lock(); ops = rcu_dereference(lwtun_encaps[lwtstate->type]); - if (likely(ops && ops->input)) + if (likely(ops && ops->input)) { + lwtunnel_recursion_inc(); ret = ops->input(skb); + lwtunnel_recursion_dec(); + } rcu_read_unlock(); if (ret == -EOPNOTSUPP) diff --git a/net/core/lwtunnel.h b/net/core/lwtunnel.h new file mode 100644 index 000000000000..32880ecdd8bb --- /dev/null +++ b/net/core/lwtunnel.h @@ -0,0 +1,42 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +#ifndef _NET_CORE_LWTUNNEL_H +#define _NET_CORE_LWTUNNEL_H + +#include <linux/netdevice.h> + +#define LWTUNNEL_RECURSION_LIMIT 8 + +#ifndef CONFIG_PREEMPT_RT +static inline bool lwtunnel_recursion(void) +{ + return unlikely(__this_cpu_read(softnet_data.xmit.recursion) > + LWTUNNEL_RECURSION_LIMIT); +} + +static inline void lwtunnel_recursion_inc(void) +{ + __this_cpu_inc(softnet_data.xmit.recursion); +} + +static inline void lwtunnel_recursion_dec(void) +{ + __this_cpu_dec(softnet_data.xmit.recursion); +} +#else +static inline bool lwtunnel_recursion(void) +{ + return unlikely(current->net_xmit.recursion > LWTUNNEL_RECURSION_LIMIT); +} + +static inline void lwtunnel_recursion_inc(void) +{ + current->net_xmit.recursion++; +} + +static inline void lwtunnel_recursion_dec(void) +{ + current->net_xmit.recursion--; +} +#endif + +#endif /* _NET_CORE_LWTUNNEL_H */
Different kind of loops in most of lwtunnel users were fixed by some recent patches. This patch acts as a parachute, catch all solution, by detecting any use cases with recursion and taking care of them (e.g., a loop between routes). This is applied to lwtunnel_input(), lwtunnel_output(), and lwtunnel_xmit(). Fixes: ffce41962ef6 ("lwtunnel: support dst output redirect function") Fixes: 2536862311d2 ("lwt: Add support to redirect dst.input") Fixes: 14972cbd34ff ("net: lwtunnel: Handle fragmentation") Closes: https://lore.kernel.org/netdev/2bc9e2079e864a9290561894d2a602d6@akamai.com/ Cc: Roopa Prabhu <roopa@nvidia.com> Cc: Andrea Mayer <andrea.mayer@uniroma2.it> Cc: Stefano Salsano <stefano.salsano@uniroma2.it> Cc: Ahmed Abdelsalam <ahabdels.dev@gmail.com> Cc: Ido Schimmel <idosch@nvidia.com> Signed-off-by: Justin Iurman <justin.iurman@uliege.be> --- net/core/lwtunnel.c | 65 ++++++++++++++++++++++++++++++++++++--------- net/core/lwtunnel.h | 42 +++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 12 deletions(-) create mode 100644 net/core/lwtunnel.h