[PATCH 0/4] sanitize sg

[PATCH 0/4] sanitize sg

[Hannes Reinecke]

Hi all,

the infamous syzkaller incovered some more issues in the sg driver.
This patchset fixes those two issues (and adds a fix for yet another
potential issue; checking for a NULL dxferp when dxfer_len is not 0).
It also removes handling of the SET_FORCE_LOW_DMA ioctl, which never
worked since the initial git checkin. And does some code cleanup by
removing the private list implementation, using standard lists instead.

As usual, comments and reviews are welcome.

Hannes Reinecke (3):
  sg: disable SET_FORCE_LOW_DMA
  sg: protect access to to 'reserved' page array
  sg: use standard lists for sg_requests

Johannes Thumshirn (1):
  sg: check for valid direction before starting the request

 drivers/scsi/sg.c | 183 ++++++++++++++++++++++++------------------------------
 include/scsi/sg.h |   1 -
 2 files changed, 80 insertions(+), 104 deletions(-)

-- 
1.8.5.6

[PATCH 1/4] sg: disable SET_FORCE_LOW_DMA

[Hannes Reinecke]

The ioctl SET_FORCE_LOW_DMA has never worked since the initial
git check-in, and the respective setting is nowadays handled
correctly.
So disable it entirely.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/sg.c | 27 +++++----------------------
 include/scsi/sg.h |  1 -
 2 files changed, 5 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index dbe5b4b..8a959b2 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -149,7 +149,6 @@
 	Sg_request *headrp;	/* head of request slist, NULL->empty */
 	struct fasync_struct *async_qp;	/* used by asynchronous notification */
 	Sg_request req_arr[SG_MAX_QUEUE];	/* used as singly-linked list */
-	char low_dma;		/* as in parent but possibly overridden to 1 */
 	char force_packid;	/* 1 -> pack_id input to read(), 0 -> ignored */
 	char cmd_q;		/* 1 -> allow command queuing, 0 -> don't */
 	unsigned char next_cmd_len; /* 0: automatic, >0: use on next write() */
@@ -887,24 +886,9 @@ static int max_sectors_bytes(struct request_queue *q)
 				/* strange ..., for backward compatibility */
 		return sfp->timeout_user;
 	case SG_SET_FORCE_LOW_DMA:
-		result = get_user(val, ip);
-		if (result)
-			return result;
-		if (val) {
-			sfp->low_dma = 1;
-			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
-				val = (int) sfp->reserve.bufflen;
-				sg_remove_scat(sfp, &sfp->reserve);
-				sg_build_reserve(sfp, val);
-			}
-		} else {
-			if (atomic_read(&sdp->detaching))
-				return -ENODEV;
-			sfp->low_dma = sdp->device->host->unchecked_isa_dma;
-		}
-		return 0;
+		return -EINVAL;
 	case SG_GET_LOW_DMA:
-		return put_user((int) sfp->low_dma, ip);
+		return put_user((int) sdp->device->host->unchecked_isa_dma, ip);
 	case SG_GET_SCSI_ID:
 		if (!access_ok(VERIFY_WRITE, p, sizeof (sg_scsi_id_t)))
 			return -EFAULT;
@@ -1821,6 +1805,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
 	int sg_tablesize = sfp->parentdp->sg_tablesize;
 	int blk_size = buff_size, order;
 	gfp_t gfp_mask = GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN;
+	struct sg_device *sdp = sfp->parentdp;
 
 	if (blk_size < 0)
 		return -EFAULT;
@@ -1846,7 +1831,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
 			scatter_elem_sz_prev = num;
 	}
 
-	if (sfp->low_dma)
+	if (sdp->device->host->unchecked_isa_dma)
 		gfp_mask |= GFP_DMA;
 
 	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
@@ -2132,8 +2117,6 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
 	sfp->timeout = SG_DEFAULT_TIMEOUT;
 	sfp->timeout_user = SG_DEFAULT_TIMEOUT_USER;
 	sfp->force_packid = SG_DEF_FORCE_PACK_ID;
