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.kicin...@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.kicin...@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
