Re: [PATCH net-next 1/3] net/tls: use RCU protection on icsk->icsk_ulp_data

[Davide Caratti <dcaratti@redhat.com>]

On Thu, 2019-08-15 at 14:32 -0700, Jakub Kicinski wrote:
> On Thu, 15 Aug 2019 18:00:42 +0200, Davide Caratti wrote:
> > From: Jakub Kicinski <jakub.kicinski@netronome.com>
> > 
> > We need to make sure context does not get freed while diag
> > code is interrogating it. Free struct tls_context with
> > kfree_rcu().
> > 
> > We add the __rcu annotation directly in icsk, and cast it
> > away in the datapath accessor. Presumably all ULPs will
> > do a similar thing.
> > 
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

hello Jakub,

> > @@ -251,14 +251,31 @@ static void tls_write_space(struct sock *sk)
> >  	ctx->sk_write_space(sk);
> >  }
> >  
> > -void tls_ctx_free(struct tls_context *ctx)
> > +/**
> > + * tls_ctx_free() - free TLS ULP context
> > + * @sk:  socket to with @ctx is attached
> > + * @ctx: TLS context structure
> > + *
> > + * Free TLS context. If @sk is %NULL caller guarantees that the socket
> > + * to which @ctx was attached has no outstanding references.
> > + */
> > +void tls_ctx_free(struct sock *sk, struct tls_context *ctx)
> >  {
> > +	struct inet_connection_sock *icsk;
> > +
> >  	if (!ctx)
> >  		return;
> >  
> >  	memzero_explicit(&ctx->crypto_send, sizeof(ctx->crypto_send));
> >  	memzero_explicit(&ctx->crypto_recv, sizeof(ctx->crypto_recv));
> > -	kfree(ctx);
> > +
> > +	if (sk) {
> > +		icsk = inet_csk(sk);
> > +		rcu_assign_pointer(icsk->icsk_ulp_data, NULL);
> 
> Now that we kind of want to set the icsk_ulp_data to NULL under the
> callback_lock I think we should let the callers do it.

Ok, I will fix this in series v2.

> > 
> > @@ -649,8 +666,8 @@ static void tls_hw_sk_destruct(struct sock *sk)
> >  
> >  	ctx->sk_destruct(sk);
> >  	/* Free ctx */
> > -	tls_ctx_free(ctx);
> > -	icsk->icsk_ulp_data = NULL;
> > +	tls_ctx_free(sk, ctx);
> > +	rcu_assign_pointer(icsk->icsk_ulp_data, NULL);
> 
> Let's reorder the assignment before the free.

Ok, I will fix this in series v2.

thanks!
-- 
davide