[PATCH] block: fix sg SG_DXFER_TO_FROM_DEV regression

[PATCH] block: fix sg SG_DXFER_TO_FROM_DEV regression

[FUJITA Tomonori]

I overlooked SG_DXFER_TO_FROM_DEV support when I converted sg to use
the block layer mapping API (2.6.28).

Douglas Gilbert explained SG_DXFER_TO_FROM_DEV:

http://www.spinics.net/lists/linux-scsi/msg37135.html

=
The semantics of SG_DXFER_TO_FROM_DEV were:
   - copy user space buffer to kernel (LLD) buffer
   - do SCSI command which is assumed to be of the DATA_IN
     (data from device) variety. This would overwrite
     some or all of the kernel buffer
   - copy kernel (LLD) buffer back to the user space.

The idea was to detect short reads by filling the original
user space buffer with some marker bytes ("0xec" it would
seem in this report). The "resid" value is a better way
of detecting short reads but that was only added this century
and requires co-operation from the LLD.
=

This patch changes the block layer mapping API to support this
semantics. This simply adds another field to struct rq_map_data and
enables __bio_copy_iov() to copy data from user space even with READ
requests.

It's better to add the flags field and kills null_mapped and the new
from_user fields in struct rq_map_data but that approach makes it
difficult to send this patch to stable trees because st and osst
drivers use struct rq_map_data (they were converted to use the block
layer in 2.6.29 and 2.6.30). Well, I should clean up the block layer
mapping API.

zhou sf reported this regiression and tested this patch:

http://www.spinics.net/lists/linux-scsi/msg37128.html
http://www.spinics.net/lists/linux-scsi/msg37168.html

Reported-by: zhou sf <sxzzsf@gmail.com>
Tested-by: zhou sf <sxzzsf@gmail.com>
Cc: stable@kernel.org
Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/sg.c      |    4 ++++
 fs/bio.c               |   22 ++++++++++++----------
 include/linux/blkdev.h |    1 +
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index ef142fd..02e2db0 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1656,6 +1656,10 @@ static int sg_start_req(Sg_request *srp, unsigned char *cmd)
 		md->nr_entries = req_schp->k_use_sg;
 		md->offset = 0;
 		md->null_mapped = hp->dxferp ? 0 : 1;
