Re: [PATCHv3 3/6] sg: protect accesses to 'reserved' page array

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Fri, 2017-02-03 at 19:06 +0100, Johannes Thumshirn wrote:
> 
> On 02/03/2017 05:19 PM, Christoph Hellwig wrote:
> > On Fri, Feb 03, 2017 at 02:38:35PM +0100, Hannes Reinecke wrote:
> > > On 02/03/2017 02:31 PM, Christoph Hellwig wrote:
> > > > > -		if (sg_res_in_use(sfp)) {
> > > > > +		mutex_lock(&sfp->f_mutex);
> > > > > +		if (sfp->res_in_use) {
> > > > > +			mutex_unlock(&sfp->f_mutex);
> > > > >   			sg_remove_request(sfp, srp);
> > > > >   			return -EBUSY;	/* reserve
> > > > > buffer already being used */
> > > > >   		}
> > > > > +		mutex_unlock(&sfp->f_mutex);
> > > > Holding a mutex over a the check of a single scalar doesn't
> > > > make sense.
> > > > 
> > > It's adds a synchronisation point, doesn't it?
> > It does, but it doesn't actually protect anything..
> 
> But all the other mutex_{un,}locks() do (for instance guarding 
> sg_build_indirect()) and this one provides a synchronization point.
> 
> Sorry but I really don't get your point here.
> 
> The sole purpose is to guard the reserved list from being altered 
> while blk_rq_map_* or similar functions are in progess (that's what 
> the syzcaller reproducer was doing).

What he means is that naturally aligned reads are always atomic, so
adding further synchronisation gains nothing (you already atomically
get either the prior or next value) and only causes an unnecessary
pipeline stall.  From a reviewer's perspective, the sequence

lock
read
unlock

is always a red flag because it means the writer may not understand how
locking works.  Usually because the writer thinks there's some other
synchronization they need that this sequence doesn't give.

James