On Thu, 2019-08-15 at 14:32 -0700, Jakub Kicinski wrote:

On Thu, 15 Aug 2019 18:00:42 +0200, Davide Caratti wrote:

quoted

From: Jakub Kicinski <redacted>

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 <redacted>

hello Jakub,

quoted

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

quoted

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