[RFC PATCH net-next 0/2] net: extend INET_DIAG_INFO with information specific to TCP ULP

[RFC PATCH net-next 0/2] net: extend INET_DIAG_INFO with information specific to TCP ULP

[Davide Caratti]

The current kernel does not provide any diagnostic tool, except
getsockopt(TCP_ULP), to know more about TCP sockets that have an upper
layer protocol (ULP) on top of them. This series extends the set of
information exported by INET_DIAG_INFO, to include data that are specific
to the ULP (and that might be meaningful for debug/testing purposes).

patch 1/2 extends INET_DIAG_INFO and allows knowing the ULP name for
each TCP socket that has done setsockopt(TCP_ULP) successfully.

kernel TLS is the only TCP ULP user at the moment: patch 2/2 extends kTLS
to let programs like 'ss' know the protocol version and the cipher in use.

Davide Caratti (2):
  tcp: ulp: add functions to dump ulp-specific information
  net: tls: export protocol version and cipher to socket diag

 include/net/tcp.h              |  3 +++
 include/uapi/linux/inet_diag.h |  9 +++++++
 include/uapi/linux/tls.h       |  8 +++++++
 net/ipv4/tcp_diag.c            | 34 +++++++++++++++++++++++++--
 net/tls/tls_main.c             | 43 ++++++++++++++++++++++++++++++++++
 5 files changed, 95 insertions(+), 2 deletions(-)

-- 
2.20.1

[RFC PATCH net-next 1/2] tcp: ulp: add functions to dump ulp-specific information

[Davide Caratti]

