| Message ID | 20250211221624.18435-3-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 23:16, Justin Iurman wrote: > When the destination is the same post-transformation, we enter a > lwtunnel loop. This is true for ioam6_iptunnel, rpl_iptunnel, and > seg6_iptunnel, in both input() and output() handlers respectively, where > either dst_input() or dst_output() is called at the end. It happens for > instance with the ioam6 inline mode, but can also happen for any of them > as long as the post-transformation destination still matches the fib > entry. Note that ioam6_iptunnel was already comparing the old and new > destination address to prevent the loop, but it is not enough (e.g., > other addresses can still match the same subnet). > > Here is an example for rpl_input(): > > dump_stack_lvl+0x60/0x80 > rpl_input+0x9d/0x320 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > [...] > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > ip6_sublist_rcv_finish+0x85/0x90 > ip6_sublist_rcv+0x236/0x2f0 > > ... until rpl_do_srh() fails, which means skb_cow_head() failed. > > This patch prevents that kind of loop by redirecting to the origin > input() or output() when the destination is the same > post-transformation. > > Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation") > 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> > Cc: Alexander Aring <alex.aring@gmail.com> > Cc: David Lebrun <dlebrun@google.com> > --- > net/ipv6/ioam6_iptunnel.c | 6 ++---- > net/ipv6/rpl_iptunnel.c | 10 ++++++++++ > net/ipv6/seg6_iptunnel.c | 33 +++++++++++++++++++++++++++------ > 3 files changed, 39 insertions(+), 10 deletions(-) > > diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c > index 2c383c12a431..6c61b306f2e9 100644 > --- a/net/ipv6/ioam6_iptunnel.c > +++ b/net/ipv6/ioam6_iptunnel.c > @@ -337,7 +337,6 @@ static int ioam6_do_encap(struct net *net, struct sk_buff *skb, > static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb) > { > struct dst_entry *dst = skb_dst(skb), *cache_dst = NULL; > - struct in6_addr orig_daddr; > struct ioam6_lwt *ilwt; > int err = -EINVAL; > u32 pkt_cnt; > @@ -352,8 +351,6 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb) > if (pkt_cnt % ilwt->freq.n >= ilwt->freq.k) > goto out; > > - orig_daddr = ipv6_hdr(skb)->daddr; > - > local_bh_disable(); > cache_dst = dst_cache_get(&ilwt->cache); > local_bh_enable(); > @@ -422,7 +419,8 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb) > goto drop; > } > > - if (!ipv6_addr_equal(&orig_daddr, &ipv6_hdr(skb)->daddr)) { > + /* avoid a lwtunnel_input() loop when dst_entry is the same */ sigh... Should be lwtunnel_output() in the comment, let me know if I need to re-spin.
On Tue, Feb 11, 2025 at 11:16:23PM +0100, Justin Iurman wrote: > When the destination is the same post-transformation, we enter a > lwtunnel loop. This is true for ioam6_iptunnel, rpl_iptunnel, and > seg6_iptunnel, in both input() and output() handlers respectively, where > either dst_input() or dst_output() is called at the end. It happens for > instance with the ioam6 inline mode, but can also happen for any of them > as long as the post-transformation destination still matches the fib > entry. Note that ioam6_iptunnel was already comparing the old and new > destination address to prevent the loop, but it is not enough (e.g., > other addresses can still match the same subnet). > > Here is an example for rpl_input(): > > dump_stack_lvl+0x60/0x80 > rpl_input+0x9d/0x320 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > [...] > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > lwtunnel_input+0x64/0xa0 > ip6_sublist_rcv_finish+0x85/0x90 > ip6_sublist_rcv+0x236/0x2f0 > > ... until rpl_do_srh() fails, which means skb_cow_head() failed. > > This patch prevents that kind of loop by redirecting to the origin > input() or output() when the destination is the same > post-transformation. A loop was reported a few months ago with a similar stack trace: https://lore.kernel.org/netdev/2bc9e2079e864a9290561894d2a602d6@akamai.com/ But even with this series applied my VM gets stuck. Can you please check if the fix is incomplete?
On 2/13/25 14:28, Ido Schimmel wrote: > On Tue, Feb 11, 2025 at 11:16:23PM +0100, Justin Iurman wrote: >> When the destination is the same post-transformation, we enter a >> lwtunnel loop. This is true for ioam6_iptunnel, rpl_iptunnel, and >> seg6_iptunnel, in both input() and output() handlers respectively, where >> either dst_input() or dst_output() is called at the end. It happens for >> instance with the ioam6 inline mode, but can also happen for any of them >> as long as the post-transformation destination still matches the fib >> entry. Note that ioam6_iptunnel was already comparing the old and new >> destination address to prevent the loop, but it is not enough (e.g., >> other addresses can still match the same subnet). >> >> Here is an example for rpl_input(): >> >> dump_stack_lvl+0x60/0x80 >> rpl_input+0x9d/0x320 >> lwtunnel_input+0x64/0xa0 >> lwtunnel_input+0x64/0xa0 >> lwtunnel_input+0x64/0xa0 >> lwtunnel_input+0x64/0xa0 >> lwtunnel_input+0x64/0xa0 >> [...] >> lwtunnel_input+0x64/0xa0 >> lwtunnel_input+0x64/0xa0 >> lwtunnel_input+0x64/0xa0 >> lwtunnel_input+0x64/0xa0 >> lwtunnel_input+0x64/0xa0 >> ip6_sublist_rcv_finish+0x85/0x90 >> ip6_sublist_rcv+0x236/0x2f0 >> >> ... until rpl_do_srh() fails, which means skb_cow_head() failed. >> >> This patch prevents that kind of loop by redirecting to the origin >> input() or output() when the destination is the same >> post-transformation. > > A loop was reported a few months ago with a similar stack trace: > https://lore.kernel.org/netdev/2bc9e2079e864a9290561894d2a602d6@akamai.com/ > > But even with this series applied my VM gets stuck. Can you please check > if the fix is incomplete? Good catch! Indeed, seg6_local also needs to be fixed the same way. Back to my first idea: maybe we could directly fix it in lwtunnel_input() and lwtunnel_output() to make our lives easier, but we'd have to be careful to modify all users accordingly. The users I'm 100% sure that are concerned: ioam6 (output), rpl (input/output), seg6 (input/output), seg6_local (input). Other users I'm not totally sure (to be checked): ila (output), bpf (input). Otherwise, we'll need to apply the fix to each user concerned (probably the safest (best?) option right now). Any opinions?
On Thu, Feb 13, 2025 at 11:51:49PM +0100, Justin Iurman wrote: > On 2/13/25 14:28, Ido Schimmel wrote: > > On Tue, Feb 11, 2025 at 11:16:23PM +0100, Justin Iurman wrote: > > > When the destination is the same post-transformation, we enter a > > > lwtunnel loop. This is true for ioam6_iptunnel, rpl_iptunnel, and > > > seg6_iptunnel, in both input() and output() handlers respectively, where > > > either dst_input() or dst_output() is called at the end. It happens for > > > instance with the ioam6 inline mode, but can also happen for any of them > > > as long as the post-transformation destination still matches the fib > > > entry. Note that ioam6_iptunnel was already comparing the old and new > > > destination address to prevent the loop, but it is not enough (e.g., > > > other addresses can still match the same subnet). > > > > > > Here is an example for rpl_input(): > > > > > > dump_stack_lvl+0x60/0x80 > > > rpl_input+0x9d/0x320 > > > lwtunnel_input+0x64/0xa0 > > > lwtunnel_input+0x64/0xa0 > > > lwtunnel_input+0x64/0xa0 > > > lwtunnel_input+0x64/0xa0 > > > lwtunnel_input+0x64/0xa0 > > > [...] > > > lwtunnel_input+0x64/0xa0 > > > lwtunnel_input+0x64/0xa0 > > > lwtunnel_input+0x64/0xa0 > > > lwtunnel_input+0x64/0xa0 > > > lwtunnel_input+0x64/0xa0 > > > ip6_sublist_rcv_finish+0x85/0x90 > > > ip6_sublist_rcv+0x236/0x2f0 > > > > > > ... until rpl_do_srh() fails, which means skb_cow_head() failed. > > > > > > This patch prevents that kind of loop by redirecting to the origin > > > input() or output() when the destination is the same > > > post-transformation. > > > > A loop was reported a few months ago with a similar stack trace: > > https://lore.kernel.org/netdev/2bc9e2079e864a9290561894d2a602d6@akamai.com/ > > > > But even with this series applied my VM gets stuck. Can you please check > > if the fix is incomplete? > > Good catch! Indeed, seg6_local also needs to be fixed the same way. > > Back to my first idea: maybe we could directly fix it in lwtunnel_input() > and lwtunnel_output() to make our lives easier, but we'd have to be careful > to modify all users accordingly. The users I'm 100% sure that are concerned: > ioam6 (output), rpl (input/output), seg6 (input/output), seg6_local (input). > Other users I'm not totally sure (to be checked): ila (output), bpf (input). > > Otherwise, we'll need to apply the fix to each user concerned (probably the > safest (best?) option right now). Any opinions? I audited the various lwt users and I agree with your analysis about which users seem to be effected by this issue. I'm not entirely sure how you want to fix this in lwtunnel_{input,output}() given that only the input()/output() handlers of the individual lwt users are aware of both the old and new dst entries. BTW, I noticed that bpf implements the xmit() hook in addition to input()/output(). I wonder if a loop is possible in the following case: ip_finish_output2() <----+ lwtunnel_xmit() | bpf_xmit() | // bpf program does not change | // the packet and returns | // BPF_LWT_REROUTE | bpf_lwt_xmit_reroute() | // unmodified packet resolves | // the same dst entry | dst_output() | ip_output() -------------+
On Sun, Feb 16, 2025 at 06:31:06PM +0200, Ido Schimmel wrote: > On Thu, Feb 13, 2025 at 11:51:49PM +0100, Justin Iurman wrote: > > On 2/13/25 14:28, Ido Schimmel wrote: > > > On Tue, Feb 11, 2025 at 11:16:23PM +0100, Justin Iurman wrote: > > > > When the destination is the same post-transformation, we enter a > > > > lwtunnel loop. This is true for ioam6_iptunnel, rpl_iptunnel, and > > > > seg6_iptunnel, in both input() and output() handlers respectively, where > > > > either dst_input() or dst_output() is called at the end. It happens for > > > > instance with the ioam6 inline mode, but can also happen for any of them > > > > as long as the post-transformation destination still matches the fib > > > > entry. Note that ioam6_iptunnel was already comparing the old and new > > > > destination address to prevent the loop, but it is not enough (e.g., > > > > other addresses can still match the same subnet). > > > > > > > > Here is an example for rpl_input(): > > > > > > > > dump_stack_lvl+0x60/0x80 > > > > rpl_input+0x9d/0x320 > > > > lwtunnel_input+0x64/0xa0 > > > > lwtunnel_input+0x64/0xa0 > > > > lwtunnel_input+0x64/0xa0 > > > > lwtunnel_input+0x64/0xa0 > > > > lwtunnel_input+0x64/0xa0 > > > > [...] > > > > lwtunnel_input+0x64/0xa0 > > > > lwtunnel_input+0x64/0xa0 > > > > lwtunnel_input+0x64/0xa0 > > > > lwtunnel_input+0x64/0xa0 > > > > lwtunnel_input+0x64/0xa0 > > > > ip6_sublist_rcv_finish+0x85/0x90 > > > > ip6_sublist_rcv+0x236/0x2f0 > > > > > > > > ... until rpl_do_srh() fails, which means skb_cow_head() failed. > > > > > > > > This patch prevents that kind of loop by redirecting to the origin > > > > input() or output() when the destination is the same > > > > post-transformation. > > > > > > A loop was reported a few months ago with a similar stack trace: > > > https://lore.kernel.org/netdev/2bc9e2079e864a9290561894d2a602d6@akamai.com/ > > > > > > But even with this series applied my VM gets stuck. Can you please check > > > if the fix is incomplete? > > > > Good catch! Indeed, seg6_local also needs to be fixed the same way. > > > > Back to my first idea: maybe we could directly fix it in lwtunnel_input() > > and lwtunnel_output() to make our lives easier, but we'd have to be careful > > to modify all users accordingly. The users I'm 100% sure that are concerned: > > ioam6 (output), rpl (input/output), seg6 (input/output), seg6_local (input). > > Other users I'm not totally sure (to be checked): ila (output), bpf (input). > > > > Otherwise, we'll need to apply the fix to each user concerned (probably the > > safest (best?) option right now). Any opinions? > > I audited the various lwt users and I agree with your analysis about > which users seem to be effected by this issue. > > I'm not entirely sure how you want to fix this in > lwtunnel_{input,output}() given that only the input()/output() handlers > of the individual lwt users are aware of both the old and new dst > entries. > > BTW, I noticed that bpf implements the xmit() hook in addition to > input()/output(). I wonder if a loop is possible in the following case: > > ip_finish_output2() <----+ > lwtunnel_xmit() | > bpf_xmit() | > // bpf program does not change | > // the packet and returns | > // BPF_LWT_REROUTE | > bpf_lwt_xmit_reroute() | > // unmodified packet resolves | > // the same dst entry | > dst_output() | > ip_output() -------------+ FWIW, verified that this is indeed the case. Reproducer: $ cat lwt_xmit_repo.bpf.c // SPDX-License-Identifier: GPL-2.0 #include <linux/bpf.h> #include <bpf/bpf_helpers.h> SEC("lwt_xmit") int repo(struct __sk_buff *skb) { return BPF_LWT_REROUTE; } $ clang -O2 -target bpf -c lwt_xmit_repo.bpf.c -o lwt_xmit_repo.o # ip link add name dummy1 up type dummy # ip route add 192.0.2.0/24 nexthop encap bpf xmit obj ./lwt_xmit_repo.o sec lwt_xmit dev dummy1 # ping 192.0.2.1
On 2/16/25 17:31, Ido Schimmel wrote: > On Thu, Feb 13, 2025 at 11:51:49PM +0100, Justin Iurman wrote: >> On 2/13/25 14:28, Ido Schimmel wrote: >>> On Tue, Feb 11, 2025 at 11:16:23PM +0100, Justin Iurman wrote: >>>> When the destination is the same post-transformation, we enter a >>>> lwtunnel loop. This is true for ioam6_iptunnel, rpl_iptunnel, and >>>> seg6_iptunnel, in both input() and output() handlers respectively, where >>>> either dst_input() or dst_output() is called at the end. It happens for >>>> instance with the ioam6 inline mode, but can also happen for any of them >>>> as long as the post-transformation destination still matches the fib >>>> entry. Note that ioam6_iptunnel was already comparing the old and new >>>> destination address to prevent the loop, but it is not enough (e.g., >>>> other addresses can still match the same subnet). >>>> >>>> Here is an example for rpl_input(): >>>> >>>> dump_stack_lvl+0x60/0x80 >>>> rpl_input+0x9d/0x320 >>>> lwtunnel_input+0x64/0xa0 >>>> lwtunnel_input+0x64/0xa0 >>>> lwtunnel_input+0x64/0xa0 >>>> lwtunnel_input+0x64/0xa0 >>>> lwtunnel_input+0x64/0xa0 >>>> [...] >>>> lwtunnel_input+0x64/0xa0 >>>> lwtunnel_input+0x64/0xa0 >>>> lwtunnel_input+0x64/0xa0 >>>> lwtunnel_input+0x64/0xa0 >>>> lwtunnel_input+0x64/0xa0 >>>> ip6_sublist_rcv_finish+0x85/0x90 >>>> ip6_sublist_rcv+0x236/0x2f0 >>>> >>>> ... until rpl_do_srh() fails, which means skb_cow_head() failed. >>>> >>>> This patch prevents that kind of loop by redirecting to the origin >>>> input() or output() when the destination is the same >>>> post-transformation. >>> >>> A loop was reported a few months ago with a similar stack trace: >>> https://lore.kernel.org/netdev/2bc9e2079e864a9290561894d2a602d6@akamai.com/ >>> >>> But even with this series applied my VM gets stuck. Can you please check >>> if the fix is incomplete? >> >> Good catch! Indeed, seg6_local also needs to be fixed the same way. >> >> Back to my first idea: maybe we could directly fix it in lwtunnel_input() >> and lwtunnel_output() to make our lives easier, but we'd have to be careful >> to modify all users accordingly. The users I'm 100% sure that are concerned: >> ioam6 (output), rpl (input/output), seg6 (input/output), seg6_local (input). >> Other users I'm not totally sure (to be checked): ila (output), bpf (input). >> >> Otherwise, we'll need to apply the fix to each user concerned (probably the >> safest (best?) option right now). Any opinions? > > I audited the various lwt users and I agree with your analysis about > which users seem to be effected by this issue. > > I'm not entirely sure how you want to fix this in > lwtunnel_{input,output}() given that only the input()/output() handlers > of the individual lwt users are aware of both the old and new dst > entries. Right. The idea was to compare "orig_dst" with "new dst" before/after a call to input()/output() in lwtunnel_input()/lwtunnel_output(). Which, of course, would require to modify each of those input/output handlers respectively, so that they don't call dst_input()/dst_output() nor orig_input()/orig_output() anymore. Would be easier to apply the fix at that level, instead of each one by one.
On 2/17/25 15:40, Ido Schimmel wrote: > On Sun, Feb 16, 2025 at 06:31:06PM +0200, Ido Schimmel wrote: >> On Thu, Feb 13, 2025 at 11:51:49PM +0100, Justin Iurman wrote: >>> On 2/13/25 14:28, Ido Schimmel wrote: >>>> On Tue, Feb 11, 2025 at 11:16:23PM +0100, Justin Iurman wrote: >>>>> When the destination is the same post-transformation, we enter a >>>>> lwtunnel loop. This is true for ioam6_iptunnel, rpl_iptunnel, and >>>>> seg6_iptunnel, in both input() and output() handlers respectively, where >>>>> either dst_input() or dst_output() is called at the end. It happens for >>>>> instance with the ioam6 inline mode, but can also happen for any of them >>>>> as long as the post-transformation destination still matches the fib >>>>> entry. Note that ioam6_iptunnel was already comparing the old and new >>>>> destination address to prevent the loop, but it is not enough (e.g., >>>>> other addresses can still match the same subnet). >>>>> >>>>> Here is an example for rpl_input(): >>>>> >>>>> dump_stack_lvl+0x60/0x80 >>>>> rpl_input+0x9d/0x320 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> [...] >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> ip6_sublist_rcv_finish+0x85/0x90 >>>>> ip6_sublist_rcv+0x236/0x2f0 >>>>> >>>>> ... until rpl_do_srh() fails, which means skb_cow_head() failed. >>>>> >>>>> This patch prevents that kind of loop by redirecting to the origin >>>>> input() or output() when the destination is the same >>>>> post-transformation. >>>> >>>> A loop was reported a few months ago with a similar stack trace: >>>> https://lore.kernel.org/netdev/2bc9e2079e864a9290561894d2a602d6@akamai.com/ >>>> >>>> But even with this series applied my VM gets stuck. Can you please check >>>> if the fix is incomplete? >>> >>> Good catch! Indeed, seg6_local also needs to be fixed the same way. >>> >>> Back to my first idea: maybe we could directly fix it in lwtunnel_input() >>> and lwtunnel_output() to make our lives easier, but we'd have to be careful >>> to modify all users accordingly. The users I'm 100% sure that are concerned: >>> ioam6 (output), rpl (input/output), seg6 (input/output), seg6_local (input). >>> Other users I'm not totally sure (to be checked): ila (output), bpf (input). >>> >>> Otherwise, we'll need to apply the fix to each user concerned (probably the >>> safest (best?) option right now). Any opinions? >> >> I audited the various lwt users and I agree with your analysis about >> which users seem to be effected by this issue. >> >> I'm not entirely sure how you want to fix this in >> lwtunnel_{input,output}() given that only the input()/output() handlers >> of the individual lwt users are aware of both the old and new dst >> entries. >> >> BTW, I noticed that bpf implements the xmit() hook in addition to >> input()/output(). I wonder if a loop is possible in the following case: >> >> ip_finish_output2() <----+ >> lwtunnel_xmit() | >> bpf_xmit() | >> // bpf program does not change | >> // the packet and returns | >> // BPF_LWT_REROUTE | >> bpf_lwt_xmit_reroute() | >> // unmodified packet resolves | >> // the same dst entry | >> dst_output() | >> ip_output() -------------+ > > FWIW, verified that this is indeed the case. Reproducer: > > $ cat lwt_xmit_repo.bpf.c > // SPDX-License-Identifier: GPL-2.0 > #include <linux/bpf.h> > #include <bpf/bpf_helpers.h> > > SEC("lwt_xmit") > int repo(struct __sk_buff *skb) > { > return BPF_LWT_REROUTE; > } > $ clang -O2 -target bpf -c lwt_xmit_repo.bpf.c -o lwt_xmit_repo.o > # ip link add name dummy1 up type dummy > # ip route add 192.0.2.0/24 nexthop encap bpf xmit obj ./lwt_xmit_repo.o sec lwt_xmit dev dummy1 > # ping 192.0.2.1 Thanks, Ido, appreciate. I'll post a new series based on this (#2) patch to take all users into account. Note that the new logic I described previously to solve the issue could be applied to lwtunnel_xmit() too. I wonder what others might think about it.
On 2/17/25 15:40, Ido Schimmel wrote: > On Sun, Feb 16, 2025 at 06:31:06PM +0200, Ido Schimmel wrote: >> On Thu, Feb 13, 2025 at 11:51:49PM +0100, Justin Iurman wrote: >>> On 2/13/25 14:28, Ido Schimmel wrote: >>>> On Tue, Feb 11, 2025 at 11:16:23PM +0100, Justin Iurman wrote: >>>>> When the destination is the same post-transformation, we enter a >>>>> lwtunnel loop. This is true for ioam6_iptunnel, rpl_iptunnel, and >>>>> seg6_iptunnel, in both input() and output() handlers respectively, where >>>>> either dst_input() or dst_output() is called at the end. It happens for >>>>> instance with the ioam6 inline mode, but can also happen for any of them >>>>> as long as the post-transformation destination still matches the fib >>>>> entry. Note that ioam6_iptunnel was already comparing the old and new >>>>> destination address to prevent the loop, but it is not enough (e.g., >>>>> other addresses can still match the same subnet). >>>>> >>>>> Here is an example for rpl_input(): >>>>> >>>>> dump_stack_lvl+0x60/0x80 >>>>> rpl_input+0x9d/0x320 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> [...] >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> lwtunnel_input+0x64/0xa0 >>>>> ip6_sublist_rcv_finish+0x85/0x90 >>>>> ip6_sublist_rcv+0x236/0x2f0 >>>>> >>>>> ... until rpl_do_srh() fails, which means skb_cow_head() failed. >>>>> >>>>> This patch prevents that kind of loop by redirecting to the origin >>>>> input() or output() when the destination is the same >>>>> post-transformation. >>>> >>>> A loop was reported a few months ago with a similar stack trace: >>>> https://lore.kernel.org/netdev/2bc9e2079e864a9290561894d2a602d6@akamai.com/ Ido, That loop is another beast which is out of scope of the series I'm about to send. Indeed, what I'm doing right now is to prevent reentry loops within lwtunnel_{input|output}(). Which, by the way, is also applied to seg6_local no matter what. The reported loop above is an infinite ping pong game between two fib rules (vs an infinite loop within the same fib rule -- what I'm fixing). If we want to fix that issue as well, we may reuse something like dev_xmit_recursion() in lwtunnel_{input|output|xmit}() to catch these buggy cases. Thoughts? >> [...] >> >> BTW, I noticed that bpf implements the xmit() hook in addition to >> input()/output(). I wonder if a loop is possible in the following case: >> >> ip_finish_output2() <----+ >> lwtunnel_xmit() | >> bpf_xmit() | >> // bpf program does not change | >> // the packet and returns | >> // BPF_LWT_REROUTE | >> bpf_lwt_xmit_reroute() | >> // unmodified packet resolves | >> // the same dst entry | >> dst_output() | >> ip_output() -------------+ > > FWIW, verified that this is indeed the case. Reproducer: > > $ cat lwt_xmit_repo.bpf.c > // SPDX-License-Identifier: GPL-2.0 > #include <linux/bpf.h> > #include <bpf/bpf_helpers.h> > > SEC("lwt_xmit") > int repo(struct __sk_buff *skb) > { > return BPF_LWT_REROUTE; > } > $ clang -O2 -target bpf -c lwt_xmit_repo.bpf.c -o lwt_xmit_repo.o > # ip link add name dummy1 up type dummy > # ip route add 192.0.2.0/24 nexthop encap bpf xmit obj ./lwt_xmit_repo.o sec lwt_xmit dev dummy1 > # ping 192.0.2.1 This one's also something special because it's neither input nor output, it's xmit. In that case, we cannot apply the same fix as for the others (ioam6, rpl, seg6, ila). Here, what I suggest is simply to disallow BPF_LWT_REROUTE when the dst_entry remains unchanged (which is, IMO, a buggy case), as follows: diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c index ae74634310a3..ee3546d78903 100644 --- a/net/core/lwt_bpf.c +++ b/net/core/lwt_bpf.c @@ -180,6 +180,7 @@ static int bpf_lwt_xmit_reroute(struct sk_buff *skb) struct net_device *l3mdev = l3mdev_master_dev_rcu(skb_dst(skb)->dev); int oif = l3mdev ? l3mdev->ifindex : 0; struct dst_entry *dst = NULL; + struct dst_entry *orig_dst; int err = -EAFNOSUPPORT; struct sock *sk; struct net *net; @@ -201,6 +202,8 @@ static int bpf_lwt_xmit_reroute(struct sk_buff *skb) net = dev_net(skb_dst(skb)->dev); } + orig_dst = skb_dst(skb); + if (ipv4) { struct iphdr *iph = ip_hdr(skb); struct flowi4 fl4 = {}; @@ -254,6 +257,16 @@ static int bpf_lwt_xmit_reroute(struct sk_buff *skb) if (unlikely(err)) goto err; + /* avoid lwtunnel_xmit() reentry loop when destination is the same + * after transformation (i.e., disallow BPF_LWT_REROUTE when dst_entry + * remains the same). + */ + if (orig_dst->lwtstate == dst->lwtstate) { + dst_release(dst); + err = -EINVAL; + goto err; + } + skb_dst_drop(skb); skb_dst_set(skb, dst);
diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c index 2c383c12a431..6c61b306f2e9 100644 --- a/net/ipv6/ioam6_iptunnel.c +++ b/net/ipv6/ioam6_iptunnel.c @@ -337,7 +337,6 @@ static int ioam6_do_encap(struct net *net, struct sk_buff *skb, static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb) { struct dst_entry *dst = skb_dst(skb), *cache_dst = NULL; - struct in6_addr orig_daddr; struct ioam6_lwt *ilwt; int err = -EINVAL; u32 pkt_cnt; @@ -352,8 +351,6 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb) if (pkt_cnt % ilwt->freq.n >= ilwt->freq.k) goto out; - orig_daddr = ipv6_hdr(skb)->daddr; - local_bh_disable(); cache_dst = dst_cache_get(&ilwt->cache); local_bh_enable(); @@ -422,7 +419,8 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb) goto drop; } - if (!ipv6_addr_equal(&orig_daddr, &ipv6_hdr(skb)->daddr)) { + /* avoid a lwtunnel_input() loop when dst_entry is the same */ + if (dst->lwtstate != cache_dst->lwtstate) { skb_dst_drop(skb); skb_dst_set(skb, cache_dst); return dst_output(net, sk, skb); diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c index c26bf284459f..dc004e9aa649 100644 --- a/net/ipv6/rpl_iptunnel.c +++ b/net/ipv6/rpl_iptunnel.c @@ -247,6 +247,12 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb) goto drop; } + /* avoid a lwtunnel_output() loop when dst_entry is the same */ + if (orig_dst->lwtstate == dst->lwtstate) { + dst_release(dst); + return orig_dst->lwtstate->orig_output(net, sk, skb); + } + skb_dst_drop(skb); skb_dst_set(skb, dst); @@ -305,6 +311,10 @@ 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) + return dst->lwtstate->orig_input(skb); + return dst_input(skb); drop: diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c index 6045e850b4bf..5ce662d8f334 100644 --- a/net/ipv6/seg6_iptunnel.c +++ b/net/ipv6/seg6_iptunnel.c @@ -467,9 +467,16 @@ static int seg6_input_finish(struct net *net, struct sock *sk, return dst_input(skb); } +static int seg6_input_redirect_finish(struct net *net, struct sock *sk, + struct sk_buff *skb) +{ + return skb_dst(skb)->lwtstate->orig_input(skb); +} + static int seg6_input_core(struct net *net, struct sock *sk, struct sk_buff *skb) { + int (*in_func)(struct net *net, struct sock *sk, struct sk_buff *skb); struct dst_entry *orig_dst = skb_dst(skb); struct dst_entry *dst = NULL; struct lwtunnel_state *lwtst; @@ -515,12 +522,18 @@ 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) + in_func = seg6_input_redirect_finish; + else + in_func = seg6_input_finish; + if (static_branch_unlikely(&nf_hooks_lwtunnel_enabled)) return NF_HOOK(NFPROTO_IPV6, NF_INET_LOCAL_OUT, dev_net(skb->dev), NULL, skb, NULL, - skb_dst(skb)->dev, seg6_input_finish); + skb_dst(skb)->dev, in_func); - return seg6_input_finish(dev_net(skb->dev), NULL, skb); + return in_func(dev_net(skb->dev), NULL, skb); drop: kfree_skb(skb); return err; @@ -554,6 +567,7 @@ static int seg6_input(struct sk_buff *skb) static int seg6_output_core(struct net *net, struct sock *sk, struct sk_buff *skb) { + int (*out_func)(struct net *net, struct sock *sk, struct sk_buff *skb); struct dst_entry *orig_dst = skb_dst(skb); struct dst_entry *dst = NULL; struct seg6_lwt *slwt; @@ -598,14 +612,21 @@ static int seg6_output_core(struct net *net, struct sock *sk, goto drop; } - skb_dst_drop(skb); - skb_dst_set(skb, dst); + /* avoid a lwtunnel_output() loop when dst_entry is the same */ + if (orig_dst->lwtstate == dst->lwtstate) { + dst_release(dst); + out_func = orig_dst->lwtstate->orig_output; + } else { + skb_dst_drop(skb); + skb_dst_set(skb, dst); + out_func = dst_output; + } if (static_branch_unlikely(&nf_hooks_lwtunnel_enabled)) return NF_HOOK(NFPROTO_IPV6, NF_INET_LOCAL_OUT, net, sk, skb, - NULL, skb_dst(skb)->dev, dst_output); + NULL, skb_dst(skb)->dev, out_func); - return dst_output(net, sk, skb); + return out_func(net, sk, skb); drop: dst_release(dst); kfree_skb(skb);
When the destination is the same post-transformation, we enter a lwtunnel loop. This is true for ioam6_iptunnel, rpl_iptunnel, and seg6_iptunnel, in both input() and output() handlers respectively, where either dst_input() or dst_output() is called at the end. It happens for instance with the ioam6 inline mode, but can also happen for any of them as long as the post-transformation destination still matches the fib entry. Note that ioam6_iptunnel was already comparing the old and new destination address to prevent the loop, but it is not enough (e.g., other addresses can still match the same subnet). Here is an example for rpl_input(): dump_stack_lvl+0x60/0x80 rpl_input+0x9d/0x320 lwtunnel_input+0x64/0xa0 lwtunnel_input+0x64/0xa0 lwtunnel_input+0x64/0xa0 lwtunnel_input+0x64/0xa0 lwtunnel_input+0x64/0xa0 [...] lwtunnel_input+0x64/0xa0 lwtunnel_input+0x64/0xa0 lwtunnel_input+0x64/0xa0 lwtunnel_input+0x64/0xa0 lwtunnel_input+0x64/0xa0 ip6_sublist_rcv_finish+0x85/0x90 ip6_sublist_rcv+0x236/0x2f0 ... until rpl_do_srh() fails, which means skb_cow_head() failed. This patch prevents that kind of loop by redirecting to the origin input() or output() when the destination is the same post-transformation. Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation") 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> Cc: Alexander Aring <alex.aring@gmail.com> Cc: David Lebrun <dlebrun@google.com> --- net/ipv6/ioam6_iptunnel.c | 6 ++---- net/ipv6/rpl_iptunnel.c | 10 ++++++++++ net/ipv6/seg6_iptunnel.c | 33 +++++++++++++++++++++++++++------ 3 files changed, 39 insertions(+), 10 deletions(-)