On Mon, 19 Aug 2019 15:32:09 +0200, Davide Caratti wrote:

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

quoted

On Thu, 15 Aug 2019 20:46:01 +0200, Eric Dumazet wrote:  

quoted

On 8/15/19 6:00 PM, Davide Caratti wrote:

quoted

+	if (net_admin) {
+		const struct tcp_ulp_ops *ulp_ops;
+
+		rcu_read_lock();
+		ulp_ops = icsk->icsk_ulp_ops;
+		if (ulp_ops)
+			err = tcp_diag_put_ulp(skb, sk, ulp_ops);
+		rcu_read_unlock();
+		if (err)
+			return err;
+	}
 	return 0;    

Why is rcu_read_lock() and rcu_read_unlock() used at all ?

icsk->icsk_ulp_ops does not seem to be rcu protected ?

If this was, then an rcu_dereference() would be appropriate.  

Indeed it's ulp_data not ulp_ops that are protected.   

the goal is to protect execution of 'ss -tni' against concurrent removal
of tls.ko module, similarly to what was done in inet_sk_diag_fill() when
INET_DIAG_CONG is requested [1]. But after reading more carefully, the
assignment of ulp_ops needs to be:

	ulp_ops = READ_ONCE(icsk->icsk_ulp_ops);

which I lost in internal reviews, with some additional explanatory
comment. Ok if I correct the above hunk with READ_ONCE() and add a
comment?

Seems like a forth while future-proofing. Currently the ULP can't
change, and is only released when socket is destroyed, so we should 
be safe (unlike CC which can be changed at any moment). 

We should mark the pointer as RCU tho, I find it hard to wrap my head
around these half-way RCU pointers with just READ_ONCE() on them :S

quoted

Davide, perhaps we could push the RCU lock into tls_get_info(), after all?  

It depends on whether concurrent dump / module removal is an issue for TCP
ULPs, like it was for congestion control schemes [1]. Any advice?

If we're willing to mark icsk->icsk_ulp_ops as RCU I think it's fine.
But I'm not 100% sure its worth the churn :S

quoted

And tls_context has to use rcu_deference there, as Eric points out, 
plus we should probably NULL-check it.  

yes, it makes sense, for patch 3/3, in the assignment of 'ctx'. Instead of
calling tls_get_ctx() in tls_get_info() I will do

	ctx = rcu_dereference(inet_csk(sk)->icsk_ulp_data);

and let it return 0 in case of NULL ctx (as it doesn't look like a faulty
situation). Ok? 

SGTM!