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

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

[David Ahern]

This implements SOCK_DESTROY for UDP sockets similar to what was done
for TCP with commit c1e64e298b8ca ("net: diag: Support destroying TCP
sockets.") A process with a UDP socket targeted for destroy is awakened
and recvmsg fails with ECONNABORTED.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
v3
- add missing sock_diag_check_cookie check after socket lookup per
  Eric'c comment

v2
- changed socket lookup to __udp{4,6}_lib_lookup

 include/net/udp.h   |  1 +
 net/ipv4/udp.c      | 17 +++++++++++++++
 net/ipv4/udp_diag.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 net/ipv6/udp.c      |  1 +
 4 files changed, 79 insertions(+)

diff --git a/include/net/udp.h b/include/net/udp.h
index 8894d7144189..ea53a87d880f 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -251,6 +251,7 @@ int udp_get_port(struct sock *sk, unsigned short snum,
 		 int (*saddr_cmp)(const struct sock *,
 				  const struct sock *));
 void udp_err(struct sk_buff *, u32);
+int udp_abort(struct sock *sk, int err);
 int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len);
 int udp_push_pending_frames(struct sock *sk);
 void udp_flush_pending_frames(struct sock *sk);
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 8f5f7f6026f7..044058835ea8 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2193,6 +2193,22 @@ unsigned int udp_poll(struct file *file, struct socket *sock, poll_table *wait)
 }
 EXPORT_SYMBOL(udp_poll);
 
