The sendmsg() syscall handler for PPPoL2TP doesn't decrease the socket
reference counter after successful transmissions. Any successful
sendmsg() call from userspace will then increase the reference counter
forever, thus preventing the kernel's session and tunnel data from
being freed later on.
The problem only happens when writing directly on L2TP sockets.
PPP sockets attached to L2TP are unaffected as the PPP subsystem
uses pppol2tp_xmit() which symmetrically increase/decrease reference
counters.
This patch adds the missing call to sock_put() before returning from
pppol2tp_sendmsg().
Cc: <[email protected]>
Signed-off-by: Guillaume Nault <[email protected]>
---
net/l2tp/l2tp_ppp.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 3f4e3af..6a53371 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -355,6 +355,7 @@ static int pppol2tp_sendmsg(struct kiocb *iocb, struct socket *sock, struct msgh
l2tp_xmit_skb(session, skb, session->hdr_len);
sock_put(ps->tunnel_sock);
+ sock_put(sk);
return error;
--
1.7.10.4
From: Guillaume Nault <[email protected]>
Date: Fri, 1 Mar 2013 16:02:02 +0100
> The sendmsg() syscall handler for PPPoL2TP doesn't decrease the socket
> reference counter after successful transmissions. Any successful
> sendmsg() call from userspace will then increase the reference counter
> forever, thus preventing the kernel's session and tunnel data from
> being freed later on.
>
> The problem only happens when writing directly on L2TP sockets.
> PPP sockets attached to L2TP are unaffected as the PPP subsystem
> uses pppol2tp_xmit() which symmetrically increase/decrease reference
> counters.
>
> This patch adds the missing call to sock_put() before returning from
> pppol2tp_sendmsg().
>
> Cc: <[email protected]>
> Signed-off-by: Guillaume Nault <[email protected]>
Looking at how this code works, it is such a terrible design. This
whole reference counting issue exists purely because
pppol2tp_sock_to_session() grabs the 'sk' reference.
In all but one case, it need not do this.
The socket system calls have an implicit reference to 'sk' via
socket->sk. If you can get into the system call and socket->sk
is non-NULL then 'sk' is NOT going anywhere.
And all of these system call handlers have this pattern:
session = pppol2tp_sock_to_session(sk);
...
sock_put(sk);
The only case where the reference count is really needed is that
sequence in pppol2tp_release().
Long term the right thing to do here is stop having this session
grabber function take the 'sk' reference. Then in pppol2tp_release
we'll grab a reference explicitly. At all the other call sites we
then blast aweay all of the sock_put(sk) paths.
Anyways, for now I'll apply this patch and queue it up for -stable,
thanks.
On Fri, Mar 01, 2013 at 02:12:52PM -0500, David Miller wrote:
> From: Guillaume Nault <[email protected]>
> Date: Fri, 1 Mar 2013 16:02:02 +0100
>
> > The sendmsg() syscall handler for PPPoL2TP doesn't decrease the socket
> > reference counter after successful transmissions. Any successful
> > sendmsg() call from userspace will then increase the reference counter
> > forever, thus preventing the kernel's session and tunnel data from
> > being freed later on.
> >
> > The problem only happens when writing directly on L2TP sockets.
> > PPP sockets attached to L2TP are unaffected as the PPP subsystem
> > uses pppol2tp_xmit() which symmetrically increase/decrease reference
> > counters.
> >
> > This patch adds the missing call to sock_put() before returning from
> > pppol2tp_sendmsg().
> >
> > Cc: <[email protected]>
> > Signed-off-by: Guillaume Nault <[email protected]>
>
> Looking at how this code works, it is such a terrible design. This
> whole reference counting issue exists purely because
> pppol2tp_sock_to_session() grabs the 'sk' reference.
>
> In all but one case, it need not do this.
>
> The socket system calls have an implicit reference to 'sk' via
> socket->sk. If you can get into the system call and socket->sk
> is non-NULL then 'sk' is NOT going anywhere.
>
> And all of these system call handlers have this pattern:
>
> session = pppol2tp_sock_to_session(sk);
> ...
> sock_put(sk);
>
> The only case where the reference count is really needed is that
> sequence in pppol2tp_release().
>
> Long term the right thing to do here is stop having this session
> grabber function take the 'sk' reference. Then in pppol2tp_release
> we'll grab a reference explicitly. At all the other call sites we
> then blast aweay all of the sock_put(sk) paths.
>
Could this also apply to l2tp_sock_to_tunnel() (in l2tp_core.h)? As per
my understanding, none of its callers needs to take a socket reference.
So sock_hold() could be removed in both pppol2tp_sock_to_session() and
l2tp_sock_to_tunnel() functions. The corresponding sock_put() calls
would then be removed from all calling functions but pppol2tp_release().
If this is correct, I'll send a patch for net-next.
From: Guillaume Nault <[email protected]>
Date: Tue, 12 Mar 2013 11:36:50 +0100
> On Fri, Mar 01, 2013 at 02:12:52PM -0500, David Miller wrote:
>> Looking at how this code works, it is such a terrible design. This
>> whole reference counting issue exists purely because
>> pppol2tp_sock_to_session() grabs the 'sk' reference.
>>
>> In all but one case, it need not do this.
>>
>> The socket system calls have an implicit reference to 'sk' via
>> socket->sk. If you can get into the system call and socket->sk
>> is non-NULL then 'sk' is NOT going anywhere.
>>
>> And all of these system call handlers have this pattern:
>>
>> session = pppol2tp_sock_to_session(sk);
>> ...
>> sock_put(sk);
>>
>> The only case where the reference count is really needed is that
>> sequence in pppol2tp_release().
>>
>> Long term the right thing to do here is stop having this session
>> grabber function take the 'sk' reference. Then in pppol2tp_release
>> we'll grab a reference explicitly. At all the other call sites we
>> then blast aweay all of the sock_put(sk) paths.
>>
> Could this also apply to l2tp_sock_to_tunnel() (in l2tp_core.h)? As per
> my understanding, none of its callers needs to take a socket reference.
> So sock_hold() could be removed in both pppol2tp_sock_to_session() and
> l2tp_sock_to_tunnel() functions. The corresponding sock_put() calls
> would then be removed from all calling functions but pppol2tp_release().
> If this is correct, I'll send a patch for net-next.
Yes, it could be simplified in this way too. Just make sure that this
interface is only used in system call / user context, where we know
the underlying socket cannot go away on us.