diff mbox

sg: clean up gfp_mask in sg_build_indirect

Message ID x49fu1kb3k7.fsf@segfault.boston.devel.redhat.com (mailing list archive)
State Accepted
Headers show

Commit Message

Jeff Moyer June 18, 2018, 1:57 p.m. UTC
commit a45b599ad808c ("scsi: sg: allocate with __GFP_ZERO in
sg_build_indirect()") changed the call to alloc_pages to always use
__GFP_ZERO.  Just above that, though, there was this:

       if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
               gfp_mask |= __GFP_ZERO;

And there's only one user of the gfp_mask.  Just or in the __GFP_ZERO
flag at the top of the function and be done with it.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

p.s. I'm not even sure why the original patch was committed -- it does
nothing for security.  I assume that the zeroing isn't done for
CAP_SYS_ADMIN / CAP_SYS_RAWIO because of performance.  (The history of
that zeroing being conditional goes way back to pre-git.)  Was there any
performance measurement done after the commit a45b599ad808c was applied?

Comments

Douglas Gilbert June 18, 2018, 5:28 p.m. UTC | #1
On 2018-06-18 09:57 AM, Jeff Moyer wrote:
> commit a45b599ad808c ("scsi: sg: allocate with __GFP_ZERO in
> sg_build_indirect()") changed the call to alloc_pages to always use
> __GFP_ZERO.  Just above that, though, there was this:
> 
>         if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
>                 gfp_mask |= __GFP_ZERO;
> 
> And there's only one user of the gfp_mask.  Just or in the __GFP_ZERO
> flag at the top of the function and be done with it.
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

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

> p.s. I'm not even sure why the original patch was committed -- it does
> nothing for security.  I assume that the zeroing isn't done for
> CAP_SYS_ADMIN / CAP_SYS_RAWIO because of performance.

Yes.

> (The history of
> that zeroing being conditional goes way back to pre-git.)  Was there any
> performance measurement done after the commit a45b599ad808c was applied?

No. It was done in the name of security. Alexander Potapenko felt it was
more dangerous to see kernel memory via pseudo-random sequences pushed
to the sg driver's write()/read() async interface than it was to do
'cat /dev/mem'. He suggested some old distros gave non-root access to
/dev/sg* device nodes in some situations.

Then I added that other users of the SG_IO ioctl may have a similar
problem (e.g. ioctl(SG_IO) on a primary block device name and via the
bsg driver). That has lead to more list "noise".

> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 53ae52dbff84..c926e07aabda 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -1850,7 +1850,7 @@ sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
>   	int ret_sz = 0, i, k, rem_sz, num, mx_sc_elems;
>   	int sg_tablesize = sfp->parentdp->sg_tablesize;
>   	int blk_size = buff_size, order;
> -	gfp_t gfp_mask = GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN;
> +	gfp_t gfp_mask = GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN | __GFP_ZERO;
>   	struct sg_device *sdp = sfp->parentdp;
>   
>   	if (blk_size < 0)
> @@ -1880,9 +1880,6 @@ sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
>   	if (sdp->device->host->unchecked_isa_dma)
>   		gfp_mask |= GFP_DMA;
>   
> -	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> -		gfp_mask |= __GFP_ZERO;
> -
>   	order = get_order(num);
>   retry:
>   	ret_sz = 1 << (PAGE_SHIFT + order);
> @@ -1893,7 +1890,7 @@ sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
>   		num = (rem_sz > scatter_elem_sz_prev) ?
>   			scatter_elem_sz_prev : rem_sz;
>   
> -		schp->pages[k] = alloc_pages(gfp_mask | __GFP_ZERO, order);
> +		schp->pages[k] = alloc_pages(gfp_mask, order);
>   		if (!schp->pages[k])
>   			goto out;
>   
>
Martin K. Petersen June 19, 2018, 2:15 a.m. UTC | #2
Jeff,

> And there's only one user of the gfp_mask.  Just or in the __GFP_ZERO
> flag at the top of the function and be done with it.

Applied to 4.19/scsi-queue. Thanks!
diff mbox

Patch

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 53ae52dbff84..c926e07aabda 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1850,7 +1850,7 @@  sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
 	int ret_sz = 0, i, k, rem_sz, num, mx_sc_elems;
 	int sg_tablesize = sfp->parentdp->sg_tablesize;
 	int blk_size = buff_size, order;
-	gfp_t gfp_mask = GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN;
+	gfp_t gfp_mask = GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN | __GFP_ZERO;
 	struct sg_device *sdp = sfp->parentdp;
 
 	if (blk_size < 0)
@@ -1880,9 +1880,6 @@  sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
 	if (sdp->device->host->unchecked_isa_dma)
 		gfp_mask |= GFP_DMA;
 
-	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
-		gfp_mask |= __GFP_ZERO;
-
 	order = get_order(num);
 retry:
 	ret_sz = 1 << (PAGE_SHIFT + order);
@@ -1893,7 +1890,7 @@  sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
 		num = (rem_sz > scatter_elem_sz_prev) ?
 			scatter_elem_sz_prev : rem_sz;
 
-		schp->pages[k] = alloc_pages(gfp_mask | __GFP_ZERO, order);
+		schp->pages[k] = alloc_pages(gfp_mask, order);
 		if (!schp->pages[k])
 			goto out;