netdev - Re: [PATCH net-next 2/3] tcp: ulp: add functions to dump ulp-specific information

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 19 Aug 2019 13:40:58 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Davide Caratti <dcaratti@...hat.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        Boris Pismenny <borisp@...lanox.com>,
        John Fastabend <john.fastabend@...il.com>,
        Dave Watson <davejwatson@...com>,
        Aviad Yehezkel <aviadye@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/3] tcp: ulp: add functions to dump
 ulp-specific information

On Mon, 19 Aug 2019 15:32:09 +0200, Davide Caratti wrote:
> On Thu, 2019-08-15 at 14:38 -0700, Jakub Kicinski wrote:
> > On Thu, 15 Aug 2019 20:46:01 +0200, Eric Dumazet wrote:  
> > > On 8/15/19 6:00 PM, Davide Caratti wrote:
> > > > +	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

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

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