-	sfp->low_dma = (SG_DEF_FORCE_LOW_DMA == 0) ?
-	    sdp->device->host->unchecked_isa_dma : 1;
 	sfp->cmd_q = SG_DEF_COMMAND_Q;
 	sfp->keep_orphan = SG_DEF_KEEP_ORPHAN;
 	sfp->parentdp = sdp;
@@ -2603,7 +2586,7 @@ static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
 			   jiffies_to_msecs(fp->timeout),
 			   fp->reserve.bufflen,
 			   (int) fp->reserve.k_use_sg,
-			   (int) fp->low_dma);
+			   (int) sdp->device->host->unchecked_isa_dma);
 		seq_printf(s, "   cmd_q=%d f_packid=%d k_orphan=%d closed=0\n",
 			   (int) fp->cmd_q, (int) fp->force_packid,
 			   (int) fp->keep_orphan);
diff --git a/include/scsi/sg.h b/include/scsi/sg.h
index 3afec70..20bc71c 100644
--- a/include/scsi/sg.h
+++ b/include/scsi/sg.h
@@ -197,7 +197,6 @@
 #define SG_DEFAULT_RETRIES 0
 
 /* Defaults, commented if they differ from original sg driver */
-#define SG_DEF_FORCE_LOW_DMA 0  /* was 1 -> memory below 16MB on i386 */
 #define SG_DEF_FORCE_PACK_ID 0
 #define SG_DEF_KEEP_ORPHAN 0
 #define SG_DEF_RESERVED_SIZE SG_SCATTER_SZ /* load time option */
-- 
1.8.5.6

Re: [PATCH 1/4] sg: disable SET_FORCE_LOW_DMA

[Johannes Thumshirn]

On 02/03/2017 09:54 AM, Hannes Reinecke wrote:
> The ioctl SET_FORCE_LOW_DMA has never worked since the initial
> git check-in, and the respective setting is nowadays handled
> correctly.
> So disable it entirely.
>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---

[...]

>   	case SG_SET_FORCE_LOW_DMA:
> -		result = get_user(val, ip);
> -		if (result)
> -			return result;
> -		if (val) {
> -			sfp->low_dma = 1;
> -			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
> -				val = (int) sfp->reserve.bufflen;
> -				sg_remove_scat(sfp, &sfp->reserve);
> -				sg_build_reserve(sfp, val);
> -			}
> -		} else {
> -			if (atomic_read(&sdp->detaching))
> -				return -ENODEV;
> -			sfp->low_dma = sdp->device->host->unchecked_isa_dma;
> -		}
> -		return 0;
> +		return -EINVAL;

I'm not sure if returning a bogus '0' is better here to not break any 
existing application, which assumed it always worked.

Re: [PATCH 1/4] sg: disable SET_FORCE_LOW_DMA

[Hannes Reinecke]

On 02/03/2017 10:32 AM, Johannes Thumshirn wrote:
> 
> On 02/03/2017 09:54 AM, Hannes Reinecke wrote:
>> The ioctl SET_FORCE_LOW_DMA has never worked since the initial
>> git check-in, and the respective setting is nowadays handled
>> correctly.
>> So disable it entirely.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
> 
> [...]
> 
>>       case SG_SET_FORCE_LOW_DMA:
>> -        result = get_user(val, ip);
>> -        if (result)
>> -            return result;
>> -        if (val) {
>> -            sfp->low_dma = 1;
>> -            if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
>> -                val = (int) sfp->reserve.bufflen;
>> -                sg_remove_scat(sfp, &sfp->reserve);
>> -                sg_build_reserve(sfp, val);
>> -            }
>> -        } else {
>> -            if (atomic_read(&sdp->detaching))
>> -                return -ENODEV;
>> -            sfp->low_dma = sdp->device->host->unchecked_isa_dma;
>> -        }
>> -        return 0;
>> +        return -EINVAL;
> 
> I'm not sure if returning a bogus '0' is better here to not break any
> existing application, which assumed it always worked.
> 
Well, the ioctl already was returning error numbers, so any calling
application had to protect against that.
And I don't really see the point to protect against failures in legacy
applications relying on a functionality which never did anything.