+		if (dxfer_dir == SG_DXFER_TO_FROM_DEV)
+			md->from_user = 1;
+		else
+			md->from_user = 0;
 	}
 
 	if (iov_count) {
diff --git a/fs/bio.c b/fs/bio.c
index 1486b19..7673800 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -705,14 +705,13 @@ static struct bio_map_data *bio_alloc_map_data(int nr_segs, int iov_count,
 }
 
 static int __bio_copy_iov(struct bio *bio, struct bio_vec *iovecs,
-			  struct sg_iovec *iov, int iov_count, int uncopy,
-			  int do_free_page)
+			  struct sg_iovec *iov, int iov_count,
+			  int to_user, int from_user, int do_free_page)
 {
 	int ret = 0, i;
 	struct bio_vec *bvec;
 	int iov_idx = 0;
 	unsigned int iov_off = 0;
-	int read = bio_data_dir(bio) == READ;
 
 	__bio_for_each_segment(bvec, bio, i, 0) {
 		char *bv_addr = page_address(bvec->bv_page);
@@ -727,13 +726,14 @@ static int __bio_copy_iov(struct bio *bio, struct bio_vec *iovecs,
 			iov_addr = iov[iov_idx].iov_base + iov_off;
 
 			if (!ret) {
-				if (!read && !uncopy)
-					ret = copy_from_user(bv_addr, iov_addr,
-							     bytes);
-				if (read && uncopy)
+				if (to_user)
 					ret = copy_to_user(iov_addr, bv_addr,
 							   bytes);
 
+				if (from_user)
+					ret = copy_from_user(bv_addr, iov_addr,
+							     bytes);
+
 				if (ret)
 					ret = -EFAULT;
 			}
@@ -770,7 +770,8 @@ int bio_uncopy_user(struct bio *bio)
 
 	if (!bio_flagged(bio, BIO_NULL_MAPPED))
 		ret = __bio_copy_iov(bio, bmd->iovecs, bmd->sgvecs,
-				     bmd->nr_sgvecs, 1, bmd->is_our_pages);
+				     bmd->nr_sgvecs, bio_data_dir(bio) == READ,
+				     0, bmd->is_our_pages);
 	bio_free_map_data(bmd);
 	bio_put(bio);
 	return ret;
@@ -875,8 +876,9 @@ struct bio *bio_copy_user_iov(struct request_queue *q,
 	/*
 	 * success
 	 */
-	if (!write_to_vm && (!map_data || !map_data->null_mapped)) {
-		ret = __bio_copy_iov(bio, bio->bi_io_vec, iov, iov_count, 0, 0);
+	if ((!write_to_vm && (!map_data || !map_data->null_mapped)) ||
+	    (map_data && map_data->from_user)) {
+		ret = __bio_copy_iov(bio, bio->bi_io_vec, iov, iov_count, 0, 1, 0);
 		if (ret)
 			goto cleanup;
 	}
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 49ae079..7e9c417 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -723,6 +723,7 @@ struct rq_map_data {
 	int nr_entries;
 	unsigned long offset;
 	int null_mapped;
+	int from_user;
 };
 
 struct req_iterator {
-- 
1.6.0.6

Re: [PATCH] block: fix sg SG_DXFER_TO_FROM_DEV regression

[Douglas Gilbert]

FUJITA Tomonori wrote:
> I overlooked SG_DXFER_TO_FROM_DEV support when I converted sg to use
> the block layer mapping API (2.6.28).
> 
> Douglas Gilbert explained SG_DXFER_TO_FROM_DEV:
> 
> http://www.spinics.net/lists/linux-scsi/msg37135.html
> 
> =
> The semantics of SG_DXFER_TO_FROM_DEV were:
>    - copy user space buffer to kernel (LLD) buffer
>    - do SCSI command which is assumed to be of the DATA_IN
>      (data from device) variety. This would overwrite
>      some or all of the kernel buffer
>    - copy kernel (LLD) buffer back to the user space.
> 
> The idea was to detect short reads by filling the original
> user space buffer with some marker bytes ("0xec" it would
> seem in this report). The "resid" value is a better way
> of detecting short reads but that was only added this century
> and requires co-operation from the LLD.
> =
> 
> This patch changes the block layer mapping API to support this
> semantics. This simply adds another field to struct rq_map_data and
> enables __bio_copy_iov() to copy data from user space even with READ
> requests.
> 
> It's better to add the flags field and kills null_mapped and the new
> from_user fields in struct rq_map_data but that approach makes it
> difficult to send this patch to stable trees because st and osst
> drivers use struct rq_map_data (they were converted to use the block
> layer in 2.6.29 and 2.6.30). Well, I should clean up the block layer
> mapping API.
> 
> zhou sf reported this regiression and tested this patch:
> 
> http://www.spinics.net/lists/linux-scsi/msg37128.html
> http://www.spinics.net/lists/linux-scsi/msg37168.html

Wrote my own little test program and it gave the wrong
result (as reported by zhou sf) in lk 2.6.30 . When
this patch was applied, the expected result (i.e. a
response with user supplied fill characters) was observed.

Here is an INQUIRY response set up for a length of
128 bytes with an 0xec fill character:

$ ./sg_dxfer_to_from
resid indicates that 96 bytes returned in response
  00     00 00 05 02 5b 00 10 0a  4c 69 6e 75 78 20 20 20
  10     73 63 73 69 5f 64 65 62  75 67 20 20 20 20 20 20
  20     30 30 30 34 00 00 00 00  00 00 00 00 00 00 00 00
  30     00 00 00 00 00 00 00 00  00 00 00 77 03 14 03 3d
  40     0c 0f 00 00 00 00 00 00  00 00 00 00 00 00 00 00
  50     00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
  60     ec ec ec ec ec ec ec ec  ec ec ec ec ec ec ec ec
  70     ec ec ec ec ec ec ec ec  ec ec ec ec ec ec ec ec


Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>

Re: [PATCH] block: fix sg SG_DXFER_TO_FROM_DEV regression

[FUJITA Tomonori]

On Thu, 09 Jul 2009 02:19:22 +0200
Douglas Gilbert <dgilbert@interlog.com> wrote:

> FUJITA Tomonori wrote:
> > I overlooked SG_DXFER_TO_FROM_DEV support when I converted sg to use
> > the block layer mapping API (2.6.28).
> > 
> > Douglas Gilbert explained SG_DXFER_TO_FROM_DEV:
> > 
> > http://www.spinics.net/lists/linux-scsi/msg37135.html
> > 
> > =
> > The semantics of SG_DXFER_TO_FROM_DEV were:
> >    - copy user space buffer to kernel (LLD) buffer
> >    - do SCSI command which is assumed to be of the DATA_IN
> >      (data from device) variety. This would overwrite
> >      some or all of the kernel buffer
> >    - copy kernel (LLD) buffer back to the user space.
> > 
> > The idea was to detect short reads by filling the original
> > user space buffer with some marker bytes ("0xec" it would
> > seem in this report). The "resid" value is a better way
> > of detecting short reads but that was only added this century
> > and requires co-operation from the LLD.
> > =
> > 
> > This patch changes the block layer mapping API to support this
> > semantics. This simply adds another field to struct rq_map_data and
> > enables __bio_copy_iov() to copy data from user space even with READ
> > requests.
> > 
> > It's better to add the flags field and kills null_mapped and the new
> > from_user fields in struct rq_map_data but that approach makes it
> > difficult to send this patch to stable trees because st and osst
> > drivers use struct rq_map_data (they were converted to use the block
> > layer in 2.6.29 and 2.6.30). Well, I should clean up the block layer
> > mapping API.
> > 
> > zhou sf reported this regiression and tested this patch:
> > 
> > http://www.spinics.net/lists/linux-scsi/msg37128.html
> > http://www.spinics.net/lists/linux-scsi/msg37168.html
> 
> Wrote my own little test program and it gave the wrong
> result (as reported by zhou sf) in lk 2.6.30 . When
> this patch was applied, the expected result (i.e. a
> response with user supplied fill characters) was observed.
> 
> Here is an INQUIRY response set up for a length of
> 128 bytes with an 0xec fill character:
> 
> $ ./sg_dxfer_to_from
> resid indicates that 96 bytes returned in response
>   00     00 00 05 02 5b 00 10 0a  4c 69 6e 75 78 20 20 20
>   10     73 63 73 69 5f 64 65 62  75 67 20 20 20 20 20 20
>   20     30 30 30 34 00 00 00 00  00 00 00 00 00 00 00 00
>   30     00 00 00 00 00 00 00 00  00 00 00 77 03 14 03 3d
>   40     0c 0f 00 00 00 00 00 00  00 00 00 00 00 00 00 00
>   50     00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
>   60     ec ec ec ec ec ec ec ec  ec ec ec ec ec ec ec ec
>   70     ec ec ec ec ec ec ec ec  ec ec ec ec ec ec ec ec
> 
> 
> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>

Thanks for testing!

Jens, please apply this patch.

Re: [PATCH] block: fix sg SG_DXFER_TO_FROM_DEV regression

[Jens Axboe]

On Thu, Jul 09 2009, FUJITA Tomonori wrote:
> On Thu, 09 Jul 2009 02:19:22 +0200
> Douglas Gilbert <dgilbert@interlog.com> wrote:
> 
> > FUJITA Tomonori wrote:
> > > I overlooked SG_DXFER_TO_FROM_DEV support when I converted sg to use
> > > the block layer mapping API (2.6.28).
> > > 
> > > Douglas Gilbert explained SG_DXFER_TO_FROM_DEV:
> > > 
> > > http://www.spinics.net/lists/linux-scsi/msg37135.html
> > > 
> > > =
> > > The semantics of SG_DXFER_TO_FROM_DEV were:
> > >    - copy user space buffer to kernel (LLD) buffer
> > >    - do SCSI command which is assumed to be of the DATA_IN
> > >      (data from device) variety. This would overwrite
> > >      some or all of the kernel buffer
> > >    - copy kernel (LLD) buffer back to the user space.
> > > 
> > > The idea was to detect short reads by filling the original
> > > user space buffer with some marker bytes ("0xec" it would
> > > seem in this report). The "resid" value is a better way
> > > of detecting short reads but that was only added this century
> > > and requires co-operation from the LLD.
> > > =
> > > 
> > > This patch changes the block layer mapping API to support this
> > > semantics. This simply adds another field to struct rq_map_data and
> > > enables __bio_copy_iov() to copy data from user space even with READ
> > > requests.
> > > 
> > > It's better to add the flags field and kills null_mapped and the new
> > > from_user fields in struct rq_map_data but that approach makes it
> > > difficult to send this patch to stable trees because st and osst
> > > drivers use struct rq_map_data (they were converted to use the block
> > > layer in 2.6.29 and 2.6.30). Well, I should clean up the block layer
> > > mapping API.
> > > 
> > > zhou sf reported this regiression and tested this patch:
> > > 
> > > http://www.spinics.net/lists/linux-scsi/msg37128.html
> > > http://www.spinics.net/lists/linux-scsi/msg37168.html
> > 
> > Wrote my own little test program and it gave the wrong
> > result (as reported by zhou sf) in lk 2.6.30 . When
> > this patch was applied, the expected result (i.e. a
> > response with user supplied fill characters) was observed.
> > 
> > Here is an INQUIRY response set up for a length of
> > 128 bytes with an 0xec fill character:
> > 
> > $ ./sg_dxfer_to_from
> > resid indicates that 96 bytes returned in response
> >   00     00 00 05 02 5b 00 10 0a  4c 69 6e 75 78 20 20 20
> >   10     73 63 73 69 5f 64 65 62  75 67 20 20 20 20 20 20
> >   20     30 30 30 34 00 00 00 00  00 00 00 00 00 00 00 00
> >   30     00 00 00 00 00 00 00 00  00 00 00 77 03 14 03 3d
> >   40     0c 0f 00 00 00 00 00 00  00 00 00 00 00 00 00 00
> >   50     00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
> >   60     ec ec ec ec ec ec ec ec  ec ec ec ec ec ec ec ec
> >   70     ec ec ec ec ec ec ec ec  ec ec ec ec ec ec ec ec
> > 
> > 
> > Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
> 
> Thanks for testing!
> 
> Jens, please apply this patch.

Applied


-- 
Jens Axboe