netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Davide Caratti <dcaratti@redhat.com>
To: Jakub Kicinski <jakub.kicinski@netronome.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Dave Watson <davejwatson@fb.com>,
	Boris Pismenny <borisp@mellanox.com>,
	Aviad Yehezkel <aviadye@mellanox.com>,
	John Fastabend <john.fastabend@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	netdev@vger.kernel.org
Subject: Re: [RFC PATCH net-next 2/2] net: tls: export protocol version and cipher to socket diag
Date: Mon, 17 Jun 2019 18:04:06 +0200	[thread overview]
Message-ID: <5ed5d6b3356c505ece2a354847e3aafd09fb82f3.camel@redhat.com> (raw)
In-Reply-To: <20190605162555.59b4fb3e@cakuba.netronome.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



  reply	other threads:[~2019-06-17 16:04 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-05 15:39 [RFC PATCH net-next 0/2] net: extend INET_DIAG_INFO with information specific to TCP ULP Davide Caratti
2019-06-05 15:39 ` [RFC PATCH net-next 1/2] tcp: ulp: add functions to dump ulp-specific information Davide Caratti
2019-06-05 23:14   ` Jakub Kicinski
2019-06-17 13:06     ` Davide Caratti
2019-06-17 17:41       ` Jakub Kicinski
2019-06-05 15:39 ` [RFC PATCH net-next 2/2] net: tls: export protocol version and cipher to socket diag Davide Caratti
2019-06-05 23:25   ` Jakub Kicinski
2019-06-17 16:04     ` Davide Caratti [this message]
2019-06-17 18:07       ` Jakub Kicinski
2019-06-06  7:07   ` Boris Pismenny

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5ed5d6b3356c505ece2a354847e3aafd09fb82f3.camel@redhat.com \
    --to=dcaratti@redhat.com \
    --cc=aviadye@mellanox.com \
    --cc=borisp@mellanox.com \
    --cc=daniel@iogearbox.net \
    --cc=davejwatson@fb.com \
    --cc=davem@davemloft.net \
    --cc=jakub.kicinski@netronome.com \
    --cc=john.fastabend@gmail.com \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [RFC PATCH net-next 2/2] net: tls: export protocol version and cipher to socket diag' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).