+int udp_abort(struct sock *sk, int err)
+{
+	lock_sock(sk);
+
+	sk->sk_err = err;
+	sk->sk_error_report(sk);
+	udp_disconnect(sk, 0);
+
+	release_sock(sk);
+
+	sock_put(sk);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(udp_abort);
+
 struct proto udp_prot = {
 	.name		   = "UDP",
 	.owner		   = THIS_MODULE,
@@ -2224,6 +2240,7 @@ struct proto udp_prot = {
 	.compat_getsockopt = compat_udp_getsockopt,
 #endif
 	.clear_sk	   = sk_prot_clear_portaddr_nulls,
+	.diag_destroy	   = udp_abort,
 };
 EXPORT_SYMBOL(udp_prot);
 
diff --git a/net/ipv4/udp_diag.c b/net/ipv4/udp_diag.c
index 3d5ccf4b1412..63896dd766f3 100644
--- a/net/ipv4/udp_diag.c
+++ b/net/ipv4/udp_diag.c
@@ -165,12 +165,69 @@ static void udp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
 	r->idiag_wqueue = sk_wmem_alloc_get(sk);
 }
 
+#ifdef CONFIG_INET_DIAG_DESTROY
+static int __udp_diag_destroy(struct sk_buff *in_skb,
+			      const struct inet_diag_req_v2 *req,
+			      struct udp_table *tbl)
+{
+	struct net *net = sock_net(in_skb->sk);
+	struct sock *sk;
+
+	rcu_read_lock();
+
+	if (req->sdiag_family == AF_INET)
+		sk = __udp4_lib_lookup(net,
+				req->id.idiag_dst[0], req->id.idiag_dport,
+				req->id.idiag_src[0], req->id.idiag_sport,
+				req->id.idiag_if, tbl, NULL);
+#if IS_ENABLED(CONFIG_IPV6)
+	else if (req->sdiag_family == AF_INET6)
+		sk = __udp6_lib_lookup(net,
+				(struct in6_addr *)req->id.idiag_dst,
+				req->id.idiag_dport,
+				(struct in6_addr *)req->id.idiag_src,
+				req->id.idiag_sport,
+				req->id.idiag_if, tbl, NULL);
+#endif
+	if (sk && !atomic_inc_not_zero(&sk->sk_refcnt))
+		sk = NULL;
+
+	rcu_read_unlock();
+
+	if (!sk)
+		return -ENOENT;
+
+	if (sock_diag_check_cookie(sk, req->id.idiag_cookie)) {
+		sock_put(sk);
+		return -ENOENT;
+	}
+
+	return sock_diag_destroy(sk, ECONNABORTED);
+}
+
+static int udp_diag_destroy(struct sk_buff *in_skb,
+			    const struct inet_diag_req_v2 *req)
+{
+	return __udp_diag_destroy(in_skb, req, &udp_table);
+}
+
+static int udplite_diag_destroy(struct sk_buff *in_skb,
+				const struct inet_diag_req_v2 *req)
+{
+	return __udp_diag_destroy(in_skb, req, &udplite_table);
+}
+
+#endif
+
 static const struct inet_diag_handler udp_diag_handler = {
 	.dump		 = udp_diag_dump,
 	.dump_one	 = udp_diag_dump_one,
 	.idiag_get_info  = udp_diag_get_info,
 	.idiag_type	 = IPPROTO_UDP,
 	.idiag_info_size = 0,
+#ifdef CONFIG_INET_DIAG_DESTROY
+	.destroy	 = udp_diag_destroy,
+#endif
 };
 
 static void udplite_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
@@ -192,6 +249,9 @@ static const struct inet_diag_handler udplite_diag_handler = {
 	.idiag_get_info  = udp_diag_get_info,
 	.idiag_type	 = IPPROTO_UDPLITE,
 	.idiag_info_size = 0,
+#ifdef CONFIG_INET_DIAG_DESTROY
+	.destroy	 = udplite_diag_destroy,
+#endif
 };
 
 static int __init udp_diag_init(void)
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 81e2f98b958d..16512cf06e73 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1467,6 +1467,7 @@ struct proto udpv6_prot = {
 	.compat_getsockopt = compat_udpv6_getsockopt,
 #endif
 	.clear_sk	   = udp_v6_clear_sk,
+	.diag_destroy      = udp_abort,
 };
 
 static struct inet_protosw udpv6_protosw = {
-- 
2.1.4

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

[Eric Dumazet]

CC Lorenzo Colitti <lorenzo@google.com>

On Tue, 2016-08-23 at 08:02 -0700, David Ahern wrote:
> This implements SOCK_DESTROY for UDP sockets similar to what was done
> for TCP with commit c1e64e298b8ca ("net: diag: Support destroying TCP
> sockets.") A process with a UDP socket targeted for destroy is awakened
> and recvmsg fails with ECONNABORTED.
> 
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
> ---
> v3
> - add missing sock_diag_check_cookie check after socket lookup per
>   Eric'c comment
> 
> v2
> - changed socket lookup to __udp{4,6}_lib_lookup
> 
>  include/net/udp.h   |  1 +
>  net/ipv4/udp.c      | 17 +++++++++++++++
>  net/ipv4/udp_diag.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  net/ipv6/udp.c      |  1 +
>  4 files changed, 79 insertions(+)
> 
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 8894d7144189..ea53a87d880f 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -251,6 +251,7 @@ int udp_get_port(struct sock *sk, unsigned short snum,
>  		 int (*saddr_cmp)(const struct sock *,
>  				  const struct sock *));
>  void udp_err(struct sk_buff *, u32);
> +int udp_abort(struct sock *sk, int err);
>  int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len);
>  int udp_push_pending_frames(struct sock *sk);
>  void udp_flush_pending_frames(struct sock *sk);
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 8f5f7f6026f7..044058835ea8 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2193,6 +2193,22 @@ unsigned int udp_poll(struct file *file, struct socket *sock, poll_table *wait)
>  }
>  EXPORT_SYMBOL(udp_poll);
>  
> +int udp_abort(struct sock *sk, int err)
> +{
> +	lock_sock(sk);
> +
> +	sk->sk_err = err;
> +	sk->sk_error_report(sk);
> +	udp_disconnect(sk, 0);
> +
> +	release_sock(sk);
> +
> +	sock_put(sk);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(udp_abort);
> +
>  struct proto udp_prot = {
>  	.name		   = "UDP",
>  	.owner		   = THIS_MODULE,
> @@ -2224,6 +2240,7 @@ struct proto udp_prot = {
>  	.compat_getsockopt = compat_udp_getsockopt,
>  #endif
>  	.clear_sk	   = sk_prot_clear_portaddr_nulls,
> +	.diag_destroy	   = udp_abort,
>  };
>  EXPORT_SYMBOL(udp_prot);
>  
> diff --git a/net/ipv4/udp_diag.c b/net/ipv4/udp_diag.c
> index 3d5ccf4b1412..63896dd766f3 100644
> --- a/net/ipv4/udp_diag.c
> +++ b/net/ipv4/udp_diag.c
> @@ -165,12 +165,69 @@ static void udp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
>  	r->idiag_wqueue = sk_wmem_alloc_get(sk);
>  }
>  
> +#ifdef CONFIG_INET_DIAG_DESTROY
> +static int __udp_diag_destroy(struct sk_buff *in_skb,
> +			      const struct inet_diag_req_v2 *req,
> +			      struct udp_table *tbl)
> +{
> +	struct net *net = sock_net(in_skb->sk);
> +	struct sock *sk;
> +
> +	rcu_read_lock();
> +
> +	if (req->sdiag_family == AF_INET)
> +		sk = __udp4_lib_lookup(net,
> +				req->id.idiag_dst[0], req->id.idiag_dport,
> +				req->id.idiag_src[0], req->id.idiag_sport,
> +				req->id.idiag_if, tbl, NULL);
> +#if IS_ENABLED(CONFIG_IPV6)
> +	else if (req->sdiag_family == AF_INET6)
> +		sk = __udp6_lib_lookup(net,
> +				(struct in6_addr *)req->id.idiag_dst,
> +				req->id.idiag_dport,
> +				(struct in6_addr *)req->id.idiag_src,
> +				req->id.idiag_sport,
> +				req->id.idiag_if, tbl, NULL);
> +#endif
> +	if (sk && !atomic_inc_not_zero(&sk->sk_refcnt))
> +		sk = NULL;
> +
> +	rcu_read_unlock();
> +
> +	if (!sk)
> +		return -ENOENT;
> +
> +	if (sock_diag_check_cookie(sk, req->id.idiag_cookie)) {
> +		sock_put(sk);
> +		return -ENOENT;
> +	}
> +
> +	return sock_diag_destroy(sk, ECONNABORTED);
> +}
> +
> +static int udp_diag_destroy(struct sk_buff *in_skb,
> +			    const struct inet_diag_req_v2 *req)
> +{
> +	return __udp_diag_destroy(in_skb, req, &udp_table);
> +}
> +
> +static int udplite_diag_destroy(struct sk_buff *in_skb,
> +				const struct inet_diag_req_v2 *req)
> +{
> +	return __udp_diag_destroy(in_skb, req, &udplite_table);
> +}
> +
> +#endif
> +
>  static const struct inet_diag_handler udp_diag_handler = {
>  	.dump		 = udp_diag_dump,
>  	.dump_one	 = udp_diag_dump_one,
>  	.idiag_get_info  = udp_diag_get_info,
>  	.idiag_type	 = IPPROTO_UDP,
>  	.idiag_info_size = 0,
> +#ifdef CONFIG_INET_DIAG_DESTROY
> +	.destroy	 = udp_diag_destroy,
> +#endif
>  };
>  
>  static void udplite_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
> @@ -192,6 +249,9 @@ static const struct inet_diag_handler udplite_diag_handler = {
>  	.idiag_get_info  = udp_diag_get_info,
>  	.idiag_type	 = IPPROTO_UDPLITE,
>  	.idiag_info_size = 0,
> +#ifdef CONFIG_INET_DIAG_DESTROY
> +	.destroy	 = udplite_diag_destroy,
> +#endif
>  };
>  
>  static int __init udp_diag_init(void)
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 81e2f98b958d..16512cf06e73 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1467,6 +1467,7 @@ struct proto udpv6_prot = {
>  	.compat_getsockopt = compat_udpv6_getsockopt,
>  #endif
>  	.clear_sk	   = udp_v6_clear_sk,
> +	.diag_destroy      = udp_abort,
>  };
>  
>  static struct inet_protosw udpv6_protosw = {

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

[Eric Dumazet]

On Tue, 2016-08-23 at 08:02 -0700, David Ahern wrote:
> This implements SOCK_DESTROY for UDP sockets similar to what was done
> for TCP with commit c1e64e298b8ca ("net: diag: Support destroying TCP
> sockets.") A process with a UDP socket targeted for destroy is awakened
> and recvmsg fails with ECONNABORTED.
> 
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
> ---
> v3
> - add missing sock_diag_check_cookie check after socket lookup per
>   Eric'c comment
> 
> v2
> - changed socket lookup to __udp{4,6}_lib_lookup
> 
>  include/net/udp.h   |  1 +
>  net/ipv4/udp.c      | 17 +++++++++++++++
>  net/ipv4/udp_diag.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  net/ipv6/udp.c      |  1 +
>  4 files changed, 79 insertions(+)
> 
> diff --git a/include/net/udp.h b/include/net/udp.h
> index 8894d7144189..ea53a87d880f 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -251,6 +251,7 @@ int udp_get_port(struct sock *sk, unsigned short snum,
>  		 int (*saddr_cmp)(const struct sock *,
>  				  const struct sock *));
>  void udp_err(struct sk_buff *, u32);
> +int udp_abort(struct sock *sk, int err);
>  int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len);
>  int udp_push_pending_frames(struct sock *sk);
>  void udp_flush_pending_frames(struct sock *sk);
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 8f5f7f6026f7..044058835ea8 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2193,6 +2193,22 @@ unsigned int udp_poll(struct file *file, struct socket *sock, poll_table *wait)
>  }
>  EXPORT_SYMBOL(udp_poll);
>  
> +int udp_abort(struct sock *sk, int err)
> +{
> +	lock_sock(sk);
> +
> +	sk->sk_err = err;
> +	sk->sk_error_report(sk);
> +	udp_disconnect(sk, 0);
> +
> +	release_sock(sk);
> +
> +	sock_put(sk);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(udp_abort);
> +
>  struct proto udp_prot = {
>  	.name		   = "UDP",
>  	.owner		   = THIS_MODULE,
> @@ -2224,6 +2240,7 @@ struct proto udp_prot = {
>  	.compat_getsockopt = compat_udp_getsockopt,
>  #endif
>  	.clear_sk	   = sk_prot_clear_portaddr_nulls,
> +	.diag_destroy	   = udp_abort,
>  };
>  EXPORT_SYMBOL(udp_prot);
>  
> diff --git a/net/ipv4/udp_diag.c b/net/ipv4/udp_diag.c
> index 3d5ccf4b1412..63896dd766f3 100644
> --- a/net/ipv4/udp_diag.c
> +++ b/net/ipv4/udp_diag.c
> @@ -165,12 +165,69 @@ static void udp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
>  	r->idiag_wqueue = sk_wmem_alloc_get(sk);
>  }
>  
> +#ifdef CONFIG_INET_DIAG_DESTROY
> +static int __udp_diag_destroy(struct sk_buff *in_skb,
> +			      const struct inet_diag_req_v2 *req,
> +			      struct udp_table *tbl)
> +{
> +	struct net *net = sock_net(in_skb->sk);
> +	struct sock *sk;
> +
> +	rcu_read_lock();
> +
> +	if (req->sdiag_family == AF_INET)
> +		sk = __udp4_lib_lookup(net,
> +				req->id.idiag_dst[0], req->id.idiag_dport,
> +				req->id.idiag_src[0], req->id.idiag_sport,
> +				req->id.idiag_if, tbl, NULL);
> +#if IS_ENABLED(CONFIG_IPV6)
> +	else if (req->sdiag_family == AF_INET6)
> +		sk = __udp6_lib_lookup(net,
> +				(struct in6_addr *)req->id.idiag_dst,
> +				req->id.idiag_dport,
> +				(struct in6_addr *)req->id.idiag_src,
> +				req->id.idiag_sport,
> +				req->id.idiag_if, tbl, NULL);
> +#endif
> +	if (sk && !atomic_inc_not_zero(&sk->sk_refcnt))
> +		sk = NULL;
> +
> +	rcu_read_unlock();
> +
> +	if (!sk)
> +		return -ENOENT;
> +
> +	if (sock_diag_check_cookie(sk, req->id.idiag_cookie)) {
> +		sock_put(sk);
> +		return -ENOENT;
> +	}
> +
> +	return sock_diag_destroy(sk, ECONNABORTED);
> +}
> +
> +static int udp_diag_destroy(struct sk_buff *in_skb,
> +			    const struct inet_diag_req_v2 *req)
> +{
> +	return __udp_diag_destroy(in_skb, req, &udp_table);
> +}
> +
> +static int udplite_diag_destroy(struct sk_buff *in_skb,
> +				const struct inet_diag_req_v2 *req)
> +{
> +	return __udp_diag_destroy(in_skb, req, &udplite_table);
> +}
> +
> +#endif
> +
>  static const struct inet_diag_handler udp_diag_handler = {
>  	.dump		 = udp_diag_dump,
>  	.dump_one	 = udp_diag_dump_one,
>  	.idiag_get_info  = udp_diag_get_info,
>  	.idiag_type	 = IPPROTO_UDP,
>  	.idiag_info_size = 0,
> +#ifdef CONFIG_INET_DIAG_DESTROY
> +	.destroy	 = udp_diag_destroy,
> +#endif
>  };
>  
>  static void udplite_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
> @@ -192,6 +249,9 @@ static const struct inet_diag_handler udplite_diag_handler = {
>  	.idiag_get_info  = udp_diag_get_info,
>  	.idiag_type	 = IPPROTO_UDPLITE,
>  	.idiag_info_size = 0,
> +#ifdef CONFIG_INET_DIAG_DESTROY
> +	.destroy	 = udplite_diag_destroy,
> +#endif
>  };
>  
>  static int __init udp_diag_init(void)
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 81e2f98b958d..16512cf06e73 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1467,6 +1467,7 @@ struct proto udpv6_prot = {
>  	.compat_getsockopt = compat_udpv6_getsockopt,
>  #endif
>  	.clear_sk	   = udp_v6_clear_sk,
> +	.diag_destroy      = udp_abort,
>  };
>  
>  static struct inet_protosw udpv6_protosw = {

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

[Lorenzo Colitti]

On Wed, Aug 24, 2016 at 12:02 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> +int udp_abort(struct sock *sk, int err)
> +{
> +       lock_sock(sk);
> +
> +       sk->sk_err = err;
> +       sk->sk_error_report(sk);
> +       udp_disconnect(sk, 0);

I notice that udp_disconnect does "inet->inet_daddr = 0" but doesn't
touch sk_v6_daddr. I wonder if that's correct. Even if it isn't,
that's likely not specific to this patch, since udpv6_prot specifies
udp_disconnect as its disconnect function pointer.

> +       if (req->sdiag_family == AF_INET)
> +               sk = __udp4_lib_lookup(net,
> +                               req->id.idiag_dst[0], req->id.idiag_dport,
> +                               req->id.idiag_src[0], req->id.idiag_sport,
> +                               req->id.idiag_if, tbl, NULL);
> +#if IS_ENABLED(CONFIG_IPV6)
> +       else if (req->sdiag_family == AF_INET6)
> +               sk = __udp6_lib_lookup(net,

I think you need to check for mapped addresses like Eric added to
inet_diag_find_one_icsk in 7c1306723 .

> +                               (struct in6_addr *)req->id.idiag_dst,
> +                               req->id.idiag_dport,
> +                               (struct in6_addr *)req->id.idiag_src,
> +                               req->id.idiag_sport,
> +                               req->id.idiag_if, tbl, NULL);
> +#endif

If you want to be consistent with what the TCP code does, return
-EINVAL here instead of -ENOENT if an invalid address family was
passed in (e.g., if user passes in AF_INET6 and IPv6 is not compiled
in).

> +       if (sock_diag_check_cookie(sk, req->id.idiag_cookie)) {
> +               sock_put(sk);
> +               return -ENOENT;
> +       }
> +
> +       return sock_diag_destroy(sk, ECONNABORTED);

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.

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

[David Ahern]

On 8/23/16 10:37 AM, Lorenzo Colitti wrote:
> On Wed, Aug 24, 2016 at 12:02 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
>> +int udp_abort(struct sock *sk, int err)
>> +{
>> +       lock_sock(sk);
>> +
>> +       sk->sk_err = err;
>> +       sk->sk_error_report(sk);
>> +       udp_disconnect(sk, 0);
> 
> I notice that udp_disconnect does "inet->inet_daddr = 0" but doesn't
> touch sk_v6_daddr. I wonder if that's correct. Even if it isn't,
> that's likely not specific to this patch, since udpv6_prot specifies
> udp_disconnect as its disconnect function pointer.

ack. can send a separate patch for that one.

> 
>> +       if (req->sdiag_family == AF_INET)
>> +               sk = __udp4_lib_lookup(net,
>> +                               req->id.idiag_dst[0], req->id.idiag_dport,
>> +                               req->id.idiag_src[0], req->id.idiag_sport,
>> +                               req->id.idiag_if, tbl, NULL);
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +       else if (req->sdiag_family == AF_INET6)
>> +               sk = __udp6_lib_lookup(net,
> 
> I think you need to check for mapped addresses like Eric added to
> inet_diag_find_one_icsk in 7c1306723 .
> 
>> +                               (struct in6_addr *)req->id.idiag_dst,
>> +                               req->id.idiag_dport,
>> +                               (struct in6_addr *)req->id.idiag_src,
>> +                               req->id.idiag_sport,
>> +                               req->id.idiag_if, tbl, NULL);
>> +#endif
> 
> If you want to be consistent with what the TCP code does, return
> -EINVAL here instead of -ENOENT if an invalid address family was
> passed in (e.g., if user passes in AF_INET6 and IPv6 is not compiled
> in).

ok, I'll make this lookup mirror inet_diag_find_one_icsk. That handles these 2 comments.

> 
>> +       if (sock_diag_check_cookie(sk, req->id.idiag_cookie)) {
>> +               sock_put(sk);
>> +               return -ENOENT;
>> +       }
>> +
>> +       return sock_diag_destroy(sk, ECONNABORTED);
> 
> 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. 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.

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

[Lorenzo Colitti]

On Wed, Aug 24, 2016 at 2:02 AM, David Ahern <dsa@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.

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 <dsa@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

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

[Lorenzo Colitti]

On Wed, Aug 24, 2016 at 2:16 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
>> 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:
...
>         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

That does seem nicer. I think sock_gen_put will work on all types of
TCP sockets, so sk_fullsock is not needed here.

Eric, any thoughts?

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

[Eric Dumazet]

On Wed, 2016-08-24 at 02:24 +0900, Lorenzo Colitti wrote:
> On Wed, Aug 24, 2016 at 2:16 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> >> 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:
> ...
> >         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
> 
> That does seem nicer. I think sock_gen_put will work on all types of
> TCP sockets, so sk_fullsock is not needed here.
> 
> Eric, any thoughts?

LGTM, and if (!sk_fullsock(sk)) is not needed.