| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fb9aec0e-0d95-4ca3-8174-32174551ece3@uliege.be>
Date: Wed, 12 Mar 2025 11:40:06 +0100
From: Justin Iurman <justin.iurman@...ege.be>
To: netdev@...r.kernel.org
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, horms@...nel.org, Roopa Prabhu <roopa@...dia.com>,
Andrea Mayer <andrea.mayer@...roma2.it>,
Stefano Salsano <stefano.salsano@...roma2.it>,
Ahmed Abdelsalam <ahabdels.dev@...il.com>, Ido Schimmel <idosch@...dia.com>
Subject: Re: [PATCH net] net: lwtunnel: fix recursion loops
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@...dia.com>
> Cc: Andrea Mayer <andrea.mayer@...roma2.it>
> Cc: Stefano Salsano <stefano.salsano@...roma2.it>
> Cc: Ahmed Abdelsalam <ahabdels.dev@...il.com>
> Cc: Ido Schimmel <idosch@...dia.com>
> Signed-off-by: Justin Iurman <justin.iurman@...ege.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).
Powered by blists - more mailing lists