* support > 16 byte CDBs for blocklayer SG_IO @ 2014-07-20 10:23 Christoph Hellwig 2014-07-20 10:23 ` [PATCH 1/2] block: cleanup error handling in sg_io Christoph Hellwig 2014-07-20 10:23 ` [PATCH 2/2] block: support > 16 byte CDBs for SG_IO Christoph Hellwig 0 siblings, 2 replies; 6+ messages in thread From: Christoph Hellwig @ 2014-07-20 10:23 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-scsi, linux-kernel Follow the bsg and sg drivers to support large CDBs by allocation the larger than 16 byte CDB array if nessecary. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] block: cleanup error handling in sg_io 2014-07-20 10:23 support > 16 byte CDBs for blocklayer SG_IO Christoph Hellwig @ 2014-07-20 10:23 ` Christoph Hellwig 2014-07-20 10:23 ` [PATCH 2/2] block: support > 16 byte CDBs for SG_IO Christoph Hellwig 1 sibling, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2014-07-20 10:23 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-scsi, linux-kernel Make sure we always clean up through the out label and just have a single place to put the request. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/scsi_ioctl.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c index 14695c6..c4e6633 100644 --- a/block/scsi_ioctl.c +++ b/block/scsi_ioctl.c @@ -272,7 +272,6 @@ static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr, r = blk_rq_unmap_user(bio); if (!ret) ret = r; - blk_put_request(rq); return ret; } @@ -312,10 +311,9 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk, return -ENOMEM; blk_rq_set_block_pc(rq); - if (blk_fill_sghdr_rq(q, rq, hdr, mode)) { - blk_put_request(rq); - return -EFAULT; - } + ret = -EFAULT; + if (blk_fill_sghdr_rq(q, rq, hdr, mode)) + goto out; if (hdr->iovec_count) { size_t iov_data_len; @@ -366,7 +364,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk, hdr->duration = jiffies_to_msecs(jiffies - start_time); - return blk_complete_sghdr_rq(rq, hdr, bio); + ret = blk_complete_sghdr_rq(rq, hdr, bio); out: blk_put_request(rq); return ret; -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] block: support > 16 byte CDBs for SG_IO 2014-07-20 10:23 support > 16 byte CDBs for blocklayer SG_IO Christoph Hellwig 2014-07-20 10:23 ` [PATCH 1/2] block: cleanup error handling in sg_io Christoph Hellwig @ 2014-07-20 10:23 ` Christoph Hellwig 2014-07-20 11:47 ` Boaz Harrosh 1 sibling, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2014-07-20 10:23 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-scsi, linux-kernel Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/scsi_ioctl.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c index c4e6633..a804f3e 100644 --- a/block/scsi_ioctl.c +++ b/block/scsi_ioctl.c @@ -288,8 +288,6 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk, if (hdr->interface_id != 'S') return -EINVAL; - if (hdr->cmd_len > BLK_MAX_CDB) - return -EINVAL; if (hdr->dxfer_len > (queue_max_hw_sectors(q) << 9)) return -EIO; @@ -306,14 +304,21 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk, break; } + ret = -ENOMEM; rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL); if (!rq) - return -ENOMEM; + goto out; blk_rq_set_block_pc(rq); + if (hdr->cmd_len > BLK_MAX_CDB) { + rq->cmd = kzalloc(hdr->cmd_len, GFP_KERNEL); + if (!rq->cmd) + goto out_put_request; + } + ret = -EFAULT; if (blk_fill_sghdr_rq(q, rq, hdr, mode)) - goto out; + goto out_free_cdb; if (hdr->iovec_count) { size_t iov_data_len; @@ -323,7 +328,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk, 0, NULL, &iov); if (ret < 0) { kfree(iov); - goto out; + goto out_free_cdb; } iov_data_len = ret; @@ -346,7 +351,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk, GFP_KERNEL); if (ret) - goto out; + goto out_free_cdb; bio = rq->bio; memset(sense, 0, sizeof(sense)); @@ -365,8 +370,13 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk, hdr->duration = jiffies_to_msecs(jiffies - start_time); ret = blk_complete_sghdr_rq(rq, hdr, bio); -out: + +out_free_cdb: + if (rq->cmd != rq->__cmd) + kfree(rq->cmd); +out_put_request: blk_put_request(rq); +out: return ret; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] block: support > 16 byte CDBs for SG_IO 2014-07-20 10:23 ` [PATCH 2/2] block: support > 16 byte CDBs for SG_IO Christoph Hellwig @ 2014-07-20 11:47 ` Boaz Harrosh 2014-07-20 13:27 ` Christoph Hellwig 0 siblings, 1 reply; 6+ messages in thread From: Boaz Harrosh @ 2014-07-20 11:47 UTC (permalink / raw) To: Christoph Hellwig, Jens Axboe; +Cc: linux-scsi, linux-kernel On 07/20/2014 01:23 PM, Christoph Hellwig wrote: > Signed-off-by: Christoph Hellwig <hch@lst.de> Hi Christoph I've quickly reviewed your code and have a few questions > --- > block/scsi_ioctl.c | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c > index c4e6633..a804f3e 100644 > --- a/block/scsi_ioctl.c > +++ b/block/scsi_ioctl.c > @@ -288,8 +288,6 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk, > > if (hdr->interface_id != 'S') > return -EINVAL; > - if (hdr->cmd_len > BLK_MAX_CDB) > - return -EINVAL; > > if (hdr->dxfer_len > (queue_max_hw_sectors(q) << 9)) > return -EIO; > @@ -306,14 +304,21 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk, > break; > } > > + ret = -ENOMEM; > rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL); > if (!rq) > - return -ENOMEM; > + goto out; > blk_rq_set_block_pc(rq); > > + if (hdr->cmd_len > BLK_MAX_CDB) { > + rq->cmd = kzalloc(hdr->cmd_len, GFP_KERNEL); So two things here: - hdr->cmd_len is char so can be MAX of 255. I understand that 4 bytes alignment is a SCSI thing so you found no point of checking any max size? - Why the zero alloc, if you are going to paste over it the exact same length. Now if like in scsi you need 4 bytes alignment and zero padding yes, but here you do not do this (and probably shouldn't). Hence why zero-alloc? - Looking at sg.h where hdr->cmd_len is defined it stills has a comment of <= 16 you might want to remove the comment by now. > + if (!rq->cmd) > + goto out_put_request; > + } > + > ret = -EFAULT; > if (blk_fill_sghdr_rq(q, rq, hdr, mode)) Inside here: blk_fill_sghdr_rq() calls blk_verify_command() which does: (Below cmd[] is the buffer copied from user) /* Anybody who can open the device can do a read-safe command */ if (test_bit(cmd[0], filter->read_ok)) return 0; /* Write-safe commands require a writable open */ if (test_bit(cmd[0], filter->write_ok) && has_write_perm) return 0; Now I am not sure what type "Commands" you guys intend for these large commands but if they are say SCSI-VARLEN this test will not work. Also a user might fool the system with pretending to be "read" a device modifying command. I would pass len to this test function and only permit these above if command is <= 16. Any "special" large command is root only. Thanks Boaz > - goto out; > + goto out_free_cdb; > > if (hdr->iovec_count) { > size_t iov_data_len; > @@ -323,7 +328,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk, > 0, NULL, &iov); > if (ret < 0) { > kfree(iov); > - goto out; > + goto out_free_cdb; > } > > iov_data_len = ret; > @@ -346,7 +351,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk, > GFP_KERNEL); > > if (ret) > - goto out; > + goto out_free_cdb; > > bio = rq->bio; > memset(sense, 0, sizeof(sense)); > @@ -365,8 +370,13 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk, > hdr->duration = jiffies_to_msecs(jiffies - start_time); > > ret = blk_complete_sghdr_rq(rq, hdr, bio); > -out: > + > +out_free_cdb: > + if (rq->cmd != rq->__cmd) > + kfree(rq->cmd); > +out_put_request: > blk_put_request(rq); > +out: > return ret; > } > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] block: support > 16 byte CDBs for SG_IO 2014-07-20 11:47 ` Boaz Harrosh @ 2014-07-20 13:27 ` Christoph Hellwig 2014-07-20 16:45 ` Boaz Harrosh 0 siblings, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2014-07-20 13:27 UTC (permalink / raw) To: Boaz Harrosh; +Cc: Christoph Hellwig, Jens Axboe, linux-scsi, linux-kernel On Sun, Jul 20, 2014 at 02:47:49PM +0300, Boaz Harrosh wrote: > > So two things here: > - hdr->cmd_len is char so can be MAX of 255. I understand that 4 bytes alignment is a SCSI > thing so you found no point of checking any max size? I don't see any point to force the aligmnet - the devices need to reject garbage commands, and if for some reason we'd see future commands that are > 252 and < 255 we are prepared to handle them. > - Why the zero alloc, if you are going to paste over it the exact same length. Now if like in scsi > you need 4 bytes alignment and zero padding yes, but here you do not do this (and probably shouldn't). > Hence why zero-alloc? No good reason except that's what sg and bsg do. > - Looking at sg.h where hdr->cmd_len is defined it stills has a comment of <= 16 you might want to > remove the comment by now. The sg changes to support > 16 byte CDs remove the comment, take a look at my core-for-3.17 tree which has them applied. > Inside here: blk_fill_sghdr_rq() calls blk_verify_command() which does: > (Below cmd[] is the buffer copied from user) > > /* Anybody who can open the device can do a read-safe command */ > if (test_bit(cmd[0], filter->read_ok)) > return 0; > > /* Write-safe commands require a writable open */ > if (test_bit(cmd[0], filter->write_ok) && has_write_perm) > return 0; > > Now I am not sure what type "Commands" you guys intend for these large commands > but if they are say SCSI-VARLEN this test will not work. Also a user might fool > the system with pretending to be "read" a device modifying command. > > I would pass len to this test function and only permit these above if command is > <= 16. Any "special" large command is root only. Honestly that whole filter crap should never have been merged to start with, you'll just need proper CAP_SYS_RAWIO for variable length commands. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] block: support > 16 byte CDBs for SG_IO 2014-07-20 13:27 ` Christoph Hellwig @ 2014-07-20 16:45 ` Boaz Harrosh 0 siblings, 0 replies; 6+ messages in thread From: Boaz Harrosh @ 2014-07-20 16:45 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jens Axboe, linux-scsi, linux-kernel On 07/20/2014 04:27 PM, Christoph Hellwig wrote: > On Sun, Jul 20, 2014 at 02:47:49PM +0300, Boaz Harrosh wrote: >> >> So two things here: >> - hdr->cmd_len is char so can be MAX of 255. I understand that 4 bytes alignment is a SCSI >> thing so you found no point of checking any max size? > > I don't see any point to force the aligmnet - the devices need to reject > garbage commands, and if for some reason we'd see future commands > that are > 252 and < 255 we are prepared to handle them. > I agree >> - Why the zero alloc, if you are going to paste over it the exact same length. Now if like in scsi >> you need 4 bytes alignment and zero padding yes, but here you do not do this (and probably shouldn't). >> Hence why zero-alloc? > > No good reason except that's what sg and bsg do. > Ha sorry didn't look there. Looks redundant to me that's all <> >> Inside here: blk_fill_sghdr_rq() calls blk_verify_command() which does: >> (Below cmd[] is the buffer copied from user) >> >> /* Anybody who can open the device can do a read-safe command */ >> if (test_bit(cmd[0], filter->read_ok)) >> return 0; >> >> /* Write-safe commands require a writable open */ >> if (test_bit(cmd[0], filter->write_ok) && has_write_perm) >> return 0; >> >> Now I am not sure what type "Commands" you guys intend for these large commands >> but if they are say SCSI-VARLEN this test will not work. Also a user might fool >> the system with pretending to be "read" a device modifying command. >> >> I would pass len to this test function and only permit these above if command is >> <= 16. Any "special" large command is root only. > > Honestly that whole filter crap should never have been merged to start with, > you'll just need proper CAP_SYS_RAWIO for variable length commands. > > I agree. What I'm saying is - Are you sure that with current code as is we will not pass on large commands without the proper CAP_SYS_RAWIO. Should we not make sure and add: /* root can do any command. */ if (capable(CAP_SYS_RAWIO)) return 0; + + if (cmnd_len > BLK_MAX_CDB) + return -EPERM; Rrrr you are right. I finally get the filter code. Anything that is not "white-listed" is rejected. OK sorry for the noise. Reviewed-by: Boaz Harrosh <boaz@plexistor.com> Thanks Boaz ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-20 16:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-07-20 10:23 support > 16 byte CDBs for blocklayer SG_IO Christoph Hellwig 2014-07-20 10:23 ` [PATCH 1/2] block: cleanup error handling in sg_io Christoph Hellwig 2014-07-20 10:23 ` [PATCH 2/2] block: support > 16 byte CDBs for SG_IO Christoph Hellwig 2014-07-20 11:47 ` Boaz Harrosh 2014-07-20 13:27 ` Christoph Hellwig 2014-07-20 16:45 ` Boaz Harrosh
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.