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!
Re: [PATCH net-next 2/3] tcp: ulp: add functions to dump ulp-specific information
On Thu, 2019-08-15 at 14:38 -0700, Jakub Kicinski wrote:
> On Thu, 15 Aug 2019 20:46:01 +0200, Eric Dumazet wrote:
hello Eric and Jakub, thanks a lot for looking at this.
> > 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?
> 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?
> 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?
--
davide
[1] see:
commit 521f1cf1dbb9d5ad858dca5dc75d1b45f64b6589
Author: Eric Dumazet
Date: Thu Apr 16 18:10:35 2015 -0700
inet_diag: fix access to tcp cc information
Re: [PATCH net-next 2/3] tcp: ulp: add functions to dump ulp-specific information
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. Davide,
perhaps we could push the RCU lock into tls_get_info(), after all?
And tls_context has to use rcu_deference there, as Eric points out,
plus we should probably NULL-check it.
Re: [PATCH net-next 2/3] tcp: ulp: add functions to dump ulp-specific information
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.
