| Message ID | x49fu1kb3k7.fsf@segfault.boston.devel.redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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; > >
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 --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;
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?