Re: [PATCH] sg: protect access to to 'reserved' page array

[Hannes Reinecke <hare@suse.de>]

On 02/01/2017 02:12 PM, Christoph Hellwig wrote:
> On Wed, Feb 01, 2017 at 12:22:15PM +0100, Hannes Reinecke wrote:
>> The 'reserved' page array is used as a short-cut for mapping
>> data, saving us to allocate pages per request.
>> However, the 'reserved' array is only capable of holding one
>> request, so we need to protect it against concurrent accesses.
>>
>> Cc: stable@vger.kernel.org
>> Reported-by: Dmitry Vyukov <dvyukov@google.com>
>> Link: http://www.spinics.net/lists/linux-scsi/msg104326.html
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> Tested-by: Johannes Thumshirn <jth@kernel.org>
>> ---
>>  drivers/scsi/sg.c | 30 ++++++++++++------------------
>>  1 file changed, 12 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index 652b934..6a8601c 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -155,6 +155,8 @@
>>  	unsigned char next_cmd_len; /* 0: automatic, >0: use on next write() */
>>  	char keep_orphan;	/* 0 -> drop orphan (def), 1 -> keep for read() */
>>  	char mmap_called;	/* 0 -> mmap() never called on this fd */
>> +	unsigned long flags;
>> +#define SG_RESERVED_IN_USE 1
>>  	struct kref f_ref;
>>  	struct execute_work ew;
>>  } Sg_fd;
>> @@ -198,7 +200,6 @@ static int sg_common_write(Sg_fd * sfp, Sg_request * srp,
>>  static Sg_request *sg_get_rq_mark(Sg_fd * sfp, int pack_id);
>>  static Sg_request *sg_add_request(Sg_fd * sfp);
>>  static int sg_remove_request(Sg_fd * sfp, Sg_request * srp);
>> -static int sg_res_in_use(Sg_fd * sfp);
>>  static Sg_device *sg_get_dev(int dev);
>>  static void sg_device_destroy(struct kref *kref);
>>  
>> @@ -721,7 +722,7 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
>>  			sg_remove_request(sfp, srp);
>>  			return -EINVAL;	/* either MMAP_IO or DIRECT_IO (not both) */
>>  		}
>> -		if (sg_res_in_use(sfp)) {
>> +		if (test_bit(SG_RESERVED_IN_USE, &sfp->flags)) {
>>  			sg_remove_request(sfp, srp);
>>  			return -EBUSY;	/* reserve buffer already being used */
>>  		}
>> @@ -963,10 +964,14 @@ static int max_sectors_bytes(struct request_queue *q)
>>  		val = min_t(int, val,
>>  			    max_sectors_bytes(sdp->device->request_queue));
>>  		if (val != sfp->reserve.bufflen) {
>> -			if (sg_res_in_use(sfp) || sfp->mmap_called)
>> +			if (sfp->mmap_called)
>> +				return -EBUSY;
>> +			if (test_and_set_bit(SG_RESERVED_IN_USE, &sfp->flags))
>>  				return -EBUSY;
>> +
>>  			sg_remove_scat(sfp, &sfp->reserve);
>>  			sg_build_reserve(sfp, val);
>> +			clear_bit(SG_RESERVED_IN_USE, &sfp->flags);
> 
> 
> This seems to be abusing an atomic bitflag as a lock.

Hmm. I wouldn't call it 'abusing'; the driver can proceed quite happily
without the 'reserved' buffer, so taking a lock would be overkill.
I could modify it to use a mutex if you insist ...

>  And I think
> in general we have two different things here that this patch conflates:
> 
>  a) a lock to protect building and using the reserve lists
>  b) a flag is a reservations is in use
> 
No. This is not about reservations, this is about the internal
'reserved' page buffer array.
(Just in case to avoid any misunderstandings).
We need to have an atomic / protected check in the 'sfp' structure if
the 'reserved' page buffer array is in use; there's an additional check
in the 'sg_request' structure (res_in_use) telling us which of the
requests is using it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)