| Message ID | 20250130031519.2716843-2-kuba@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2,1/2] net: ipv6: fix dst refleaks in rpl, seg6 and ioam6 lwtunnels | expand |
On Wed, Jan 29, 2025 at 07:15:19PM -0800, Jakub Kicinski wrote: > Some lwtunnels have a dst cache for post-transformation dst. > 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. > > Discovered by the ioam6.sh test, kmemleak was recently fixed > to catch per-cpu memory leaks. I'm not sure if rpl and seg6 > can actually hit this, but in principle I don't see why not. > > 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> > --- > v2: > - fix spello in the comments > v1: https://lore.kernel.org/20250129021346.2333089-2-kuba@kernel.org Hi Jakub, This fix looks correct to me. And I believe that the double allocation issue raised at the cited link for v1 relates to an optimisation rather than a bug, so this patch seems appropriate for net without addressing that issue. I am, however, unsure why the cited patches are used in the Fixes tags rather than the patches that added use of the cache to the output routines. e.g. af4a2209b134 ("ipv6: sr: use dst_cache in seg6_input") ...
On 1/30/25 4:15 AM, Jakub Kicinski wrote: > Some lwtunnels have a dst cache for post-transformation dst. > 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. The series LGTM, but I'm wondering if we can't have a similar loop for input lwt? Thanks, Paolo
On 1/30/25 11:28, Simon Horman wrote: > On Wed, Jan 29, 2025 at 07:15:19PM -0800, Jakub Kicinski wrote: >> Some lwtunnels have a dst cache for post-transformation dst. >> 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. >> >> Discovered by the ioam6.sh test, kmemleak was recently fixed >> to catch per-cpu memory leaks. I'm not sure if rpl and seg6 >> can actually hit this, but in principle I don't see why not. >> >> 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> >> --- >> v2: >> - fix spello in the comments >> v1: https://lore.kernel.org/20250129021346.2333089-2-kuba@kernel.org > > Hi Jakub, > > This fix looks correct to me. And I believe that the double allocation > issue raised at the cited link for v1 relates to an optimisation > rather than a bug, so this patch seems appropriate for net without > addressing that issue. +1. Just to make sure, do you think I should re-apply a fix for the double allocation on top of this one and target net or net-next? > I am, however, unsure why the cited patches are used in the Fixes tags > rather than the patches that added use of the cache to the output > routines. > > e.g. af4a2209b134 ("ipv6: sr: use dst_cache in seg6_input") > > ... This was my thought as well. While Fixes tags are correct for #1, what #2 is trying to fix was already there before 985ec6f5e623, 40475b63761a and dce525185bc9 respectively. I think it should be: Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation") Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and injection with lwtunnels") Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel")
On 1/30/25 12:34, Paolo Abeni wrote: > > > On 1/30/25 4:15 AM, Jakub Kicinski wrote: >> Some lwtunnels have a dst cache for post-transformation dst. >> 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. > > The series LGTM, but I'm wondering if we can't have a similar loop for > input lwt? Hmmm, I think Paolo is right. At least, I don't see a reason why it wouldn't be correct. We should also take care of input lwt for both seg6_iptunnel and rpl_iptunnel (ioam6_iptunnel does not implement input).
On Thu, 30 Jan 2025 14:52:14 +0100 Justin Iurman wrote: > > On 1/30/25 4:15 AM, Jakub Kicinski wrote: > >> Some lwtunnels have a dst cache for post-transformation dst. > >> 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. > > > > The series LGTM, but I'm wondering if we can't have a similar loop for > > input lwt? > > Hmmm, I think Paolo is right. At least, I don't see a reason why it > wouldn't be correct. We should also take care of input lwt for both > seg6_iptunnel and rpl_iptunnel (ioam6_iptunnel does not implement input). Would you be able to take care of that? And perhaps add a selftest at least for the looped cases?
On Thu, 30 Jan 2025 14:41:56 +0100 Justin Iurman wrote: > This was my thought as well. While Fixes tags are correct for #1, what > #2 is trying to fix was already there before 985ec6f5e623, 40475b63761a > and dce525185bc9 respectively. I think it should be: > > Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation") > Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and > injection with lwtunnels") > Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel") I'll swap the tags when applying, if there are no other comments.
On 1/30/25 15:55, Jakub Kicinski wrote: > On Thu, 30 Jan 2025 14:52:14 +0100 Justin Iurman wrote: >>> On 1/30/25 4:15 AM, Jakub Kicinski wrote: >>>> Some lwtunnels have a dst cache for post-transformation dst. >>>> 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. >>> >>> The series LGTM, but I'm wondering if we can't have a similar loop for >>> input lwt? >> >> Hmmm, I think Paolo is right. At least, I don't see a reason why it >> wouldn't be correct. We should also take care of input lwt for both >> seg6_iptunnel and rpl_iptunnel (ioam6_iptunnel does not implement input). > > Would you be able to take care of that? Sure, I'll send a patch as soon as this patchset is merged to net. > And perhaps add a selftest at least for the looped cases? ioam6.sh already triggers the looped cases in both inline and encap tests. Not sure about seg6 though, and there is no selftest for rpl.
On Thu, Jan 30, 2025 at 06:56:10AM -0800, Jakub Kicinski wrote: > On Thu, 30 Jan 2025 14:41:56 +0100 Justin Iurman wrote: > > This was my thought as well. While Fixes tags are correct for #1, what > > #2 is trying to fix was already there before 985ec6f5e623, 40475b63761a > > and dce525185bc9 respectively. I think it should be: > > > > Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation") > > Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and > > injection with lwtunnels") > > Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel") > > I'll swap the tags when applying, if there are no other comments. Thanks, SGTM. Reviewed-by: Simon Horman <horms@kernel.org>
On Thu, 30 Jan 2025 16:12:23 +0100 Justin Iurman wrote: > > And perhaps add a selftest at least for the looped cases? > > ioam6.sh already triggers the looped cases in both inline and encap > tests. Not sure about seg6 though, and there is no selftest for rpl. Right! To be clear I meant just for seg6 and rpl. The ioam6 test is clearly paying off, thanks for putting in the work there! :)
diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c index 3936c137a572..2c383c12a431 100644 --- a/net/ipv6/ioam6_iptunnel.c +++ b/net/ipv6/ioam6_iptunnel.c @@ -410,9 +410,12 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb) goto drop; } - local_bh_disable(); - dst_cache_set_ip6(&ilwt->cache, cache_dst, &fl6.saddr); - local_bh_enable(); + /* cache only if we don't create a dst reference loop */ + if (dst->lwtstate != cache_dst->lwtstate) { + local_bh_disable(); + dst_cache_set_ip6(&ilwt->cache, cache_dst, &fl6.saddr); + local_bh_enable(); + } err = skb_cow_head(skb, LL_RESERVED_SPACE(cache_dst->dev)); if (unlikely(err)) diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c index 9b7d03563115..0ac4283acdf2 100644 --- a/net/ipv6/rpl_iptunnel.c +++ b/net/ipv6/rpl_iptunnel.c @@ -235,9 +235,12 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb) goto drop; } - local_bh_disable(); - dst_cache_set_ip6(&rlwt->cache, dst, &fl6.saddr); - local_bh_enable(); + /* cache only if we don't create a dst reference loop */ + if (orig_dst->lwtstate != dst->lwtstate) { + local_bh_disable(); + dst_cache_set_ip6(&rlwt->cache, dst, &fl6.saddr); + local_bh_enable(); + } err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev)); if (unlikely(err)) diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c index eacc4e91b48e..33833b2064c0 100644 --- a/net/ipv6/seg6_iptunnel.c +++ b/net/ipv6/seg6_iptunnel.c @@ -576,9 +576,12 @@ static int seg6_output_core(struct net *net, struct sock *sk, goto drop; } - local_bh_disable(); - dst_cache_set_ip6(&slwt->cache, dst, &fl6.saddr); - local_bh_enable(); + /* cache only if we don't create a dst reference loop */ + if (orig_dst->lwtstate != dst->lwtstate) { + local_bh_disable(); + dst_cache_set_ip6(&slwt->cache, dst, &fl6.saddr); + local_bh_enable(); + } err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev)); if (unlikely(err))
Some lwtunnels have a dst cache for post-transformation dst. 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. Discovered by the ioam6.sh test, kmemleak was recently fixed to catch per-cpu memory leaks. I'm not sure if rpl and seg6 can actually hit this, but in principle I don't see why not. 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> --- v2: - fix spello in the comments v1: https://lore.kernel.org/20250129021346.2333089-2-kuba@kernel.org CC: dsahern@kernel.org CC: justin.iurman@uliege.be --- net/ipv6/ioam6_iptunnel.c | 9 ++++++--- net/ipv6/rpl_iptunnel.c | 9 ++++++--- net/ipv6/seg6_iptunnel.c | 9 ++++++--- 3 files changed, 18 insertions(+), 9 deletions(-)