| Message ID | 20250211221624.18435-4-justin.iurman@uliege.be (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | several fixes for ioam6, rpl and seg6 lwtunnels | expand |
On 2/11/25 11:16 PM, Justin Iurman wrote: > Some lwtunnel users implement both lwt input and output handlers. If the > post-transformation destination on input is the same, the output handler > is also called and the same transformation is applied (again). Here are > the users: ila, bpf, rpl, seg6. The first one (ila) does not need this > fix, since it already implements a check to avoid such a duplicate. The > second (bpf) may need this fix, but I'm not familiar with that code path > and will keep it out of this patch. The two others (rpl and seg6) do > need this patch. > > Due to the ila implementation (as an example), we cannot fix the issue > in lwtunnel_input() and lwtunnel_output() directly. Instead, we need to > do it on a case-by-case basis. This patch fixes both rpl_iptunnel and > seg6_iptunnel users. The fix re-uses skb->redirected in input handlers > to notify corresponding output handlers that the transformation was > already applied and to skip it. The "redirected" field seems safe to be > used here. > > Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel") > Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and injection with lwtunnels") > Signed-off-by: Justin Iurman <justin.iurman@uliege.be> > --- > net/ipv6/rpl_iptunnel.c | 14 ++++++++++++-- > net/ipv6/seg6_iptunnel.c | 16 +++++++++++++--- > 2 files changed, 25 insertions(+), 5 deletions(-) > > diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c > index dc004e9aa649..2dc1f2297e39 100644 > --- a/net/ipv6/rpl_iptunnel.c > +++ b/net/ipv6/rpl_iptunnel.c > @@ -208,6 +208,12 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb) > struct rpl_lwt *rlwt; > int err; > > + /* Don't re-apply the transformation when rpl_input() already did it */ > + if (skb_is_redirected(skb)) { This check looks false-positive prone, i.e. if packet lands on an LWT tunnel due to an tc redirect from another non lwt device. On the flip side I don't see any good method to propagate the relevant information. A skb ext would work, but I would not call that a good method. /P
On 2/13/25 15:33, Paolo Abeni wrote: > On 2/11/25 11:16 PM, Justin Iurman wrote: >> Some lwtunnel users implement both lwt input and output handlers. If the >> post-transformation destination on input is the same, the output handler >> is also called and the same transformation is applied (again). Here are >> the users: ila, bpf, rpl, seg6. The first one (ila) does not need this >> fix, since it already implements a check to avoid such a duplicate. The >> second (bpf) may need this fix, but I'm not familiar with that code path >> and will keep it out of this patch. The two others (rpl and seg6) do >> need this patch. >> >> Due to the ila implementation (as an example), we cannot fix the issue >> in lwtunnel_input() and lwtunnel_output() directly. Instead, we need to >> do it on a case-by-case basis. This patch fixes both rpl_iptunnel and >> seg6_iptunnel users. The fix re-uses skb->redirected in input handlers >> to notify corresponding output handlers that the transformation was >> already applied and to skip it. The "redirected" field seems safe to be >> used here. >> >> Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel") >> Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and injection with lwtunnels") >> Signed-off-by: Justin Iurman <justin.iurman@uliege.be> >> --- >> net/ipv6/rpl_iptunnel.c | 14 ++++++++++++-- >> net/ipv6/seg6_iptunnel.c | 16 +++++++++++++--- >> 2 files changed, 25 insertions(+), 5 deletions(-) >> >> diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c >> index dc004e9aa649..2dc1f2297e39 100644 >> --- a/net/ipv6/rpl_iptunnel.c >> +++ b/net/ipv6/rpl_iptunnel.c >> @@ -208,6 +208,12 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb) >> struct rpl_lwt *rlwt; >> int err; >> >> + /* Don't re-apply the transformation when rpl_input() already did it */ >> + if (skb_is_redirected(skb)) { > > This check looks false-positive prone, i.e. if packet lands on an LWT > tunnel due to an tc redirect from another non lwt device. True, it was indeed a trade-off solution :-/ > On the flip side I don't see any good method to propagate the relevant > information. A skb ext would work, but I would not call that a good method. Agree :-( Did not check but maybe we could also look at skb->tc_at_ingress in that case? Not sure it'd help though. Or... any chance we could find a hole in sk_buff for a new :1 field?
diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c index dc004e9aa649..2dc1f2297e39 100644 --- a/net/ipv6/rpl_iptunnel.c +++ b/net/ipv6/rpl_iptunnel.c @@ -208,6 +208,12 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb) struct rpl_lwt *rlwt; int err; + /* Don't re-apply the transformation when rpl_input() already did it */ + if (skb_is_redirected(skb)) { + skb_reset_redirect(skb); + return orig_dst->lwtstate->orig_output(net, sk, skb); + } + rlwt = rpl_lwt_lwtunnel(orig_dst->lwtstate); local_bh_disable(); @@ -311,9 +317,13 @@ static int rpl_input(struct sk_buff *skb) skb_dst_set(skb, dst); } - /* avoid a lwtunnel_input() loop when dst_entry is the same */ - if (lwtst == dst->lwtstate) + /* avoid a lwtunnel_input() loop when dst_entry is the same, and make + * sure rpl_output() does not apply the transformation one more time + */ + if (lwtst == dst->lwtstate) { + skb_set_redirected_noclear(skb, true); return dst->lwtstate->orig_input(skb); + } return dst_input(skb); diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c index 5ce662d8f334..69233c2ed658 100644 --- a/net/ipv6/seg6_iptunnel.c +++ b/net/ipv6/seg6_iptunnel.c @@ -522,11 +522,15 @@ static int seg6_input_core(struct net *net, struct sock *sk, skb_dst_set(skb, dst); } - /* avoid a lwtunnel_input() loop when dst_entry is the same */ - if (lwtst == dst->lwtstate) + /* avoid a lwtunnel_input() loop when dst_entry is the same, and make + * sure seg6_output() does not apply the transformation one more time + */ + if (lwtst == dst->lwtstate) { + skb_set_redirected_noclear(skb, true); in_func = seg6_input_redirect_finish; - else + } else { in_func = seg6_input_finish; + } if (static_branch_unlikely(&nf_hooks_lwtunnel_enabled)) return NF_HOOK(NFPROTO_IPV6, NF_INET_LOCAL_OUT, @@ -573,6 +577,12 @@ static int seg6_output_core(struct net *net, struct sock *sk, struct seg6_lwt *slwt; int err; + /* Don't re-apply the transformation when seg6_input() already did it */ + if (skb_is_redirected(skb)) { + skb_reset_redirect(skb); + return orig_dst->lwtstate->orig_output(net, sk, skb); + } + slwt = seg6_lwt_lwtunnel(orig_dst->lwtstate); local_bh_disable();
Some lwtunnel users implement both lwt input and output handlers. If the post-transformation destination on input is the same, the output handler is also called and the same transformation is applied (again). Here are the users: ila, bpf, rpl, seg6. The first one (ila) does not need this fix, since it already implements a check to avoid such a duplicate. The second (bpf) may need this fix, but I'm not familiar with that code path and will keep it out of this patch. The two others (rpl and seg6) do need this patch. Due to the ila implementation (as an example), we cannot fix the issue in lwtunnel_input() and lwtunnel_output() directly. Instead, we need to do it on a case-by-case basis. This patch fixes both rpl_iptunnel and seg6_iptunnel users. The fix re-uses skb->redirected in input handlers to notify corresponding output handlers that the transformation was already applied and to skip it. The "redirected" field seems safe to be used here. Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel") Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and injection with lwtunnels") Signed-off-by: Justin Iurman <justin.iurman@uliege.be> --- net/ipv6/rpl_iptunnel.c | 14 ++++++++++++-- net/ipv6/seg6_iptunnel.c | 16 +++++++++++++--- 2 files changed, 25 insertions(+), 5 deletions(-)