Re: [PATCH 4/7] [SCSI] scst: Add SRP target driver

[Roland Dreier <rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>]

So, just looking over some of the atomic_t usage here (which is usually
one of the first places I review):

 > +struct srpt_send_ioctx {
 > ...
 > +	atomic_t		state;

this seems to be accessed only with atomic_read(), atomic_set() and
atomic_cmpxchg() (without any memory barriers).  It's rather hard to see
that this is correct (and indeed without memory barriers I suspect it's
not; also since atomic_read() does not protect the state from changing
immediately afterwards there may be other races).

I think it might be better to handle this in a simpler, more naive
way -- just have a lock (probably an existing one) that protects the
contents of state and not use cmpxchg().

In any case since no "real" atomic operations are used, I suspect it
would be better to just code this in terms of unsigned int and regular
cmpxchg().

Also, there is processing_compl:

 > +static void srpt_completion(struct ib_cq *cq, void *ctx)
 > +{
 > +	struct srpt_rdma_ch *ch = ctx;
 > +
 > +	BUG_ON(!ch);
 > +	atomic_inc(&ch->processing_compl);

and

 > +static void srpt_unregister_channel(struct srpt_rdma_ch *ch)
 > ...
 > +	while (atomic_read(&ch->processing_compl))
 > +		;

this seems racy to me -- I don't see any reason why we couldn't have:

	srpt_completion()

					srpt_unregister_channel()
					  processing_compl == 0,
					  continue

	  atomic_inc(&ch->processing_compl);

					  finish unregistering channel

	  use unregistered channel

 - R.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html