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

[Davide Caratti <dcaratti@redhat.com>]

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