* [PATCH net-next] mptcp: put reference in mptcp timeout timer
@ 2020-11-24 16:24 Florian Westphal
2020-11-24 16:42 ` Paolo Abeni
0 siblings, 1 reply; 3+ messages in thread
From: Florian Westphal @ 2020-11-24 16:24 UTC (permalink / raw)
To: netdev; +Cc: mptcp, Florian Westphal, Paolo Abeni, Davide Caratti
On close this timer might be scheduled. mptcp uses sk_reset_timer for
this, so the a reference on the mptcp socket is taken.
This causes a refcount leak which can for example be reproduced
with 'mp_join_server_v4.pkt' from the mptcp-packetdrill repo.
The leak has nothing to do with join requests, v1_mp_capable_bind_no_cs.pkt
works too when replacing the last ack mpcapable to v1 instead of v0.
unreferenced object 0xffff888109bba040 (size 2744):
comm "packetdrill", [..]
backtrace:
[..] sk_prot_alloc.isra.0+0x2b/0xc0
[..] sk_clone_lock+0x2f/0x740
[..] mptcp_sk_clone+0x33/0x1a0
[..] subflow_syn_recv_sock+0x2b1/0x690 [..]
Fixes: e16163b6e2b7 ("mptcp: refactor shutdown and close")
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Davide Caratti <dcaratti@redhat.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/mptcp/protocol.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 4b7794835fea..dc979571f561 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1710,6 +1710,7 @@ static void mptcp_timeout_timer(struct timer_list *t)
struct sock *sk = from_timer(sk, t, sk_timer);
mptcp_schedule_work(sk);
+ sock_put(sk);
}
/* Find an idle subflow. Return NULL if there is unacked data at tcp
--
2.26.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net-next] mptcp: put reference in mptcp timeout timer
2020-11-24 16:24 [PATCH net-next] mptcp: put reference in mptcp timeout timer Florian Westphal
@ 2020-11-24 16:42 ` Paolo Abeni
2020-11-25 20:33 ` Jakub Kicinski
0 siblings, 1 reply; 3+ messages in thread
From: Paolo Abeni @ 2020-11-24 16:42 UTC (permalink / raw)
To: Florian Westphal, netdev; +Cc: mptcp, Davide Caratti
On Tue, 2020-11-24 at 17:24 +0100, Florian Westphal wrote:
> On close this timer might be scheduled. mptcp uses sk_reset_timer for
> this, so the a reference on the mptcp socket is taken.
>
> This causes a refcount leak which can for example be reproduced
> with 'mp_join_server_v4.pkt' from the mptcp-packetdrill repo.
Whoops, my fault!
> The leak has nothing to do with join requests, v1_mp_capable_bind_no_cs.pkt
> works too when replacing the last ack mpcapable to v1 instead of v0.
>
> unreferenced object 0xffff888109bba040 (size 2744):
> comm "packetdrill", [..]
> backtrace:
> [..] sk_prot_alloc.isra.0+0x2b/0xc0
> [..] sk_clone_lock+0x2f/0x740
> [..] mptcp_sk_clone+0x33/0x1a0
> [..] subflow_syn_recv_sock+0x2b1/0x690 [..]
>
> Fixes: e16163b6e2b7 ("mptcp: refactor shutdown and close")
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Davide Caratti <dcaratti@redhat.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
> net/mptcp/protocol.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 4b7794835fea..dc979571f561 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1710,6 +1710,7 @@ static void mptcp_timeout_timer(struct timer_list *t)
> struct sock *sk = from_timer(sk, t, sk_timer);
>
> mptcp_schedule_work(sk);
> + sock_put(sk);
> }
>
> /* Find an idle subflow. Return NULL if there is unacked data at tcp
LGTM, thanks!
Acked-by: Paolo Abeni <pabeni@redhat.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net-next] mptcp: put reference in mptcp timeout timer
2020-11-24 16:42 ` Paolo Abeni
@ 2020-11-25 20:33 ` Jakub Kicinski
0 siblings, 0 replies; 3+ messages in thread
From: Jakub Kicinski @ 2020-11-25 20:33 UTC (permalink / raw)
To: Paolo Abeni; +Cc: Florian Westphal, netdev, mptcp, Davide Caratti
On Tue, 24 Nov 2020 17:42:21 +0100 Paolo Abeni wrote:
> On Tue, 2020-11-24 at 17:24 +0100, Florian Westphal wrote:
> > On close this timer might be scheduled. mptcp uses sk_reset_timer for
> > this, so the a reference on the mptcp socket is taken.
> >
> > This causes a refcount leak which can for example be reproduced
> > with 'mp_join_server_v4.pkt' from the mptcp-packetdrill repo.
>
> Whoops, my fault!
>
> > The leak has nothing to do with join requests, v1_mp_capable_bind_no_cs.pkt
> > works too when replacing the last ack mpcapable to v1 instead of v0.
> >
> > unreferenced object 0xffff888109bba040 (size 2744):
> > comm "packetdrill", [..]
> > backtrace:
> > [..] sk_prot_alloc.isra.0+0x2b/0xc0
> > [..] sk_clone_lock+0x2f/0x740
> > [..] mptcp_sk_clone+0x33/0x1a0
> > [..] subflow_syn_recv_sock+0x2b1/0x690 [..]
> >
> > Fixes: e16163b6e2b7 ("mptcp: refactor shutdown and close")
> > Cc: Paolo Abeni <pabeni@redhat.com>
> > Cc: Davide Caratti <dcaratti@redhat.com>
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> > net/mptcp/protocol.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 4b7794835fea..dc979571f561 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1710,6 +1710,7 @@ static void mptcp_timeout_timer(struct timer_list *t)
> > struct sock *sk = from_timer(sk, t, sk_timer);
> >
> > mptcp_schedule_work(sk);
> > + sock_put(sk);
> > }
> >
> > /* Find an idle subflow. Return NULL if there is unacked data at tcp
>
> LGTM, thanks!
> Acked-by: Paolo Abeni <pabeni@redhat.com>
Applied, thanks!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-25 20:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-24 16:24 [PATCH net-next] mptcp: put reference in mptcp timeout timer Florian Westphal
2020-11-24 16:42 ` Paolo Abeni
2020-11-25 20:33 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).