currently, only getsockopt(TCP_ULP) can be invoked to know if a ULP is on
top of a TCP socket. Extend idiag_get_aux() and idiag_get_aux_size(),
introduced by commit b37e88407c1d ("inet_diag: allow protocols to provide
additional data"), to report the ULP name and other information that can
be made available by the ULP through optional functions.

Users having CAP_NET_ADMIN privileges will then be able to retrieve this
information through inet_diag_handler, if they specify INET_DIAG_INFO in
the request.

Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 include/net/tcp.h              |  3 +++
 include/uapi/linux/inet_diag.h |  8 ++++++++
 net/ipv4/tcp_diag.c            | 34 ++++++++++++++++++++++++++++++++--
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 0083a14fb64f..94431562c4b4 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2108,6 +2108,9 @@ struct tcp_ulp_ops {
 	int (*init)(struct sock *sk);
 	/* cleanup ulp */
 	void (*release)(struct sock *sk);
+	/* diagnostic */
+	int (*get_info)(struct sock *sk, struct sk_buff *skb);
+	size_t (*get_info_size)(struct sock *sk);
 
 	char		name[TCP_ULP_NAME_MAX];
 	struct module	*owner;
diff --git a/include/uapi/linux/inet_diag.h b/include/uapi/linux/inet_diag.h
index e8baca85bac6..844133de3212 100644
--- a/include/uapi/linux/inet_diag.h
+++ b/include/uapi/linux/inet_diag.h
@@ -153,11 +153,19 @@ enum {
 	INET_DIAG_BBRINFO,	/* request as INET_DIAG_VEGASINFO */
 	INET_DIAG_CLASS_ID,	/* request as INET_DIAG_TCLASS */
 	INET_DIAG_MD5SIG,
+	INET_DIAG_ULP_INFO,
 	__INET_DIAG_MAX,
 };
 
 #define INET_DIAG_MAX (__INET_DIAG_MAX - 1)
 
+enum {
+	ULP_INFO_NAME,
+	__ULP_INFO_MAX,
+};
+
+#define ULP_INFO_MAX (__ULP_INFO_MAX - 1)
+
 /* INET_DIAG_MEM */
 
 struct inet_diag_meminfo {
diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
index 81148f7a2323..de2e9e75b8e0 100644
--- a/net/ipv4/tcp_diag.c
+++ b/net/ipv4/tcp_diag.c
@@ -88,10 +88,12 @@ static int tcp_diag_put_md5sig(struct sk_buff *skb,
 static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
 			    struct sk_buff *skb)
 {
+	struct inet_connection_sock *icsk = inet_csk(sk);
+	int err = 0;
+
 #ifdef CONFIG_TCP_MD5SIG
 	if (net_admin) {
 		struct tcp_md5sig_info *md5sig;
-		int err = 0;
 
 		rcu_read_lock();
 		md5sig = rcu_dereference(tcp_sk(sk)->md5sig_info);
@@ -103,11 +105,33 @@ static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
 	}
 #endif
 
-	return 0;
+	if (net_admin && icsk->icsk_ulp_ops) {
+		struct nlattr *nest;
+
+		nest = nla_nest_start_noflag(skb, INET_DIAG_ULP_INFO);
+		if (!nest) {
+			err = -EMSGSIZE;
+			goto nla_failure;
+		}
+		err = nla_put_string(skb, ULP_INFO_NAME,
+				     icsk->icsk_ulp_ops->name);
+		if (err < 0)
+			goto nla_failure;
+		if (icsk->icsk_ulp_ops->get_info)
+			err = icsk->icsk_ulp_ops->get_info(sk, skb);
+		if (err < 0) {
+nla_failure:
+			nla_nest_cancel(skb, nest);
+			return err;
+		}
+		nla_nest_end(skb, nest);
+	}
+	return err;
 }
 
 static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
 {
+	struct inet_connection_sock *icsk = inet_csk(sk);
 	size_t size = 0;
 
 #ifdef CONFIG_TCP_MD5SIG
@@ -128,6 +152,12 @@ static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
 	}
 #endif
 
+	if (net_admin && icsk->icsk_ulp_ops) {
+		size +=   nla_total_size(0) /* INET_DIAG_ULP_INFO */
+			+ nla_total_size(TCP_ULP_NAME_MAX); /* ULP_INFO_NAME */
+		if (icsk->icsk_ulp_ops->get_info_size)
+			size += icsk->icsk_ulp_ops->get_info_size(sk);
+	}
 	return size;
 }
 
-- 
2.20.1

[RFC PATCH net-next 2/2] net: tls: export protocol version and cipher to socket diag

[Davide Caratti]

When an application configures kernel TLS on top of a TCP socket, it's
now possible for inet_diag_handler to collect information regarding the
protocol version and the cipher, in case INET_DIAG_INFO is requested.

Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 include/uapi/linux/inet_diag.h |  1 +
 include/uapi/linux/tls.h       |  8 +++++++
 net/tls/tls_main.c             | 43 ++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/include/uapi/linux/inet_diag.h b/include/uapi/linux/inet_diag.h
index 844133de3212..92208535c096 100644
--- a/include/uapi/linux/inet_diag.h
+++ b/include/uapi/linux/inet_diag.h
@@ -161,6 +161,7 @@ enum {
 
 enum {
 	ULP_INFO_NAME,
+	ULP_INFO_TLS,
 	__ULP_INFO_MAX,
 };
 
diff --git a/include/uapi/linux/tls.h b/include/uapi/linux/tls.h
index 5b9c26753e46..442348bd2e54 100644
--- a/include/uapi/linux/tls.h
+++ b/include/uapi/linux/tls.h
@@ -109,4 +109,12 @@ struct tls12_crypto_info_aes_ccm_128 {
 	unsigned char rec_seq[TLS_CIPHER_AES_CCM_128_REC_SEQ_SIZE];
 };
 
+enum {
+	TLS_INFO_VERSION,
+	TLS_INFO_CIPHER,
+	__TLS_INFO_MAX,
+};
+
+#define TLS_INFO_MAX (__TLS_INFO_MAX - 1)
+
 #endif /* _UAPI_LINUX_TLS_H */
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index fc81ae18cc44..14597526981c 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -39,6 +39,7 @@
 #include <linux/netdevice.h>
 #include <linux/sched/signal.h>
 #include <linux/inetdevice.h>
+#include <linux/inet_diag.h>
 
 #include <net/tls.h>
 
@@ -798,6 +799,46 @@ static int tls_init(struct sock *sk)
 	return rc;
 }
 
+static int tls_get_info(struct sock *sk, struct sk_buff *skb)
+{
+	struct tls_context *ctx = tls_get_ctx(sk);
+	struct nlattr *start = 0;
+	int err = 0;
+
+	if (sk->sk_state != TCP_ESTABLISHED)
+		goto end;
+	start = nla_nest_start_noflag(skb, ULP_INFO_TLS);
+	if (!start) {
+		err = -EMSGSIZE;
+		goto nla_failure;
+	}
+	err = nla_put_u16(skb, TLS_INFO_VERSION, ctx->prot_info.version);
+	if (err < 0)
+		goto nla_failure;
+	err = nla_put_u16(skb, TLS_INFO_CIPHER, ctx->prot_info.cipher_type);
+	if (err < 0)
+		goto nla_failure;
+	nla_nest_end(skb, start);
+end:
+	return err;
+nla_failure:
+	nla_nest_cancel(skb, start);
+	goto end;
+}
+
+static size_t tls_get_info_size(struct sock *sk)
+{
+	size_t size = 0;
+
+	if (sk->sk_state != TCP_ESTABLISHED)
+		return size;
+
+	size +=   nla_total_size(0) /* ULP_INFO_TLS */
+		+ nla_total_size(sizeof(__u16))	/* TLS_INFO_VERSION */
+		+ nla_total_size(sizeof(__u16)); /* TLS_INFO_CIPHER */
+	return size;
+}
+
 void tls_register_device(struct tls_device *device)
 {
 	spin_lock_bh(&device_spinlock);
@@ -818,6 +859,8 @@ static struct tcp_ulp_ops tcp_tls_ulp_ops __read_mostly = {
 	.name			= "tls",
 	.owner			= THIS_MODULE,
 	.init			= tls_init,
+	.get_info		= tls_get_info,
+	.get_info_size		= tls_get_info_size,
 };
 
 static int __init tls_register(void)
-- 
2.20.1

Re: [RFC PATCH net-next 1/2] tcp: ulp: add functions to dump ulp-specific information

[Jakub Kicinski]

On Wed,  5 Jun 2019 17:39:22 +0200, Davide Caratti wrote:
> currently, only getsockopt(TCP_ULP) can be invoked to know if a ULP is on
> top of a TCP socket. Extend idiag_get_aux() and idiag_get_aux_size(),
> introduced by commit b37e88407c1d ("inet_diag: allow protocols to provide
> additional data"), to report the ULP name and other information that can
> be made available by the ULP through optional functions.
> 
> Users having CAP_NET_ADMIN privileges will then be able to retrieve this
> information through inet_diag_handler, if they specify INET_DIAG_INFO in
> the request.
> 
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
>  include/net/tcp.h              |  3 +++
>  include/uapi/linux/inet_diag.h |  8 ++++++++
>  net/ipv4/tcp_diag.c            | 34 ++++++++++++++++++++++++++++++++--
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 0083a14fb64f..94431562c4b4 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2108,6 +2108,9 @@ struct tcp_ulp_ops {
>  	int (*init)(struct sock *sk);
>  	/* cleanup ulp */
>  	void (*release)(struct sock *sk);
> +	/* diagnostic */
> +	int (*get_info)(struct sock *sk, struct sk_buff *skb);
> +	size_t (*get_info_size)(struct sock *sk);
>  
>  	char		name[TCP_ULP_NAME_MAX];
>  	struct module	*owner;
> diff --git a/include/uapi/linux/inet_diag.h b/include/uapi/linux/inet_diag.h
> index e8baca85bac6..844133de3212 100644
> --- a/include/uapi/linux/inet_diag.h
> +++ b/include/uapi/linux/inet_diag.h
> @@ -153,11 +153,19 @@ enum {
>  	INET_DIAG_BBRINFO,	/* request as INET_DIAG_VEGASINFO */
>  	INET_DIAG_CLASS_ID,	/* request as INET_DIAG_TCLASS */
>  	INET_DIAG_MD5SIG,
> +	INET_DIAG_ULP_INFO,
>  	__INET_DIAG_MAX,
>  };
>  
>  #define INET_DIAG_MAX (__INET_DIAG_MAX - 1)
>  
> +enum {

Value of 0 is commonly defined as UNSPEC (or NONE), so:

	ULP_UNSPEC,

here.  Also perhaps INET_ULP_..?

> +	ULP_INFO_NAME,
> +	__ULP_INFO_MAX,
> +};
> +
> +#define ULP_INFO_MAX (__ULP_INFO_MAX - 1)
> +
>  /* INET_DIAG_MEM */
>  
>  struct inet_diag_meminfo {
> diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
> index 81148f7a2323..de2e9e75b8e0 100644
> --- a/net/ipv4/tcp_diag.c
> +++ b/net/ipv4/tcp_diag.c
> @@ -88,10 +88,12 @@ static int tcp_diag_put_md5sig(struct sk_buff *skb,
>  static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
>  			    struct sk_buff *skb)
>  {
> +	struct inet_connection_sock *icsk = inet_csk(sk);
> +	int err = 0;
> +
>  #ifdef CONFIG_TCP_MD5SIG
>  	if (net_admin) {
>  		struct tcp_md5sig_info *md5sig;
> -		int err = 0;
>  
>  		rcu_read_lock();
>  		md5sig = rcu_dereference(tcp_sk(sk)->md5sig_info);
> @@ -103,11 +105,33 @@ static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
>  	}
>  #endif
>  
> -	return 0;
> +	if (net_admin && icsk->icsk_ulp_ops) {
> +		struct nlattr *nest;
> +
> +		nest = nla_nest_start_noflag(skb, INET_DIAG_ULP_INFO);
> +		if (!nest) {
> +			err = -EMSGSIZE;
> +			goto nla_failure;
> +		}
> +		err = nla_put_string(skb, ULP_INFO_NAME,
> +				     icsk->icsk_ulp_ops->name);
> +		if (err < 0)

nit: nla_put_string() does not return positive non-zero codes

> +			goto nla_failure;
> +		if (icsk->icsk_ulp_ops->get_info)
> +			err = icsk->icsk_ulp_ops->get_info(sk, skb);

And neither should this, probably.

> +		if (err < 0) {
> +nla_failure:
> +			nla_nest_cancel(skb, nest);
> +			return err;
> +		}
> +		nla_nest_end(skb, nest);
> +	}
> +	return err;

So just return 0 here.

>  }
>  
>  static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
>  {
> +	struct inet_connection_sock *icsk = inet_csk(sk);
>  	size_t size = 0;
>  
>  #ifdef CONFIG_TCP_MD5SIG
> @@ -128,6 +152,12 @@ static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
>  	}
>  #endif
>  
> +	if (net_admin && icsk->icsk_ulp_ops) {
> +		size +=   nla_total_size(0) /* INET_DIAG_ULP_INFO */

                       ^^^ not sure we want those multiple spaces here.

> +			+ nla_total_size(TCP_ULP_NAME_MAX); /* ULP_INFO_NAME */

+ usually goes at the end of previous line

> +		if (icsk->icsk_ulp_ops->get_info_size)
> +			size += icsk->icsk_ulp_ops->get_info_size(sk);

I don't know the diag code, is the socket locked at this point?

> +	}
>  	return size;
>  }
>  

Re: [RFC PATCH net-next 2/2] net: tls: export protocol version and cipher to socket diag

[Jakub Kicinski]

On Wed,  5 Jun 2019 17:39:23 +0200, Davide Caratti wrote:
> When an application configures kernel TLS on top of a TCP socket, it's
> now possible for inet_diag_handler to collect information regarding the
> protocol version and the cipher, in case INET_DIAG_INFO is requested.
> 
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
>  include/uapi/linux/inet_diag.h |  1 +
>  include/uapi/linux/tls.h       |  8 +++++++
>  net/tls/tls_main.c             | 43 ++++++++++++++++++++++++++++++++++
>  3 files changed, 52 insertions(+)
> 
> diff --git a/include/uapi/linux/inet_diag.h b/include/uapi/linux/inet_diag.h
> index 844133de3212..92208535c096 100644
> --- a/include/uapi/linux/inet_diag.h
> +++ b/include/uapi/linux/inet_diag.h
> @@ -161,6 +161,7 @@ enum {
>  
>  enum {
>  	ULP_INFO_NAME,
> +	ULP_INFO_TLS,
>  	__ULP_INFO_MAX,
>  };
>  
> diff --git a/include/uapi/linux/tls.h b/include/uapi/linux/tls.h
> index 5b9c26753e46..442348bd2e54 100644
> --- a/include/uapi/linux/tls.h
> +++ b/include/uapi/linux/tls.h
> @@ -109,4 +109,12 @@ struct tls12_crypto_info_aes_ccm_128 {
>  	unsigned char rec_seq[TLS_CIPHER_AES_CCM_128_REC_SEQ_SIZE];
>  };
>  
> +enum {

USPEC

> +	TLS_INFO_VERSION,
> +	TLS_INFO_CIPHER,

We need some indication of the directions in which kTLS is active
(none, rx, tx, rx/tx).

Also perhaps could you add TLS_SW vs TLS_HW etc. ? :)

> +	__TLS_INFO_MAX,
> +};
> +

Traditionally we put no new line between enum and the max define.

> +#define TLS_INFO_MAX (__TLS_INFO_MAX - 1)
> +
>  #endif /* _UAPI_LINUX_TLS_H */
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index fc81ae18cc44..14597526981c 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -39,6 +39,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/sched/signal.h>
>  #include <linux/inetdevice.h>
> +#include <linux/inet_diag.h>
> 
>  #include <net/tls.h>
>  
> @@ -798,6 +799,46 @@ static int tls_init(struct sock *sk)
>  	return rc;
>  }
>  
> +static int tls_get_info(struct sock *sk, struct sk_buff *skb)
> +{
> +	struct tls_context *ctx = tls_get_ctx(sk);
> +	struct nlattr *start = 0;

Hm.. NULL?  Does this not give you a warning?

> +	int err = 0;

There should be no need to init this.

> +	if (sk->sk_state != TCP_ESTABLISHED)

Hmm.. why this check?  We never clean up the state once installed until
the socket dies completely (currently, pending John's unhash work).

> +		goto end;

Please don't do this, just return 0; here.

> +	start = nla_nest_start_noflag(skb, ULP_INFO_TLS);
> +	if (!start) {
> +		err = -EMSGSIZE;
> +		goto nla_failure;

		return -EMSGSIZE;

> +	}
> +	err = nla_put_u16(skb, TLS_INFO_VERSION, ctx->prot_info.version);
> +	if (err < 0)
> +		goto nla_failure;
> +	err = nla_put_u16(skb, TLS_INFO_CIPHER, ctx->prot_info.cipher_type);
> +	if (err < 0)
> +		goto nla_failure;
> +	nla_nest_end(skb, start);
> +end:
> +	return err;

	return 0;

> +nla_failure:
> +	nla_nest_cancel(skb, start);
> +	goto end;

	return err;

> +}
> +
> +static size_t tls_get_info_size(struct sock *sk)
> +{
> +	size_t size = 0;
> +
> +	if (sk->sk_state != TCP_ESTABLISHED)
> +		return size;
> +
> +	size +=   nla_total_size(0) /* ULP_INFO_TLS */
> +		+ nla_total_size(sizeof(__u16))	/* TLS_INFO_VERSION */
> +		+ nla_total_size(sizeof(__u16)); /* TLS_INFO_CIPHER */
> +	return size;
> +}

Same comments as on patch 1 and above.

>  void tls_register_device(struct tls_device *device)
>  {
>  	spin_lock_bh(&device_spinlock);

Thanks for working on this, it was on my todo list! :)

Re: [RFC PATCH net-next 2/2] net: tls: export protocol version and cipher to socket diag

[Boris Pismenny]

Hi Davide,

TLS statistics are long overdue. I'd like to extend this later for the 
tls_device code, e.g. device_decrypted vs. software_decrypted.

On 6/5/2019 6:39 PM, Davide Caratti wrote:

>   
> +static int tls_get_info(struct sock *sk, struct sk_buff *skb)
> +{
> +	struct tls_context *ctx = tls_get_ctx(sk);
> +	struct nlattr *start = 0;
> +	int err = 0;
> +
> +	if (sk->sk_state != TCP_ESTABLISHED)
> +		goto end;

Maybe it would be best to verify that the version and cipher have been 
initialized. As the TLS_ULP might be enabled but no socket option has 
been called to set its values.

Also, I suggest this check is placed in the tls_get_info_size to make 
this more explicit to the user.

> +	start = nla_nest_start_noflag(skb, ULP_INFO_TLS);
> +	if (!start) {
> +		err = -EMSGSIZE;
> +		goto nla_failure;
> +	}
> +	err = nla_put_u16(skb, TLS_INFO_VERSION, ctx->prot_info.version);
> +	if (err < 0)
> +		goto nla_failure;
> +	err = nla_put_u16(skb, TLS_INFO_CIPHER, ctx->prot_info.cipher_type);
> +	if (err < 0)
> +		goto nla_failure;
> +	nla_nest_end(skb, start);
> +end:
> +	return err;
> +nla_failure:
> +	nla_nest_cancel(skb, start);
> +	goto end;
> +}
> +
> +static size_t tls_get_info_size(struct sock *sk)
> +{
> +	size_t size = 0;
> +
> +	if (sk->sk_state != TCP_ESTABLISHED)
> +		return size;
> +
> +	size +=   nla_total_size(0) /* ULP_INFO_TLS */
> +		+ nla_total_size(sizeof(__u16))	/* TLS_INFO_VERSION */
> +		+ nla_total_size(sizeof(__u16)); /* TLS_INFO_CIPHER */
> +	return size;
> +}


Thanks,
Boris.

Re: [RFC PATCH net-next 1/2] tcp: ulp: add functions to dump ulp-specific information

[Davide Caratti]

On Wed, 2019-06-05 at 16:14 -0700, Jakub Kicinski wrote:
> On Wed,  5 Jun 2019 17:39:22 +0200, Davide Caratti wrote:
> > currently, only getsockopt(TCP_ULP) can be invoked to know if a ULP is on
> > top of a TCP socket. Extend idiag_get_aux() and idiag_get_aux_size(),
> > introduced by commit b37e88407c1d ("inet_diag: allow protocols to provide
> > additional data"), to report the ULP name and other information that can
> > be made available by the ULP through optional functions.
> > 
> > Users having CAP_NET_ADMIN privileges will then be able to retrieve this
> > information through inet_diag_handler, if they specify INET_DIAG_INFO in
> > the request.
> > 
> > Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> > ---
> >  include/net/tcp.h              |  3 +++
> >  include/uapi/linux/inet_diag.h |  8 ++++++++
> >  net/ipv4/tcp_diag.c            | 34 ++++++++++++++++++++++++++++++++--
> >  3 files changed, 43 insertions(+), 2 deletions(-)

hi Jakub, thanks a lot for reviewing!

[...]
> > --- a/include/uapi/linux/inet_diag.h
> > +++ b/include/uapi/linux/inet_diag.h
> > @@ -153,11 +153,19 @@ enum {
> >  	INET_DIAG_BBRINFO,	/* request as INET_DIAG_VEGASINFO */
> >  	INET_DIAG_CLASS_ID,	/* request as INET_DIAG_TCLASS */
> >  	INET_DIAG_MD5SIG,
> > +	INET_DIAG_ULP_INFO,
> >  	__INET_DIAG_MAX,
> >  };
> >  
> >  #define INET_DIAG_MAX (__INET_DIAG_MAX - 1)
> >  
> > +enum {
> 
> Value of 0 is commonly defined as UNSPEC (or NONE), so:
> 
> 	ULP_UNSPEC,
> 
> here.  Also perhaps INET_ULP_..?

ok, will fix that in patch v1.

> > +	ULP_INFO_NAME,
> > +	__ULP_INFO_MAX,
> > +};
> > +
> > +#define ULP_INFO_MAX (__ULP_INFO_MAX - 1)
> > +
> >  /* INET_DIAG_MEM */
> >  

[...]

> > @@ -103,11 +105,33 @@ static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
> >  	}
> >  #endif
> >  
> > -	return 0;
> > +	if (net_admin && icsk->icsk_ulp_ops) {
> > +		struct nlattr *nest;
> > +
> > +		nest = nla_nest_start_noflag(skb, INET_DIAG_ULP_INFO);
> > +		if (!nest) {
> > +			err = -EMSGSIZE;
> > +			goto nla_failure;
> > +		}
> > +		err = nla_put_string(skb, ULP_INFO_NAME,
> > +				     icsk->icsk_ulp_ops->name);
> > +		if (err < 0)
> 
> nit: nla_put_string() does not return positive non-zero codes

so, I will replace the test above with 

if (err)

> > +			goto nla_failure;
> > +		if (icsk->icsk_ulp_ops->get_info)
> > +			err = icsk->icsk_ulp_ops->get_info(sk, skb);
> 
> And neither should this, probably.

> > +		if (err < 0) {

same here.

> > +nla_failure:
> > +			nla_nest_cancel(skb, nest);
> > +			return err;
> > +		}
> > +		nla_nest_end(skb, nest);
> > +	}
> > +	return err;
> 
> So just return 0 here.

ok, I found comfortable with 'return err' because of the initialization at
the beginning of the function (this patch extends the scope of 'err' to
the whole function). But I'm not against return 0.

> >  }
> >  
> >  static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
> >  {
> > +	struct inet_connection_sock *icsk = inet_csk(sk);
> >  	size_t size = 0;
> >  
> >  #ifdef CONFIG_TCP_MD5SIG
> > @@ -128,6 +152,12 @@ static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
> >  	}
> >  #endif
> >  
> > +	if (net_admin && icsk->icsk_ulp_ops) {
> > +		size +=   nla_total_size(0) /* INET_DIAG_ULP_INFO */
> 
>                        ^^^ not sure we want those multiple spaces here.
> 
> > +			+ nla_total_size(TCP_ULP_NAME_MAX); /* ULP_INFO_NAME */
> 
> + usually goes at the end of previous line

I took the inspiration from the caller of .idiag_get_aux_size(). But you are right,
vxlan_get_size() has a better formatting: will fix that in patch v1.

> > +		if (icsk->icsk_ulp_ops->get_info_size)
> > +			size += icsk->icsk_ulp_ops->get_info_size(sk);
> 
> I don't know the diag code, is the socket locked at this point?

as far as I can see, it's not. Thanks a lot for noticing this!

anyway, I see a similar pattern for icsk_ca_ops: when we set the congestion
control with do_tcp_setsockopt(), the socket is locked - but then, when 'ss'
reads a diag request with INET_DIAG_CONG bit set, the value of icsk->icsk_ca_ops
is accessed with READ_ONCE(), surrounded by rcu_read_{,un}lock(). 

Maybe it's sufficient to do something similar, and then the socket lock can be
optionally taken within icsk_ulp_ops->get_info(), only in case we need to access
members of sk that are protected with the socket lock?

-- 
davide

Re: [RFC PATCH net-next 2/2] net: tls: export protocol version and cipher to socket diag

[Davide Caratti]

On Wed, 2019-06-05 at 16:25 -0700, Jakub Kicinski wrote:
> On Wed,  5 Jun 2019 17:39:23 +0200, Davide Caratti wrote:
> > When an application configures kernel TLS on top of a TCP socket, it's
> > now possible for inet_diag_handler to collect information regarding the
> > protocol version and the cipher, in case INET_DIAG_INFO is requested.
> > 
> > Signed-off-by: Davide Caratti <dcaratti@redhat.com>

> >  
> > +enum {
> 
> USPEC
> 
> > +	TLS_INFO_VERSION,
> > +	TLS_INFO_CIPHER,
> 

Ok,

> We need some indication of the directions in which kTLS is active
> (none, rx, tx, rx/tx).
> 
> Also perhaps could you add TLS_SW vs TLS_HW etc. ? :)

I can add a couple of u16 (or larger?) bitmasks to dump txconf and rxconf.
do you think this is sufficient?

> > +	__TLS_INFO_MAX,
> > +};
> > +

> Traditionally we put no new line between enum and the max define.

Ok, will fix that in v1.

> > +#define TLS_INFO_MAX (__TLS_INFO_MAX - 1)
> > +
> >  #endif /* _UAPI_LINUX_TLS_H */
> > diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> > index fc81ae18cc44..14597526981c 100644
> > --- a/net/tls/tls_main.c
> > +++ b/net/tls/tls_main.c
> > @@ -39,6 +39,7 @@
> >  #include <linux/netdevice.h>
> >  #include <linux/sched/signal.h>
> >  #include <linux/inetdevice.h>
> > +#include <linux/inet_diag.h>
> > 
> >  #include <net/tls.h>
> >  
> > @@ -798,6 +799,46 @@ static int tls_init(struct sock *sk)
> >  	return rc;
> >  }
> >  
> > +static int tls_get_info(struct sock *sk, struct sk_buff *skb)
> > +{
> > +	struct tls_context *ctx = tls_get_ctx(sk);
> > +	struct nlattr *start = 0;
> 
> Hm.. NULL?  Does this not give you a warning?

I didn't notice it, but sure. will fix in v1.

> > +	int err = 0;
> 
> There should be no need to init this.
> 
> > +	if (sk->sk_state != TCP_ESTABLISHED)
> 
> Hmm.. why this check?  We never clean up the state once installed until
> the socket dies completely (currently, pending John's unhash work).

the goal was to ensure that we don't read ctx anymore after
tls_sk_proto_close() has freed ctx, and I thought that a test on the value
of sk_state was sufficient.

If it's not, then we might invent something else. For example, we might
defer freeing kTLS ctx, so that it's called as the very last thing with
tcp_cleanup_ulp().
 
> > +		goto end;
> 
> Please don't do this, just return 0; here.
> 
> > +	start = nla_nest_start_noflag(skb, ULP_INFO_TLS);
> > +	if (!start) {
> > +		err = -EMSGSIZE;
> > +		goto nla_failure;
> 
> 		return -EMSGSIZE;
> 
> > +	}
> > +	err = nla_put_u16(skb, TLS_INFO_VERSION, ctx->prot_info.version);
> > +	if (err < 0)
> > +		goto nla_failure;
> > +	err = nla_put_u16(skb, TLS_INFO_CIPHER, ctx->prot_info.cipher_type);
> > +	if (err < 0)
> > +		goto nla_failure;
> > +	nla_nest_end(skb, start);
> > +end:
> > +	return err;
> 
> 	return 0;
> 
> > +nla_failure:
> > +	nla_nest_cancel(skb, start);
> > +	goto end;
> 
> 	return err;

Ok, i can remove that 'goto end'. 

> > +}
> > +
> > +static size_t tls_get_info_size(struct sock *sk)
> > +{
> > +	size_t size = 0;
> > +
> > +	if (sk->sk_state != TCP_ESTABLISHED)
> > +		return size;
> > +
> > +	size +=   nla_total_size(0) /* ULP_INFO_TLS */
> > +		+ nla_total_size(sizeof(__u16))	/* TLS_INFO_VERSION */
> > +		+ nla_total_size(sizeof(__u16)); /* TLS_INFO_CIPHER */
> > +	return size;
> > +}
> 
> Same comments as on patch 1 and above.

sure, ok.

> >  void tls_register_device(struct tls_device *device)
> >  {
> >  	spin_lock_bh(&device_spinlock);
> 
> Thanks for working on this, it was on my todo list! :)

thanks for the review!
-- 
davide

Re: [RFC PATCH net-next 1/2] tcp: ulp: add functions to dump ulp-specific information

[Jakub Kicinski]

On Mon, 17 Jun 2019 15:06:33 +0200, Davide Caratti wrote:
> > > +		if (icsk->icsk_ulp_ops->get_info_size)
> > > +			size += icsk->icsk_ulp_ops->get_info_size(sk);  
> > 
> > I don't know the diag code, is the socket locked at this point?  
> 
> as far as I can see, it's not. Thanks a lot for noticing this!
> 
> anyway, I see a similar pattern for icsk_ca_ops: when we set the congestion
> control with do_tcp_setsockopt(), the socket is locked - but then, when 'ss'
> reads a diag request with INET_DIAG_CONG bit set, the value of icsk->icsk_ca_ops
> is accessed with READ_ONCE(), surrounded by rcu_read_{,un}lock(). 
> 
> Maybe it's sufficient to do something similar, and then the socket lock can be
> optionally taken within icsk_ulp_ops->get_info(), only in case we need to access
> members of sk that are protected with the socket lock?

Sounds reasonable, we just need to keep that in mind as we extend TLS
code do dump more information.

Re: [RFC PATCH net-next 2/2] net: tls: export protocol version and cipher to socket diag

[Jakub Kicinski]

On Mon, 17 Jun 2019 18:04:06 +0200, Davide Caratti wrote:
> On Wed, 2019-06-05 at 16:25 -0700, Jakub Kicinski wrote:
> > On Wed,  5 Jun 2019 17:39:23 +0200, Davide Caratti wrote:  
> > We need some indication of the directions in which kTLS is active
> > (none, rx, tx, rx/tx).
> > 
> > Also perhaps could you add TLS_SW vs TLS_HW etc. ? :)  
> 
> I can add a couple of u16 (or larger?) bitmasks to dump txconf and rxconf.
> do you think this is sufficient?

SGTM!

> > > +	int err = 0;  
> > 
> > There should be no need to init this.
> >   
> > > +	if (sk->sk_state != TCP_ESTABLISHED)  
> > 
> > Hmm.. why this check?  We never clean up the state once installed until
> > the socket dies completely (currently, pending John's unhash work).  
> 
> the goal was to ensure that we don't read ctx anymore after
> tls_sk_proto_close() has freed ctx, and I thought that a test on the value
> of sk_state was sufficient.
> 
> If it's not, then we might invent something else. For example, we might
> defer freeing kTLS ctx, so that it's called as the very last thing with
> tcp_cleanup_ulp().

Mm.. I was hoping the user space can no longer access a socket once
it reaches sk_prot->close :S  Perhaps I got this wrong.  If it can 
we need to make sure we don't free context before calling tcp_close()
otherwise the state may still be established, no?

In particular:

#ifdef CONFIG_TLS_DEVICE
	if (ctx->rx_conf == TLS_HW)
		tls_device_offload_cleanup_rx(sk);

	if (ctx->tx_conf != TLS_HW && ctx->rx_conf != TLS_HW) {
#else
	{
#endif
		tls_ctx_free(ctx);        <<<  <<<   <<<   <<< kfree()
		ctx = NULL;
	}

skip_tx_cleanup:
	release_sock(sk);
	sk_proto_close(sk, timeout);      <<<  <<<   <<<   <<< tcp_close()
	/* free ctx for TLS_HW_RECORD, used by tcp_set_state
	 * for sk->sk_prot->unhash [tls_hw_unhash]
	 */
	if (free_ctx)
		tls_ctx_free(ctx);