sg: Free data buffers after calling blk_rq_unmap_user

sg: Free data buffers after calling blk_rq_unmap_user

[Christof Schmitt]

This patch fixes a problem with the sg driver that is only visible
with the CONFIG_DEBUG_PAGEALLOC kernel config option. The patch works
for me, but i would appreciate the review of somebody with more
knowledge about the interactions between the sg driver and the block
layer.

Christof
---
sg: Free data buffers after calling blk_rq_unmap_user

From: Christof Schmitt <christof.schmitt@de.ibm.com>

Running sg_luns on s390x with CONFIG_DEBUG_PAGEALLOC enabled fails
with EFAULT from the SG_IO ioctl. The EFAULT is the result from
copy_to_user failing in this call chain:

sg_ioctl
sg_new_read
sg_finish_rem_req
blk_rq_unmap_user
__blk_rq_unmap_user
bio_uncopy_user
__bio_copy_iov
copy_to_user

The sg driver calls sg_remove_scat to free the memory pages before
calling blk_rq_unmap_user that tries to copy the data back to
userspace. Change the order to first call blk_rq_unmap_user before
freeing the pages in sg_remove_scat.

Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
---
 drivers/scsi/sg.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

--- a/drivers/scsi/sg.c	2009-09-15 18:18:11.000000000 +0200
+++ b/drivers/scsi/sg.c	2009-09-15 18:18:45.000000000 +0200
@@ -1708,11 +1708,6 @@ static int sg_finish_rem_req(Sg_request 
 	Sg_scatter_hold *req_schp = &srp->data;
 
 	SCSI_LOG_TIMEOUT(4, printk("sg_finish_rem_req: res_used=%d\n", (int) srp->res_used));
-	if (srp->res_used)
-		sg_unlink_reserve(sfp, srp);
-	else
-		sg_remove_scat(req_schp);
-
 	if (srp->rq) {
 		if (srp->bio)
 			ret = blk_rq_unmap_user(srp->bio);
@@ -1720,6 +1715,11 @@ static int sg_finish_rem_req(Sg_request 
 		blk_put_request(srp->rq);
 	}
 
+	if (srp->res_used)
+		sg_unlink_reserve(sfp, srp);
+	else
+		sg_remove_scat(req_schp);
+
 	sg_remove_request(sfp, srp);
 
 	return ret;

Re: sg: Free data buffers after calling blk_rq_unmap_user

[FUJITA Tomonori]

Sorry for the delay,

On Tue, 15 Sep 2009 18:53:57 +0200
Christof Schmitt <christof.schmitt@de.ibm.com> wrote:

> This patch fixes a problem with the sg driver that is only visible
> with the CONFIG_DEBUG_PAGEALLOC kernel config option. The patch works
> for me, but i would appreciate the review of somebody with more
> knowledge about the interactions between the sg driver and the block
> layer.
> 
> Christof
> ---
> sg: Free data buffers after calling blk_rq_unmap_user
> 
> From: Christof Schmitt <christof.schmitt@de.ibm.com>
> 
> Running sg_luns on s390x with CONFIG_DEBUG_PAGEALLOC enabled fails
> with EFAULT from the SG_IO ioctl. The EFAULT is the result from
> copy_to_user failing in this call chain:
> 
> sg_ioctl
> sg_new_read
> sg_finish_rem_req
> blk_rq_unmap_user
> __blk_rq_unmap_user
> bio_uncopy_user
> __bio_copy_iov
> copy_to_user
> 
> The sg driver calls sg_remove_scat to free the memory pages before
> calling blk_rq_unmap_user that tries to copy the data back to
> userspace. Change the order to first call blk_rq_unmap_user before
> freeing the pages in sg_remove_scat.
> 
> Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
> ---
>  drivers/scsi/sg.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Oops, thanks a lot!

Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

It would be better to add "Cc: stable@kernel.org" tag since this fix
needs to be sent to stable trees.

Re: sg: Free data buffers after calling blk_rq_unmap_user

[Christof Schmitt]

On Thu, Sep 17, 2009 at 03:00:44PM +0900, FUJITA Tomonori wrote:
> Sorry for the delay,
> 
> On Tue, 15 Sep 2009 18:53:57 +0200
> Christof Schmitt <christof.schmitt@de.ibm.com> wrote:
> 
> > This patch fixes a problem with the sg driver that is only visible
> > with the CONFIG_DEBUG_PAGEALLOC kernel config option. The patch works
> > for me, but i would appreciate the review of somebody with more
> > knowledge about the interactions between the sg driver and the block
> > layer.
> > 
> > Christof
> > ---
> > sg: Free data buffers after calling blk_rq_unmap_user
> > 
> > From: Christof Schmitt <christof.schmitt@de.ibm.com>
> > 
> > Running sg_luns on s390x with CONFIG_DEBUG_PAGEALLOC enabled fails
> > with EFAULT from the SG_IO ioctl. The EFAULT is the result from
> > copy_to_user failing in this call chain:
> > 
> > sg_ioctl
> > sg_new_read
> > sg_finish_rem_req
> > blk_rq_unmap_user
> > __blk_rq_unmap_user
> > bio_uncopy_user
> > __bio_copy_iov
> > copy_to_user
> > 
> > The sg driver calls sg_remove_scat to free the memory pages before
> > calling blk_rq_unmap_user that tries to copy the data back to
> > userspace. Change the order to first call blk_rq_unmap_user before
> > freeing the pages in sg_remove_scat.
> > 
> > Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
> > ---
> >  drivers/scsi/sg.c |   10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Oops, thanks a lot!
> 
> Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> 
> It would be better to add "Cc: stable@kernel.org" tag since this fix
> needs to be sent to stable trees.

Thanks for the review. I will resend the patch with the added acked-by
and cc.

--
Christof