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

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

[Hannes Reinecke]

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);
 		}
 		return 0;
 	case SG_GET_RESERVED_SIZE:
@@ -1720,7 +1725,9 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
 		md = &map_data;
 
 	if (md) {
-		if (!sg_res_in_use(sfp) && dxfer_len <= rsv_schp->bufflen)
+		if (dxfer_len <= rsv_schp->bufflen &&
+		    test_and_set_bit(SG_RESERVED_IN_USE,
+				     &sfp->flags) == 0)
 			sg_link_reserve(sfp, srp, dxfer_len);
 		else {
 			res = sg_build_indirect(req_schp, sfp, dxfer_len);
@@ -2013,6 +2020,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
 	req_schp->sglist_len = 0;
 	sfp->save_scat_len = 0;
 	srp->res_used = 0;
+	clear_bit(SG_RESERVED_IN_USE, &sfp->flags);
 }
 
 static Sg_request *
@@ -2199,20 +2207,6 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
 	schedule_work(&sfp->ew.work);
 }
 
-static int
-sg_res_in_use(Sg_fd * sfp)
-{
-	const Sg_request *srp;
-	unsigned long iflags;
-
-	read_lock_irqsave(&sfp->rq_list_lock, iflags);
-	for (srp = sfp->headrp; srp; srp = srp->nextrp)
-		if (srp->res_used)
-			break;
-	read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
-	return srp ? 1 : 0;
-}
-
 #ifdef CONFIG_SCSI_PROC_FS
 static int
 sg_idr_max_id(int id, void *p, void *data)
-- 
1.8.5.6

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

[Johannes Thumshirn]

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>
> ---

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2134 bytes --]

Hi Hannes,

[auto build test ERROR on scsi/for-next]
[also build test ERROR on v4.10-rc6 next-20170201]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/sg-protect-access-to-to-reserved-page-array/20170201-192716
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: x86_64-randconfig-x009-201705 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/scsi/sg.c: In function 'sg_ioctl':
>> drivers/scsi/sg.c:896:37: error: implicit declaration of function 'sg_res_in_use' [-Werror=implicit-function-declaration]
       if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
                                        ^~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/sg_res_in_use +896 drivers/scsi/sg.c

^1da177e Linus Torvalds  2005-04-16  890  	case SG_SET_FORCE_LOW_DMA:
^1da177e Linus Torvalds  2005-04-16  891  		result = get_user(val, ip);
^1da177e Linus Torvalds  2005-04-16  892  		if (result)
^1da177e Linus Torvalds  2005-04-16  893  			return result;
^1da177e Linus Torvalds  2005-04-16  894  		if (val) {
^1da177e Linus Torvalds  2005-04-16  895  			sfp->low_dma = 1;
^1da177e Linus Torvalds  2005-04-16 @896  			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
^1da177e Linus Torvalds  2005-04-16  897  				val = (int) sfp->reserve.bufflen;
95e159d6 Hannes Reinecke 2014-06-25  898  				sg_remove_scat(sfp, &sfp->reserve);
^1da177e Linus Torvalds  2005-04-16  899  				sg_build_reserve(sfp, val);

:::::: The code at line 896 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28708 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3989 bytes --]

Hi Hannes,

[auto build test WARNING on scsi/for-next]
[also build test WARNING on v4.10-rc6 next-20170201]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/sg-protect-access-to-to-reserved-page-array/20170201-192716
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: i386-randconfig-x004-201705 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/linux/list.h:4,
                    from include/linux/module.h:9,
                    from drivers/scsi/sg.c:29:
   drivers/scsi/sg.c: In function 'sg_ioctl':
   drivers/scsi/sg.c:896:37: error: implicit declaration of function 'sg_res_in_use' [-Werror=implicit-function-declaration]
       if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
                                        ^
   include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> drivers/scsi/sg.c:896:4: note: in expansion of macro 'if'
       if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
       ^~
   cc1: some warnings being treated as errors

vim +/if +896 drivers/scsi/sg.c