Cheers,

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

Re: [PATCH 1/4] sg: disable SET_FORCE_LOW_DMA

[Christoph Hellwig]

On Fri, Feb 03, 2017 at 10:32:28AM +0100, Johannes Thumshirn wrote:
> I'm not sure if returning a bogus '0' is better here to not break any 
> existing application, which assumed it always worked.

Agreed, this should return 0, and maybe also print a (ratelimited)
warning.

[PATCH 2/4] 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 8a959b2..c29962c 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -154,6 +154,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;
@@ -197,7 +199,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);
 
@@ -720,7 +721,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 */
 		}
@@ -952,10 +953,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:
@@ -1709,7 +1714,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);
@@ -2003,6 +2010,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 *
@@ -2187,20 +2195,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 2/4] sg: protect access to to 'reserved' page array

[Johannes Thumshirn]

On 02/03/2017 09:54 AM, 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 2/4] sg: protect access to to 'reserved' page array

[Christoph Hellwig]

On Fri, Feb 03, 2017 at 09:54:49AM +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.

Can you please explain how you protect the access here a bit more,
as mentioned before the set_bit for exclusion trick is always
suspicious, so the changelog needs to have a justification for it.

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

[Hannes Reinecke]

On 02/03/2017 11:24 AM, Christoph Hellwig wrote:
> On Fri, Feb 03, 2017 at 09:54:49AM +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.
> 
> Can you please explain how you protect the access here a bit more,
> as mentioned before the set_bit for exclusion trick is always
> suspicious, so the changelog needs to have a justification for it.
> 
The 'reserved' array provides for a fast/reliable mechanism for mapping
data of a request. However, it only has enough room to hold one request
at a time.
Plus we can change the size of the buffer during runtime via an ioctl.
So we need to mark the array as 'in use' atomically, and keep that
marker as long as the request using it is active.
While I surely can introduce a variable 'in_use' and protect accesses to
it via mutex or somesuch, I found this to be a bit pointless given that
it's actually just one bit which needs to be checked.
Which is what I did.

But okay, I'll update the description.

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)

[PATCH 3/4] sg: check for valid direction before starting the request

[Hannes Reinecke]

From: Johannes Thumshirn <jthumshirn@suse.de>

Check for a valid direction before starting the request, otherwise we risk
running into an assertion in the scsi midlayer checking for vaild requests.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
Link: http://www.spinics.net/lists/linux-scsi/msg104400.html
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/sg.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index c29962c..3599551 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -752,6 +752,20 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
 	return count;
 }
 
+static bool sg_is_valid_direction(int dxfer_direction)
+{
+	switch (dxfer_direction) {
+	case SG_DXFER_NONE:
+	case SG_DXFER_TO_DEV:
+	case SG_DXFER_FROM_DEV:
+	case SG_DXFER_TO_FROM_DEV:
+	case SG_DXFER_UNKNOWN:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int
 sg_common_write(Sg_fd * sfp, Sg_request * srp,
 		unsigned char *cmnd, int timeout, int blocking)
@@ -772,6 +786,11 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)
 			"sg_common_write:  scsi opcode=0x%02x, cmd_size=%d\n",
 			(int) cmnd[0], (int) hp->cmd_len));
 
