Re: [PATCH for-next 12/22] RDMA/rtrs-clt: Check state of the rtrs_clt_sess before reading its stats

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Apr 06, 2021 at 10:55:59AM +0200, Gioh Kim wrote:
> On Thu, Apr 1, 2021 at 8:44 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Thu, Mar 25, 2021 at 04:32:58PM +0100, Gioh Kim wrote:
> > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > index 42f49208b8f7..1519191d7154 100644
> > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
> > > @@ -808,6 +808,9 @@ static struct rtrs_clt_sess *get_next_path_min_inflight(struct path_it *it)
> > >       int inflight;
> > >
> > >       list_for_each_entry_rcu(sess, &clt->paths_list, s.entry) {
> > > +             if (unlikely(READ_ONCE(sess->state) != RTRS_CLT_CONNECTED))
> > > +                     continue;
> >
> > There is no way this could be right, a READ_ONCE can't guarentee that
> > a following load is not going to happen without races.
> >
> > You need locking.
> 
> Hi Jason,
> 
> rtrs_clt_request() calls rcu_read_lock() before calling
> get_next_path_min_inflight().
> And rtrs_clt_change_state_from_to(), that changes the sess->state,
> calls spin_lock_irq() before changing it.
> I think that is enough, isn't it?

Why would that be enough?

Under RCU this check is racy and effetively does nothing.

This is an OK usage of RCU:

	list_del_rcu(&sess->s.entry);

	/* Make sure everybody observes path removal. */
	synchronize_rcu();

And you could say that observing the sess in the list is required, but
checking state is pointless.

Jason