f8630bd7e Paul Burton     2016-08-19  880  		if (val >= mult_frac((s64)INT_MAX, USER_HZ, HZ))
f8630bd7e Paul Burton     2016-08-19  881  			val = min_t(s64, mult_frac((s64)INT_MAX, USER_HZ, HZ),
b9b6e80ad Paul Burton     2016-08-19  882  				    INT_MAX);
^1da177e4 Linus Torvalds  2005-04-16  883  		sfp->timeout_user = val;
f8630bd7e Paul Burton     2016-08-19  884  		sfp->timeout = mult_frac(val, HZ, USER_HZ);
^1da177e4 Linus Torvalds  2005-04-16  885  
^1da177e4 Linus Torvalds  2005-04-16  886  		return 0;
^1da177e4 Linus Torvalds  2005-04-16  887  	case SG_GET_TIMEOUT:	/* N.B. User receives timeout as return value */
^1da177e4 Linus Torvalds  2005-04-16  888  				/* strange ..., for backward compatibility */
^1da177e4 Linus Torvalds  2005-04-16  889  		return sfp->timeout_user;
^1da177e4 Linus Torvalds  2005-04-16  890  	case SG_SET_FORCE_LOW_DMA:
^1da177e4 Linus Torvalds  2005-04-16  891  		result = get_user(val, ip);
^1da177e4 Linus Torvalds  2005-04-16  892  		if (result)
^1da177e4 Linus Torvalds  2005-04-16  893  			return result;
^1da177e4 Linus Torvalds  2005-04-16  894  		if (val) {
^1da177e4 Linus Torvalds  2005-04-16  895  			sfp->low_dma = 1;
^1da177e4 Linus Torvalds  2005-04-16 @896  			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
^1da177e4 Linus Torvalds  2005-04-16  897  				val = (int) sfp->reserve.bufflen;
95e159d6d Hannes Reinecke 2014-06-25  898  				sg_remove_scat(sfp, &sfp->reserve);
^1da177e4 Linus Torvalds  2005-04-16  899  				sg_build_reserve(sfp, val);
^1da177e4 Linus Torvalds  2005-04-16  900  			}
^1da177e4 Linus Torvalds  2005-04-16  901  		} else {
cc833acbe Douglas Gilbert 2014-06-25  902  			if (atomic_read(&sdp->detaching))
^1da177e4 Linus Torvalds  2005-04-16  903  				return -ENODEV;
^1da177e4 Linus Torvalds  2005-04-16  904  			sfp->low_dma = sdp->device->host->unchecked_isa_dma;

:::::: The code at line 896 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30735 bytes --]

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

[Christoph Hellwig]

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.  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

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

[Johannes Thumshirn]

On Wed, Feb 01, 2017 at 02:12:48PM +0100, Christoph Hellwig wrote:
> 
> This seems to be abusing an atomic bitflag as a lock.  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

I did have a patch doing exactly that but (appart from lockdep complaints) we
decided to drop it, as it made the code even more confusing.

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

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

[Hannes Reinecke]

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)

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

[Dmitry Vyukov]

On Wed, Feb 1, 2017 at 2:21 PM, Hannes Reinecke <hare@suse.de> wrote:
> 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.


So, how should we proceed here?
We could use a mutex with only trylock, but it would be effectively the same.

There is one missed user of sg_res_in_use in "case SG_SET_FORCE_LOW_DMA".

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

[Hannes Reinecke]

On 02/14/2017 09:48 PM, Dmitry Vyukov wrote:
> On Wed, Feb 1, 2017 at 2:21 PM, Hannes Reinecke <hare@suse.de> wrote:
>> 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.
> 
> 
> So, how should we proceed here?
> We could use a mutex with only trylock, but it would be effectively the same.
> 
> There is one missed user of sg_res_in_use in "case SG_SET_FORCE_LOW_DMA".
> 
I've played around using a spinlock (can't use a mutex as one access in
done from softirq context), but always ended up in lockdep hell.

And in the end, we really only need to have a marker whether the
reserved array is in use or not. Which should be atomic, so we _could_
be using an atomic variable here.
Which would only ever have a value of '0' or '1', hence the use of a
bitfield here.
But if that fall foul of some style guide I could modify it to use an
atomic counter.

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)