| Message ID | 20250211221624.18435-2-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 Tue, Feb 11, 2025 at 11:16:22PM +0100, Justin Iurman wrote: > As a follow up to commit 92191dd10730 ("net: ipv6: fix dst ref loops in > rpl, seg6 and ioam6 lwtunnels"), we also need a conditional dst cache on > input for seg6_iptunnel and rpl_iptunnel to prevent dst ref loops (i.e., > if the packet destination did not change, we may end up recording a > reference to the lwtunnel in its own cache, and the lwtunnel state will > never be freed). > > Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel") > Fixes: af4a2209b134 ("ipv6: sr: use dst_cache in seg6_input") > Signed-off-by: Justin Iurman <justin.iurman@uliege.be> > Cc: Alexander Aring <alex.aring@gmail.com> > Cc: David Lebrun <dlebrun@google.com> Not an expert but was asked to take a look. Seems consistent with the output path and comparing the state address seems safe as it is only compared and never dereferenced after dropping the dst in the input path. I would have probably split it into two patches to make it a bit easier to backport. 5.4.y needs the seg6 fix, but not the rpl one. Reviewed-by: Ido Schimmel <idosch@nvidia.com>
On 2/13/25 13:27, Ido Schimmel wrote: > On Tue, Feb 11, 2025 at 11:16:22PM +0100, Justin Iurman wrote: >> As a follow up to commit 92191dd10730 ("net: ipv6: fix dst ref loops in >> rpl, seg6 and ioam6 lwtunnels"), we also need a conditional dst cache on >> input for seg6_iptunnel and rpl_iptunnel to prevent dst ref loops (i.e., >> if the packet destination did not change, we may end up recording a >> reference to the lwtunnel in its own cache, and the lwtunnel state will >> never be freed). >> >> Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel") >> Fixes: af4a2209b134 ("ipv6: sr: use dst_cache in seg6_input") >> Signed-off-by: Justin Iurman <justin.iurman@uliege.be> >> Cc: Alexander Aring <alex.aring@gmail.com> >> Cc: David Lebrun <dlebrun@google.com> > > Not an expert but was asked to take a look. Seems consistent with the > output path and comparing the state address seems safe as it is only > compared and never dereferenced after dropping the dst in the input > path. > > I would have probably split it into two patches to make it a bit easier > to backport. 5.4.y needs the seg6 fix, but not the rpl one. > > Reviewed-by: Ido Schimmel <idosch@nvidia.com> Thanks Ido. I'll split it in two for v3.
diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c index 0ac4283acdf2..c26bf284459f 100644 --- a/net/ipv6/rpl_iptunnel.c +++ b/net/ipv6/rpl_iptunnel.c @@ -262,10 +262,18 @@ static int rpl_input(struct sk_buff *skb) { struct dst_entry *orig_dst = skb_dst(skb); struct dst_entry *dst = NULL; + struct lwtunnel_state *lwtst; struct rpl_lwt *rlwt; int err; - rlwt = rpl_lwt_lwtunnel(orig_dst->lwtstate); + /* Get the address of lwtstate now, because "orig_dst" may not be there + * anymore after a call to skb_dst_drop(). Note that ip6_route_input() + * also calls skb_dst_drop(). Below, we compare the address of lwtstate + * to detect loops. + */ + lwtst = orig_dst->lwtstate; + + rlwt = rpl_lwt_lwtunnel(lwtst); local_bh_disable(); dst = dst_cache_get(&rlwt->cache); @@ -280,7 +288,9 @@ static int rpl_input(struct sk_buff *skb) if (!dst) { ip6_route_input(skb); dst = skb_dst(skb); - if (!dst->error) { + + /* cache only if we don't create a dst reference loop */ + if (!dst->error && lwtst != dst->lwtstate) { local_bh_disable(); dst_cache_set_ip6(&rlwt->cache, dst, &ipv6_hdr(skb)->saddr); diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c index 33833b2064c0..6045e850b4bf 100644 --- a/net/ipv6/seg6_iptunnel.c +++ b/net/ipv6/seg6_iptunnel.c @@ -472,10 +472,18 @@ static int seg6_input_core(struct net *net, struct sock *sk, { struct dst_entry *orig_dst = skb_dst(skb); struct dst_entry *dst = NULL; + struct lwtunnel_state *lwtst; struct seg6_lwt *slwt; int err; - slwt = seg6_lwt_lwtunnel(orig_dst->lwtstate); + /* Get the address of lwtstate now, because "orig_dst" may not be there + * anymore after a call to skb_dst_drop(). Note that ip6_route_input() + * also calls skb_dst_drop(). Below, we compare the address of lwtstate + * to detect loops. + */ + lwtst = orig_dst->lwtstate; + + slwt = seg6_lwt_lwtunnel(lwtst); local_bh_disable(); dst = dst_cache_get(&slwt->cache); @@ -490,7 +498,9 @@ static int seg6_input_core(struct net *net, struct sock *sk, if (!dst) { ip6_route_input(skb); dst = skb_dst(skb); - if (!dst->error) { + + /* cache only if we don't create a dst reference loop */ + if (!dst->error && lwtst != dst->lwtstate) { local_bh_disable(); dst_cache_set_ip6(&slwt->cache, dst, &ipv6_hdr(skb)->saddr);
As a follow up to commit 92191dd10730 ("net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels"), we also need a conditional dst cache on input for seg6_iptunnel and rpl_iptunnel to prevent dst ref loops (i.e., if the packet destination did not change, we may end up recording a reference to the lwtunnel in its own cache, and the lwtunnel state will never be freed). Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel") Fixes: af4a2209b134 ("ipv6: sr: use dst_cache in seg6_input") Signed-off-by: Justin Iurman <justin.iurman@uliege.be> Cc: Alexander Aring <alex.aring@gmail.com> Cc: David Lebrun <dlebrun@google.com> --- net/ipv6/rpl_iptunnel.c | 14 ++++++++++++-- net/ipv6/seg6_iptunnel.c | 14 ++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-)