Re: [PATCH v3 net-next] net: diag: support SOCK_DESTROY for UDP sockets

[David Ahern]

On 8/23/16 11:13 AM, Lorenzo Colitti wrote:
> On Wed, Aug 24, 2016 at 2:02 AM, David Ahern <d...@cumulusnetworks.com> wrote:
>>> Looking at the code again, it seems that there's a bug in
>>> sock_diag_destroy. If the destroy operation does not occur (e.g., if
>>> sock_diag_destroy returns EPERM, or the protocol doesn't support
>>> destroy), then it doesn't release the refcount. This affects the TCP
>>> code as well and as such is my fault, not yours. The most obvious way
>>> to fix this might be to call sock_gen_put in sock_diag_destroy.
>>
>> sock_gen_put seems specific to tcp which is why I used sock_put here.
> 
> I thought sock_gen_put can be used on UDP as well, but it does seem a
> bit counterintuitive.
> 
>> Perhaps better to have the callers of sock_diag_destroy handle the refcnt? 
>> This
>> function took it; it should release it success or fail. Same for tcp.
> 
> So you'd remove the sock_put and sock_gen_put calls from tcp_abort and
> just add one sock_gen_put in tcp_diag_destroy? That does seem simpler.
> 

untested and mangled on a copy and paste but the intent is:

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index f1a9a0a8a1f3..54c5d0a8f6e9 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3176,7 +3176,6 @@ int tcp_abort(struct sock *sk, int err)
                        local_bh_enable();
                        return 0;
                }
-               sock_gen_put(sk);
                return -EOPNOTSUPP;
        }

@@ -3205,7 +3204,6 @@ int tcp_abort(struct sock *sk, int err)
        bh_unlock_sock(sk);
        local_bh_enable();
        release_sock(sk);
-       sock_put(sk);
        return 0;
 }
 EXPORT_SYMBOL_GPL(tcp_abort);
diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
index 4d610934fb39..a748c74aa8b7 100644
--- a/net/ipv4/tcp_diag.c
+++ b/net/ipv4/tcp_diag.c
@@ -54,11 +54,16 @@ static int tcp_diag_destroy(struct sk_buff *in_skb,
 {
        struct net *net = sock_net(in_skb->sk);
        struct sock *sk = inet_diag_find_one_icsk(net, &tcp_hashinfo, req);
+       int err;

        if (IS_ERR(sk))
                return PTR_ERR(sk);

-       return sock_diag_destroy(sk, ECONNABORTED);
+       err = sock_diag_destroy(sk, ECONNABORTED);
+
+       sock_gen_put(sk);     <---- if (!sk_fullsock(sk)) needed here? seems 
like this works for all TCP
+
+       return err;
 }
 #endif