+	if (!sg_is_valid_direction(hp->dxfer_direction))
+		return -EINVAL;
+	if (hp->dxferp == NULL && hp->dxfer_len > 0)
+		return -EINVAL;
+
 	k = sg_start_req(srp, cmnd);
 	if (k) {
 		SCSI_LOG_TIMEOUT(1, sg_printk(KERN_INFO, sfp->parentdp,
-- 
1.8.5.6

Re: [PATCH 3/4] sg: check for valid direction before starting the request

[Christoph Hellwig]

On Fri, Feb 03, 2017 at 09:54:50AM +0100, Hannes Reinecke wrote:
> From: Johannes Thumshirn <jthumshirn@suse.de>
> 
> Check for a valid direction before starting the request, otherwise we risk
> running into an assertion in the scsi midlayer checking for vaild requests.

Good idea, but..

> +static bool sg_is_valid_direction(int dxfer_direction)
> +{
> +	switch (dxfer_direction) {
> +	case SG_DXFER_NONE:
> +	case SG_DXFER_TO_DEV:
> +	case SG_DXFER_FROM_DEV:
> +	case SG_DXFER_TO_FROM_DEV:

This isn't strictly valid as sg doesn't actually handle real bidi
commands, but we work around it.

It might be a good idea to move the warning about it from sg_write
to here and use printk_ratelimited instead of the hand-rolled
per-thread warning there.

> +	case SG_DXFER_UNKNOWN:
> +		return true;

And how valid is this one (Question to Doug I guess): for a 0-sized
transfer we should be fine, but otherwise it should probably be
rejected.

Re: [PATCH 3/4] sg: check for valid direction before starting the request

[Hannes Reinecke]

On 02/03/2017 11:28 AM, Christoph Hellwig wrote:
> On Fri, Feb 03, 2017 at 09:54:50AM +0100, Hannes Reinecke wrote:
>> From: Johannes Thumshirn <jthumshirn@suse.de>
>>
>> Check for a valid direction before starting the request, otherwise we risk
>> running into an assertion in the scsi midlayer checking for vaild requests.
> 
> Good idea, but..
> 
>> +static bool sg_is_valid_direction(int dxfer_direction)
>> +{
>> +	switch (dxfer_direction) {
>> +	case SG_DXFER_NONE:
>> +	case SG_DXFER_TO_DEV:
>> +	case SG_DXFER_FROM_DEV:
>> +	case SG_DXFER_TO_FROM_DEV:
> 
> This isn't strictly valid as sg doesn't actually handle real bidi
> commands, but we work around it.
> 
> It might be a good idea to move the warning about it from sg_write
> to here and use printk_ratelimited instead of the hand-rolled
> per-thread warning there.
> 
>> +	case SG_DXFER_UNKNOWN:
>> +		return true;
> 
> And how valid is this one (Question to Doug I guess): for a 0-sized
> transfer we should be fine, but otherwise it should probably be
> rejected.
> 
yes, we should be checking for dxferp and dxfer_len here, too.

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 3/4] sg: check for valid direction before starting the request

[Hannes Reinecke]

On 02/03/2017 11:28 AM, Christoph Hellwig wrote:
> On Fri, Feb 03, 2017 at 09:54:50AM +0100, Hannes Reinecke wrote:
>> From: Johannes Thumshirn <jthumshirn@suse.de>
>>
>> Check for a valid direction before starting the request, otherwise we risk
>> running into an assertion in the scsi midlayer checking for vaild requests.
> 
> Good idea, but..
> 
>> +static bool sg_is_valid_direction(int dxfer_direction)
>> +{
>> +	switch (dxfer_direction) {
>> +	case SG_DXFER_NONE:
>> +	case SG_DXFER_TO_DEV:
>> +	case SG_DXFER_FROM_DEV:
>> +	case SG_DXFER_TO_FROM_DEV:
> 
> This isn't strictly valid as sg doesn't actually handle real bidi
> commands, but we work around it.
> 
> It might be a good idea to move the warning about it from sg_write
> to here and use printk_ratelimited instead of the hand-rolled
> per-thread warning there.
> 
Well, that warning is only applicable to sg_write(); sg_new_write() is
supposed to have the correct header, so that warning doesn't apply.
We could drop the per-thread-thingie, though.


>> +	case SG_DXFER_UNKNOWN:
>> +		return true;
> 
> And how valid is this one (Question to Doug I guess): for a 0-sized
> transfer we should be fine, but otherwise it should probably be
> rejected.
> 
Yes, true.
Will be fixing it up.

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)

[PATCH 4/4] sg: use standard lists for sg_requests

[Hannes Reinecke]

'Sg_request' is using a private list implementation; convert it
to standard lists.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/sg.c | 107 ++++++++++++++++++++++--------------------------------
 1 file changed, 44 insertions(+), 63 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 3599551..9b0429d 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -122,7 +122,7 @@
 struct sg_fd;
 
 typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
-	struct sg_request *nextrp;	/* NULL -> tail request (slist) */
+	struct list_head nextrp;	/* list entry */
 	struct sg_fd *parentfp;	/* NULL -> not in use */
 	Sg_scatter_hold data;	/* hold buffer, perhaps scatter list */
 	sg_io_hdr_t header;	/* scsi command+info, see <scsi/sg.h> */
@@ -146,7 +146,7 @@
 	int timeout_user;	/* defaults to SG_DEFAULT_TIMEOUT_USER */
 	Sg_scatter_hold reserve;	/* buffer held for this file descriptor */
 	unsigned save_scat_len;	/* original length of trunc. scat. element */
-	Sg_request *headrp;	/* head of request slist, NULL->empty */
+	struct list_head rq_list; /* head of request list */
 	struct fasync_struct *async_qp;	/* used by asynchronous notification */
 	Sg_request req_arr[SG_MAX_QUEUE];	/* used as singly-linked list */
 	char force_packid;	/* 1 -> pack_id input to read(), 0 -> ignored */
@@ -942,7 +942,7 @@ static int max_sectors_bytes(struct request_queue *q)
 		if (!access_ok(VERIFY_WRITE, ip, sizeof (int)))
 			return -EFAULT;
 		read_lock_irqsave(&sfp->rq_list_lock, iflags);
-		for (srp = sfp->headrp; srp; srp = srp->nextrp) {
+		list_for_each_entry(srp, &sfp->rq_list, nextrp) {
 			if ((1 == srp->done) && (!srp->sg_io_owned)) {
 				read_unlock_irqrestore(&sfp->rq_list_lock,
 						       iflags);
@@ -955,7 +955,8 @@ static int max_sectors_bytes(struct request_queue *q)
 		return 0;
 	case SG_GET_NUM_WAITING:
 		read_lock_irqsave(&sfp->rq_list_lock, iflags);
-		for (val = 0, srp = sfp->headrp; srp; srp = srp->nextrp) {
+		val = 0;
+		list_for_each_entry(srp, &sfp->rq_list, nextrp) {
 			if ((1 == srp->done) && (!srp->sg_io_owned))
 				++val;
 		}
@@ -1026,35 +1027,33 @@ static int max_sectors_bytes(struct request_queue *q)
 			if (!rinfo)
 				return -ENOMEM;
 			read_lock_irqsave(&sfp->rq_list_lock, iflags);
-			for (srp = sfp->headrp, val = 0; val < SG_MAX_QUEUE;
-			     ++val, srp = srp ? srp->nextrp : srp) {
+			val = 0;
+			list_for_each_entry(srp, &sfp->rq_list, nextrp) {
+				if (val > SG_MAX_QUEUE)
+					break;
 				memset(&rinfo[val], 0, SZ_SG_REQ_INFO);
-				if (srp) {
-					rinfo[val].req_state = srp->done + 1;
-					rinfo[val].problem =
-					    srp->header.masked_status & 
-					    srp->header.host_status & 
-					    srp->header.driver_status;
-					if (srp->done)
-						rinfo[val].duration =
-							srp->header.duration;
-					else {
-						ms = jiffies_to_msecs(jiffies);
-						rinfo[val].duration =
-						    (ms > srp->header.duration) ?
-						    (ms - srp->header.duration) : 0;
-					}
-					rinfo[val].orphan = srp->orphan;
-					rinfo[val].sg_io_owned =
-							srp->sg_io_owned;
-					rinfo[val].pack_id =
-							srp->header.pack_id;
-					rinfo[val].usr_ptr =
-							srp->header.usr_ptr;
+				rinfo[val].req_state = srp->done + 1;
+				rinfo[val].problem =
+					srp->header.masked_status &
+					srp->header.host_status &
+					srp->header.driver_status;
+				if (srp->done)
+					rinfo[val].duration =
+						srp->header.duration;
+				else {
+					ms = jiffies_to_msecs(jiffies);
+					rinfo[val].duration =
+						(ms > srp->header.duration) ?
+						(ms - srp->header.duration) : 0;
 				}
+				rinfo[val].orphan = srp->orphan;
+				rinfo[val].sg_io_owned = srp->sg_io_owned;
+				rinfo[val].pack_id = srp->header.pack_id;
+				rinfo[val].usr_ptr = srp->header.usr_ptr;
+				val++;
 			}
 			read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
-			result = __copy_to_user(p, rinfo, 
+			result = __copy_to_user(p, rinfo,
 						SZ_SG_REQ_INFO * SG_MAX_QUEUE);
 			result = result ? -EFAULT : 0;
 			kfree(rinfo);
@@ -1160,7 +1159,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
 		return POLLERR;
 	poll_wait(filp, &sfp->read_wait, wait);
 	read_lock_irqsave(&sfp->rq_list_lock, iflags);
-	for (srp = sfp->headrp; srp; srp = srp->nextrp) {
+	list_for_each_entry(srp, &sfp->rq_list, nextrp) {
 		/* if any read waiting, flag it */
 		if ((0 == res) && (1 == srp->done) && (!srp->sg_io_owned))
 			res = POLLIN | POLLRDNORM;
@@ -2039,7 +2038,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
 	unsigned long iflags;
 
 	write_lock_irqsave(&sfp->rq_list_lock, iflags);
-	for (resp = sfp->headrp; resp; resp = resp->nextrp) {
+	list_for_each_entry(resp, &sfp->rq_list, nextrp) {
 		/* look for requests that are ready + not SG_IO owned */
 		if ((1 == resp->done) && (!resp->sg_io_owned) &&
 		    ((-1 == pack_id) || (resp->header.pack_id == pack_id))) {
@@ -2061,12 +2060,11 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
 	Sg_request *rp = sfp->req_arr;
 
 	write_lock_irqsave(&sfp->rq_list_lock, iflags);
-	resp = sfp->headrp;
-	if (!resp) {
+	if (list_empty(&sfp->rq_list)) {
 		memset(rp, 0, sizeof (Sg_request));
 		rp->parentfp = sfp;
 		resp = rp;
-		sfp->headrp = resp;
+		list_add(&rp->nextrp, &sfp->rq_list);
 	} else {
 		if (0 == sfp->cmd_q)
 			resp = NULL;	/* command queuing disallowed */
@@ -2078,16 +2076,13 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
 			if (k < SG_MAX_QUEUE) {
 				memset(rp, 0, sizeof (Sg_request));
 				rp->parentfp = sfp;
-				while (resp->nextrp)
-					resp = resp->nextrp;
-				resp->nextrp = rp;
+				list_add(&rp->nextrp, &sfp->rq_list);
 				resp = rp;
 			} else
 				resp = NULL;
 		}
 	}
 	if (resp) {
-		resp->nextrp = NULL;
 		resp->header.duration = jiffies_to_msecs(jiffies);
 	}
 	write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
@@ -2098,29 +2093,16 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
 static int
 sg_remove_request(Sg_fd * sfp, Sg_request * srp)
 {
-	Sg_request *prev_rp;
-	Sg_request *rp;
 	unsigned long iflags;
 	int res = 0;
 
-	if ((!sfp) || (!srp) || (!sfp->headrp))
+	if ((!sfp) || (!srp) || (list_empty(&sfp->rq_list)))
 		return res;
 	write_lock_irqsave(&sfp->rq_list_lock, iflags);
-	prev_rp = sfp->headrp;
-	if (srp == prev_rp) {
-		sfp->headrp = prev_rp->nextrp;
-		prev_rp->parentfp = NULL;
+	if (!list_empty(&srp->nextrp)) {
+		list_del_init(&srp->nextrp);
+		srp->parentfp = NULL;
 		res = 1;
-	} else {
-		while ((rp = prev_rp->nextrp)) {
-			if (srp == rp) {
-				prev_rp->nextrp = rp->nextrp;
-				rp->parentfp = NULL;
-				res = 1;
-				break;
-			}
-			prev_rp = rp;
-		}
 	}
 	write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
 	return res;
@@ -2177,10 +2159,11 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
 {
 	struct sg_fd *sfp = container_of(work, struct sg_fd, ew.work);
 	struct sg_device *sdp = sfp->parentdp;
+	Sg_request *srp, *tmp;
 
 	/* Cleanup any responses which were never read(). */
-	while (sfp->headrp)
-		sg_finish_rem_req(sfp->headrp);
+	list_for_each_entry_safe(srp, tmp, &sfp->rq_list, nextrp)
+		sg_finish_rem_req(srp);
 
 	if (sfp->reserve.bufflen > 0) {
 		SCSI_LOG_TIMEOUT(6, sg_printk(KERN_INFO, sdp,
@@ -2583,7 +2566,7 @@ static int sg_proc_seq_show_devstrs(struct seq_file *s, void *v)
 /* must be called while holding sg_index_lock */
 static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
 {
-	int k, m, new_interface, blen, usg;
+	int k, new_interface, blen, usg;
 	Sg_request *srp;
 	Sg_fd *fp;
 	const sg_io_hdr_t *hp;
@@ -2603,13 +2586,11 @@ static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
 		seq_printf(s, "   cmd_q=%d f_packid=%d k_orphan=%d closed=0\n",
 			   (int) fp->cmd_q, (int) fp->force_packid,
 			   (int) fp->keep_orphan);
-		for (m = 0, srp = fp->headrp;
-				srp != NULL;
-				++m, srp = srp->nextrp) {
+		list_for_each_entry(srp, &fp->rq_list, nextrp) {
 			hp = &srp->header;
 			new_interface = (hp->interface_id == '\0') ? 0 : 1;
 			if (srp->res_used) {
-				if (new_interface && 
+				if (new_interface &&
 				    (SG_FLAG_MMAP_IO & hp->flags))
 					cp = "     mmap>> ";
 				else
@@ -2640,7 +2621,7 @@ static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)
 			seq_printf(s, "ms sgat=%d op=0x%02x\n", usg,
 				   (int) srp->data.cmd_opcode);
 		}
-		if (0 == m)
+		if (list_empty(&fp->rq_list))
 			seq_puts(s, "     No requests active\n");
 		read_unlock(&fp->rq_list_lock);
 	}
-- 
1.8.5.6

Re: [PATCH 4/4] sg: use standard lists for sg_requests

[Johannes Thumshirn]

On 02/03/2017 09:54 AM, Hannes Reinecke wrote:
> 'Sg_request' is using a private list implementation; convert it
> to standard lists.
>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
Looks good,
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 4/4] sg: use standard lists for sg_requests

[Christoph Hellwig]

>  typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
> -	struct sg_request *nextrp;	/* NULL -> tail request (slist) */
> +	struct list_head nextrp;	/* list entry */

s/nextrp/entry/

> @@ -2078,16 +2076,13 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
>  			if (k < SG_MAX_QUEUE) {
>  				memset(rp, 0, sizeof (Sg_request));
>  				rp->parentfp = sfp;
> +				list_add(&rp->nextrp, &sfp->rq_list);

The old code did a tail insertation.  And this whole function should
become a lot simpler with proper lists anyway:

static Sg_request *
sg_add_request(Sg_fd * sfp)
{
	int k;
	unsigned long iflags;
	Sg_request *rp = sfp->req_arr;

	write_lock_irqsave(&sfp->rq_list_lock, iflags);
	if (!list_empty(&sfp->rq_list)) {
		if (!sfp->cmd_q)
			goto out_unlock;

		for (k = 0; k < SG_MAX_QUEUE; ++k, ++rp) {
			if (!rp->parentfp)
				break;
		}
		if (k >= SG_MAX_QUEUE)
			goto out_unlock;
	}

	memset(rp, 0, sizeof (Sg_request));
	rp->parentfp = sfp;
	rp->header.duration = jiffies_to_msecs(jiffies);
	list_add_tail(&rp->nextrp, &sfp->rq_list);
	write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
	return rp;

out_unlock:
	write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
	return NULL;

> +	if ((!sfp) || (!srp) || (list_empty(&sfp->rq_list)))

No need for all these braces.

> +	if (!list_empty(&srp->nextrp)) {
> +		list_del_init(&srp->nextrp);

I don't think we need the _init as we never check for an empty entry.

>  {
>  	struct sg_fd *sfp = container_of(work, struct sg_fd, ew.work);
>  	struct sg_device *sdp = sfp->parentdp;
> +	Sg_request *srp, *tmp;
>  
>  	/* Cleanup any responses which were never read(). */
> -	while (sfp->headrp)
> -		sg_finish_rem_req(sfp->headrp);
> +	list_for_each_entry_safe(srp, tmp, &sfp->rq_list, nextrp)
> +		sg_finish_rem_req(srp);

What protects us from concurrent removals here?

Either way I'd rather keep the whіle not emptry style even with
proper lists.

Re: [PATCH 4/4] sg: use standard lists for sg_requests

[Hannes Reinecke]

On 02/03/2017 11:43 AM, Christoph Hellwig wrote:
>>  typedef struct sg_request {	/* SG_MAX_QUEUE requests outstanding per file */
>> -	struct sg_request *nextrp;	/* NULL -> tail request (slist) */
>> +	struct list_head nextrp;	/* list entry */
> 
> s/nextrp/entry/
> 
>> @@ -2078,16 +2076,13 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon
>>  			if (k < SG_MAX_QUEUE) {
>>  				memset(rp, 0, sizeof (Sg_request));
>>  				rp->parentfp = sfp;
>> +				list_add(&rp->nextrp, &sfp->rq_list);
> 
> The old code did a tail insertation.  And this whole function should
> become a lot simpler with proper lists anyway:
> 
Yeah, thought about that, too, but then I just went for the sloppy
approach to minimize changes.

> static Sg_request *
> sg_add_request(Sg_fd * sfp)
> {
> 	int k;
> 	unsigned long iflags;
> 	Sg_request *rp = sfp->req_arr;
> 
> 	write_lock_irqsave(&sfp->rq_list_lock, iflags);
> 	if (!list_empty(&sfp->rq_list)) {
> 		if (!sfp->cmd_q)
> 			goto out_unlock;
> 
> 		for (k = 0; k < SG_MAX_QUEUE; ++k, ++rp) {
> 			if (!rp->parentfp)
> 				break;
> 		}
> 		if (k >= SG_MAX_QUEUE)
> 			goto out_unlock;
> 	}
> 
> 	memset(rp, 0, sizeof (Sg_request));
> 	rp->parentfp = sfp;
> 	rp->header.duration = jiffies_to_msecs(jiffies);
> 	list_add_tail(&rp->nextrp, &sfp->rq_list);
> 	write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
> 	return rp;
> 
> out_unlock:
> 	write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
> 	return NULL;
> 
Okay, will be updating the patch.

>> +	if ((!sfp) || (!srp) || (list_empty(&sfp->rq_list)))
> 
> No need for all these braces.
> 
Okay.

>> +	if (!list_empty(&srp->nextrp)) {
>> +		list_del_init(&srp->nextrp);
> 
> I don't think we need the _init as we never check for an empty entry.
> 
Yes.

>>  {
>>  	struct sg_fd *sfp = container_of(work, struct sg_fd, ew.work);
>>  	struct sg_device *sdp = sfp->parentdp;
>> +	Sg_request *srp, *tmp;
>>  
>>  	/* Cleanup any responses which were never read(). */
>> -	while (sfp->headrp)
>> -		sg_finish_rem_req(sfp->headrp);
>> +	list_for_each_entry_safe(srp, tmp, &sfp->rq_list, nextrp)
>> +		sg_finish_rem_req(srp);
> 
> What protects us from concurrent removals here?
> 
Nothing.
But this patch is intended to just replace the hand-rolled list
implementation, not fixing bugs here.
The problem is that 'sg_finish_rem_req()' is taking the rq_list_lock,
so it needs a bit of rework to make that work properly.
But I'll give it a go.

> Either way I'd rather keep the whіle not empty style even with
> proper lists.
> 
Okay.

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)