| Message ID | 20250129021346.2333089-1-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/2] net: ipv6: fix dst refleaks in rpl, seg6 and ioam6 lwtunnels | expand |
On 1/29/25 03:13, Jakub Kicinski wrote: > dst_cache_get() gives us a reference, we need to release it. > > Discovered by the ioam6.sh test, kmemleak was recently fixed > to catch per-cpu memory leaks. > > Fixes: 985ec6f5e623 ("net: ipv6: rpl_iptunnel: mitigate 2-realloc issue") > Fixes: 40475b63761a ("net: ipv6: seg6_iptunnel: mitigate 2-realloc issue") > Fixes: dce525185bc9 ("net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue") > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > CC: dsahern@kernel.org > CC: justin.iurman@uliege.be > --- > net/ipv6/ioam6_iptunnel.c | 5 +++-- > net/ipv6/rpl_iptunnel.c | 6 ++++-- > net/ipv6/seg6_iptunnel.c | 6 ++++-- > 3 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c > index 28e5a89dc255..3936c137a572 100644 > --- a/net/ipv6/ioam6_iptunnel.c > +++ b/net/ipv6/ioam6_iptunnel.c > @@ -336,7 +336,7 @@ 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; > + struct dst_entry *dst = skb_dst(skb), *cache_dst = NULL; > struct in6_addr orig_daddr; > struct ioam6_lwt *ilwt; > int err = -EINVAL; > @@ -407,7 +407,6 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb) > cache_dst = ip6_route_output(net, NULL, &fl6); > if (cache_dst->error) { > err = cache_dst->error; > - dst_release(cache_dst); > goto drop; > } > > @@ -426,8 +425,10 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb) > return dst_output(net, sk, skb); > } > out: > + dst_release(cache_dst); > return dst->lwtstate->orig_output(net, sk, skb); > drop: > + dst_release(cache_dst); > kfree_skb(skb); > return err; > } > diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c > index 7ba22d2f2bfe..9b7d03563115 100644 > --- a/net/ipv6/rpl_iptunnel.c > +++ b/net/ipv6/rpl_iptunnel.c > @@ -232,7 +232,6 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb) > dst = ip6_route_output(net, NULL, &fl6); > if (dst->error) { > err = dst->error; > - dst_release(dst); > goto drop; > } > > @@ -251,6 +250,7 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb) > return dst_output(net, sk, skb); > > drop: > + dst_release(dst); > kfree_skb(skb); > return err; > } > @@ -269,8 +269,10 @@ static int rpl_input(struct sk_buff *skb) > local_bh_enable(); > > err = rpl_do_srh(skb, rlwt, dst); > - if (unlikely(err)) > + if (unlikely(err)) { > + dst_release(dst); > goto drop; > + } > > if (!dst) { > ip6_route_input(skb); > diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c > index 4bf937bfc263..eacc4e91b48e 100644 > --- a/net/ipv6/seg6_iptunnel.c > +++ b/net/ipv6/seg6_iptunnel.c > @@ -482,8 +482,10 @@ static int seg6_input_core(struct net *net, struct sock *sk, > local_bh_enable(); > > err = seg6_do_srh(skb, dst); > - if (unlikely(err)) > + if (unlikely(err)) { > + dst_release(dst); > goto drop; > + } > > if (!dst) { > ip6_route_input(skb); > @@ -571,7 +573,6 @@ static int seg6_output_core(struct net *net, struct sock *sk, > dst = ip6_route_output(net, NULL, &fl6); > if (dst->error) { > err = dst->error; > - dst_release(dst); > goto drop; > } > > @@ -593,6 +594,7 @@ static int seg6_output_core(struct net *net, struct sock *sk, > > return dst_output(net, sk, skb); > drop: > + dst_release(dst); > kfree_skb(skb); > return err; > } Reviewed-by: Justin Iurman <justin.iurman@uliege.be> Thanks Jakub!
On 1/29/25 03:13, Jakub Kicinski wrote: > dst_cache_get() gives us a reference, we need to release it. > > Discovered by the ioam6.sh test, kmemleak was recently fixed > to catch per-cpu memory leaks. > > Fixes: 985ec6f5e623 ("net: ipv6: rpl_iptunnel: mitigate 2-realloc issue") > Fixes: 40475b63761a ("net: ipv6: seg6_iptunnel: mitigate 2-realloc issue") > Fixes: dce525185bc9 ("net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue") > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > CC: dsahern@kernel.org > CC: justin.iurman@uliege.be > --- > net/ipv6/ioam6_iptunnel.c | 5 +++-- > net/ipv6/rpl_iptunnel.c | 6 ++++-- > net/ipv6/seg6_iptunnel.c | 6 ++++-- > 3 files changed, 11 insertions(+), 6 deletions(-) I think both ila_output() and tipc_udp_xmit() should also be patched accordingly. Other users seem fine. > diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c > index 28e5a89dc255..3936c137a572 100644 > --- a/net/ipv6/ioam6_iptunnel.c > +++ b/net/ipv6/ioam6_iptunnel.c > @@ -336,7 +336,7 @@ 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; > + struct dst_entry *dst = skb_dst(skb), *cache_dst = NULL; > struct in6_addr orig_daddr; > struct ioam6_lwt *ilwt; > int err = -EINVAL; > @@ -407,7 +407,6 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb) > cache_dst = ip6_route_output(net, NULL, &fl6); > if (cache_dst->error) { > err = cache_dst->error; > - dst_release(cache_dst); > goto drop; > } > > @@ -426,8 +425,10 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb) > return dst_output(net, sk, skb); > } > out: > + dst_release(cache_dst); > return dst->lwtstate->orig_output(net, sk, skb); > drop: > + dst_release(cache_dst); > kfree_skb(skb); > return err; > } > diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c > index 7ba22d2f2bfe..9b7d03563115 100644 > --- a/net/ipv6/rpl_iptunnel.c > +++ b/net/ipv6/rpl_iptunnel.c > @@ -232,7 +232,6 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb) > dst = ip6_route_output(net, NULL, &fl6); > if (dst->error) { > err = dst->error; > - dst_release(dst); > goto drop; > } > > @@ -251,6 +250,7 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb) > return dst_output(net, sk, skb); > > drop: > + dst_release(dst); > kfree_skb(skb); > return err; > } > @@ -269,8 +269,10 @@ static int rpl_input(struct sk_buff *skb) > local_bh_enable(); > > err = rpl_do_srh(skb, rlwt, dst); > - if (unlikely(err)) > + if (unlikely(err)) { > + dst_release(dst); > goto drop; > + } > > if (!dst) { > ip6_route_input(skb); > diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c > index 4bf937bfc263..eacc4e91b48e 100644 > --- a/net/ipv6/seg6_iptunnel.c > +++ b/net/ipv6/seg6_iptunnel.c > @@ -482,8 +482,10 @@ static int seg6_input_core(struct net *net, struct sock *sk, > local_bh_enable(); > > err = seg6_do_srh(skb, dst); > - if (unlikely(err)) > + if (unlikely(err)) { > + dst_release(dst); > goto drop; > + } > > if (!dst) { > ip6_route_input(skb); > @@ -571,7 +573,6 @@ static int seg6_output_core(struct net *net, struct sock *sk, > dst = ip6_route_output(net, NULL, &fl6); > if (dst->error) { > err = dst->error; > - dst_release(dst); > goto drop; > } > > @@ -593,6 +594,7 @@ static int seg6_output_core(struct net *net, struct sock *sk, > > return dst_output(net, sk, skb); > drop: > + dst_release(dst); > kfree_skb(skb); > return err; > }
On Wed, 29 Jan 2025 17:17:28 +0100 Justin Iurman wrote: > > net/ipv6/ioam6_iptunnel.c | 5 +++-- > > net/ipv6/rpl_iptunnel.c | 6 ++++-- > > net/ipv6/seg6_iptunnel.c | 6 ++++-- > > 3 files changed, 11 insertions(+), 6 deletions(-) > > I think both ila_output() and tipc_udp_xmit() should also be patched > accordingly. Other users seem fine. TIPC is not a lwt, the cache belongs to a socket not another dst, AFAICT. I think in ILA ilwt->connected will only be set if we re-routed the traffic to the real address already? So the dst can't match. CC: Tom, full patch: https://lore.kernel.org/all/20250129021346.2333089-2-kuba@kernel.org/
On 1/29/25 21:23, Jakub Kicinski wrote: > On Wed, 29 Jan 2025 17:17:28 +0100 Justin Iurman wrote: >>> net/ipv6/ioam6_iptunnel.c | 5 +++-- >>> net/ipv6/rpl_iptunnel.c | 6 ++++-- >>> net/ipv6/seg6_iptunnel.c | 6 ++++-- >>> 3 files changed, 11 insertions(+), 6 deletions(-) >> >> I think both ila_output() and tipc_udp_xmit() should also be patched >> accordingly. Other users seem fine. > > TIPC is not a lwt, the cache belongs to a socket not another dst, > AFAICT. *sigh* You're right, my bad. > I think in ILA ilwt->connected will only be set if we re-routed > the traffic to the real address already? So the dst can't match. > CC: Tom, full patch: > https://lore.kernel.org/all/20250129021346.2333089-2-kuba@kernel.org/ Looks like I was in a hurry when I reported this one. On second reading, this is also how I understand it.
diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c index 28e5a89dc255..3936c137a572 100644 --- a/net/ipv6/ioam6_iptunnel.c +++ b/net/ipv6/ioam6_iptunnel.c @@ -336,7 +336,7 @@ 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; + struct dst_entry *dst = skb_dst(skb), *cache_dst = NULL; struct in6_addr orig_daddr; struct ioam6_lwt *ilwt; int err = -EINVAL; @@ -407,7 +407,6 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb) cache_dst = ip6_route_output(net, NULL, &fl6); if (cache_dst->error) { err = cache_dst->error; - dst_release(cache_dst); goto drop; } @@ -426,8 +425,10 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb) return dst_output(net, sk, skb); } out: + dst_release(cache_dst); return dst->lwtstate->orig_output(net, sk, skb); drop: + dst_release(cache_dst); kfree_skb(skb); return err; } diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c index 7ba22d2f2bfe..9b7d03563115 100644 --- a/net/ipv6/rpl_iptunnel.c +++ b/net/ipv6/rpl_iptunnel.c @@ -232,7 +232,6 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb) dst = ip6_route_output(net, NULL, &fl6); if (dst->error) { err = dst->error; - dst_release(dst); goto drop; } @@ -251,6 +250,7 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb) return dst_output(net, sk, skb); drop: + dst_release(dst); kfree_skb(skb); return err; } @@ -269,8 +269,10 @@ static int rpl_input(struct sk_buff *skb) local_bh_enable(); err = rpl_do_srh(skb, rlwt, dst); - if (unlikely(err)) + if (unlikely(err)) { + dst_release(dst); goto drop; + } if (!dst) { ip6_route_input(skb); diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c index 4bf937bfc263..eacc4e91b48e 100644 --- a/net/ipv6/seg6_iptunnel.c +++ b/net/ipv6/seg6_iptunnel.c @@ -482,8 +482,10 @@ static int seg6_input_core(struct net *net, struct sock *sk, local_bh_enable(); err = seg6_do_srh(skb, dst); - if (unlikely(err)) + if (unlikely(err)) { + dst_release(dst); goto drop; + } if (!dst) { ip6_route_input(skb); @@ -571,7 +573,6 @@ static int seg6_output_core(struct net *net, struct sock *sk, dst = ip6_route_output(net, NULL, &fl6); if (dst->error) { err = dst->error; - dst_release(dst); goto drop; } @@ -593,6 +594,7 @@ static int seg6_output_core(struct net *net, struct sock *sk, return dst_output(net, sk, skb); drop: + dst_release(dst); kfree_skb(skb); return err; }
dst_cache_get() gives us a reference, we need to release it. Discovered by the ioam6.sh test, kmemleak was recently fixed to catch per-cpu memory leaks. Fixes: 985ec6f5e623 ("net: ipv6: rpl_iptunnel: mitigate 2-realloc issue") Fixes: 40475b63761a ("net: ipv6: seg6_iptunnel: mitigate 2-realloc issue") Fixes: dce525185bc9 ("net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue") Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- CC: dsahern@kernel.org CC: justin.iurman@uliege.be --- net/ipv6/ioam6_iptunnel.c | 5 +++-- net/ipv6/rpl_iptunnel.c | 6 ++++-- net/ipv6/seg6_iptunnel.c | 6 ++++-- 3 files changed, 11 insertions(+), 6 deletions(-)