RFC: sk leak in sock_graft?

We're seeing a memleak when we run an infinite loop that loads/unloads rds-tcp, and runs some traffic between each load/unload.

Analysis shows that this is happening for the following reason:

inet_accept -> sock_graft does

	parent->sk = sk

but if the parent->sk was previously pointing at some other struct sock "old_sk" (happens in the case of rds_tcp_accept_one() which has historically called sock_create_kern() to set up the new_sock), we need to sock_put(old_sk), else we'd leak it.

In general, sock_graft() is cutting loose the parent->sk, so it looks like it needs to release its refcnt on it?

Patch below takes care of the leak in our case, but I could use some input on other locking considerations, and if this is ok with other modules that use sock_graft()

-----------------------patch below---------------------------------

index 5374c0d..014ad56 100644
@@ -1686,12 +1686,19 @@ static inline void sock_orphan(struct sock *sk)
 
 static inline void sock_graft(struct sock *sk, struct socket *parent)
 {
+       struct sock *old_sk;
+
        write_lock_bh(&sk->sk_callback_lock);
        sk->sk_wq = parent->wq;
+       old_sk = parent->sk;
        parent->sk = sk;
        sk_set_socket(sk, parent);
        security_sock_graft(sk, parent);
        write_unlock_bh(&sk->sk_callback_lock);
+       if (old_sk) {
+               sock_orphan(old_sk);
+               sock_put(old_sk);
+       }
 }

From: Sowmini Varadhan <[email protected]>
Date: Sat, 24 Jun 2017 09:08:27 -0400

It could simply be the case that rds-tcp is the first setup that created that situation where there is a parent->sk already.

In all of the normal accept*() code paths, a plain struct socket is allocated and nothing sets newsock->sk to anything before that sock_graft() call.

Why does rds-tcp need to call sock_graft() without those invariants met?

Thanks.

Possibly, I noticed that other callers call sock_create_lite() and I dont know the history here - this seems to have been the case from day-1 of rds-tcp. (and I dread changing rds_tcp_accept_kern() to do this, because then every module unload would need to go and check if sock->sk is non-null first, before cleaning it up

It would certainly help to declare "dont use sock_creeate_kern() if you are going to accept on this socket"- I dont see that being mandated anywhere.

It would also help to have a BUG_ON(parent->sk) or at least a WARN_ON(parent->sk) in sock_graft, before unilaterally assigning it to the new sk.

--Sowmini

I can look into getting rds_tcp_accept_one also calling sock_create_lite like every other caller, (though I may not get to this for another week, due to travel), but the code in sock_graft() doesnt look right either.

At the very least, there needs to be a WARN_ON(parent->sk) there, to provide a gentle dope-slap for the next slob that stumbles on this type of leak.

--Sowmini

From: Sowmini Varadhan <[email protected]>
Date: Tue, 27 Jun 2017 16:45:29 -0400

In the accept case it is, if anything, very wasteful.

This is because you allocate a sock and then immediately free it up.

I would say mimick the logic of sys_accept4() and just use sock_alloc() to allocate the struct socket without a struct sock. Or, as you say, use sock_create_lite() or a similar helper.