* VLA removal, device_handler and COMMAND_SIZE
@ 2018-03-09 22:29 Stephen Kitt
2018-03-09 22:32 ` [PATCH] device_handler: remove VLAs Stephen Kitt
2018-03-09 22:33 ` [PATCH] scsi: resolve COMMAND_SIZE at compile time Stephen Kitt
0 siblings, 2 replies; 14+ messages in thread
From: Stephen Kitt @ 2018-03-09 22:29 UTC (permalink / raw)
To: Jens Axboe, James E.J. Bottomley, Martin K. Petersen, Hannes Reinecke
Cc: linux-block, linux-scsi, linux-kernel, Kernel Hardening
[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]
Hi,
I’ve been looking into removing some VLAs from device_handler drivers,
prompted by https://lkml.org/lkml/2018/3/7/621
The uses in question here are quite straightforward, e.g. in
drivers/scsi/device_handler/scsi_dh_alua.c:
u8 cdb[COMMAND_SIZE(MAINTENANCE_IN)];
There’s no trivial way of keeping the compiler happy with -Wvla though here,
at least not while keeping the behaviour strictly identical. I’ve come up
with two approaches, and I’m curious whether they’re appropriate or if
there’s a better way...
The first approach is to use MAX_COMMAND_SIZE instead; this wastes a few
bytes on the stack here and there, and stays reasonably maintainable.
The second approach might be symptomatic of a twisted mind, and involves
replacing COMMAND_SIZE so that it can be calculated at compile time when the
opcode is known:
/*
* SCSI command sizes are as follows, in bytes, for fixed size commands, per
* group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode
* determine its group.
* The size table is encoded into a 32-bit value by subtracting each value
* from 16, resulting in a value of 1715488362
* (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + 10).
* Command group 3 is reserved and should never be used.
*/
#define COMMAND_SIZE(opcode) \
(16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7)))))
This has the side-effect of making some of the call sites more complex, and
the macro itself isn’t the most maintainer-friendly. It does mean we can drop
BLK_SCSI_REQUEST from drivers/target/Kconfig so it’s not all bad...
Both patches will follow in reply to this email, I’ll let more familiar
developers judge which is appropriate (if any).
Regards,
Stephen
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH] device_handler: remove VLAs 2018-03-09 22:29 VLA removal, device_handler and COMMAND_SIZE Stephen Kitt @ 2018-03-09 22:32 ` Stephen Kitt 2018-03-09 22:48 ` Bart Van Assche ` (2 more replies) 2018-03-09 22:33 ` [PATCH] scsi: resolve COMMAND_SIZE at compile time Stephen Kitt 1 sibling, 3 replies; 14+ messages in thread From: Stephen Kitt @ 2018-03-09 22:32 UTC (permalink / raw) To: hare, axboe, jejb, martin.petersen Cc: linux-scsi, kernel-hardening, linux-kernel, linux-block, Stephen Kitt In preparation to enabling -Wvla, remove VLAs and replace them with fixed-length arrays instead. scsi_dh_{alua,emc,rdac} use variable-length array declarations to store command blocks, with the appropriate size as determined by COMMAND_SIZE. This patch replaces these with fixed-sized arrays using MAX_COMMAND_SIZE, so that the array size can be determined at compile time. This was prompted by https://lkml.org/lkml/2018/3/7/621 Signed-off-by: Stephen Kitt <steve@sk2.org> --- drivers/scsi/device_handler/scsi_dh_alua.c | 8 ++++---- drivers/scsi/device_handler/scsi_dh_emc.c | 2 +- drivers/scsi/device_handler/scsi_dh_rdac.c | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c index 4b44325d1a82..982a52528a1c 100644 --- a/drivers/scsi/device_handler/scsi_dh_alua.c +++ b/drivers/scsi/device_handler/scsi_dh_alua.c @@ -138,12 +138,12 @@ static void release_port_group(struct kref *kref) static int submit_rtpg(struct scsi_device *sdev, unsigned char *buff, int bufflen, struct scsi_sense_hdr *sshdr, int flags) { - u8 cdb[COMMAND_SIZE(MAINTENANCE_IN)]; + u8 cdb[MAX_COMMAND_SIZE]; int req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT | REQ_FAILFAST_DRIVER; /* Prepare the command. */ - memset(cdb, 0x0, COMMAND_SIZE(MAINTENANCE_IN)); + memset(cdb, 0x0, MAX_COMMAND_SIZE); cdb[0] = MAINTENANCE_IN; if (!(flags & ALUA_RTPG_EXT_HDR_UNSUPP)) cdb[1] = MI_REPORT_TARGET_PGS | MI_EXT_HDR_PARAM_FMT; @@ -166,7 +166,7 @@ static int submit_rtpg(struct scsi_device *sdev, unsigned char *buff, static int submit_stpg(struct scsi_device *sdev, int group_id, struct scsi_sense_hdr *sshdr) { - u8 cdb[COMMAND_SIZE(MAINTENANCE_OUT)]; + u8 cdb[MAX_COMMAND_SIZE]; unsigned char stpg_data[8]; int stpg_len = 8; int req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT | @@ -178,7 +178,7 @@ static int submit_stpg(struct scsi_device *sdev, int group_id, put_unaligned_be16(group_id, &stpg_data[6]); /* Prepare the command. */ - memset(cdb, 0x0, COMMAND_SIZE(MAINTENANCE_OUT)); + memset(cdb, 0x0, MAX_COMMAND_SIZE); cdb[0] = MAINTENANCE_OUT; cdb[1] = MO_SET_TARGET_PGS; put_unaligned_be32(stpg_len, &cdb[6]); diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c index 6a2792f3a37e..95c47909a58f 100644 --- a/drivers/scsi/device_handler/scsi_dh_emc.c +++ b/drivers/scsi/device_handler/scsi_dh_emc.c @@ -249,7 +249,7 @@ static int send_trespass_cmd(struct scsi_device *sdev, struct clariion_dh_data *csdev) { unsigned char *page22; - unsigned char cdb[COMMAND_SIZE(MODE_SELECT)]; + unsigned char cdb[MAX_COMMAND_SIZE]; int err, res = SCSI_DH_OK, len; struct scsi_sense_hdr sshdr; u64 req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT | diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c index 7af31a1247ee..d27fabae8ddd 100644 --- a/drivers/scsi/device_handler/scsi_dh_rdac.c +++ b/drivers/scsi/device_handler/scsi_dh_rdac.c @@ -533,7 +533,7 @@ static void send_mode_select(struct work_struct *work) int err = SCSI_DH_OK, retry_cnt = RDAC_RETRY_COUNT; struct rdac_queue_data *tmp, *qdata; LIST_HEAD(list); - unsigned char cdb[COMMAND_SIZE(MODE_SELECT_10)]; + unsigned char cdb[MAX_COMMAND_SIZE]; struct scsi_sense_hdr sshdr; unsigned int data_size; u64 req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT | -- 2.11.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] device_handler: remove VLAs 2018-03-09 22:32 ` [PATCH] device_handler: remove VLAs Stephen Kitt @ 2018-03-09 22:48 ` Bart Van Assche 2018-03-10 13:14 ` Stephen Kitt 2018-03-12 6:25 ` Hannes Reinecke 2018-03-13 2:37 ` Martin K. Petersen 2 siblings, 1 reply; 14+ messages in thread From: Bart Van Assche @ 2018-03-09 22:48 UTC (permalink / raw) To: jejb, steve, hare, martin.petersen, axboe Cc: linux-scsi, linux-kernel, linux-block, kernel-hardening On Fri, 2018-03-09 at 23:32 +0100, Stephen Kitt wrote: > In preparation to enabling -Wvla, remove VLAs and replace them with > fixed-length arrays instead. > > scsi_dh_{alua,emc,rdac} use variable-length array declarations to > store command blocks, with the appropriate size as determined by > COMMAND_SIZE. This patch replaces these with fixed-sized arrays using > MAX_COMMAND_SIZE, so that the array size can be determined at compile > time. If COMMAND_SIZE() is evaluated at compile time, do we still need this patch? Thanks, Bart. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] device_handler: remove VLAs 2018-03-09 22:48 ` Bart Van Assche @ 2018-03-10 13:14 ` Stephen Kitt 2018-03-12 15:41 ` Bart Van Assche 0 siblings, 1 reply; 14+ messages in thread From: Stephen Kitt @ 2018-03-10 13:14 UTC (permalink / raw) To: Bart Van Assche Cc: jejb, hare, martin.petersen, axboe, linux-scsi, linux-kernel, linux-block, kernel-hardening [-- Attachment #1: Type: text/plain, Size: 1050 bytes --] Hi Bart, On Fri, 9 Mar 2018 22:48:10 +0000, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Fri, 2018-03-09 at 23:32 +0100, Stephen Kitt wrote: > > In preparation to enabling -Wvla, remove VLAs and replace them with > > fixed-length arrays instead. > > > > scsi_dh_{alua,emc,rdac} use variable-length array declarations to > > store command blocks, with the appropriate size as determined by > > COMMAND_SIZE. This patch replaces these with fixed-sized arrays using > > MAX_COMMAND_SIZE, so that the array size can be determined at compile > > time. > > If COMMAND_SIZE() is evaluated at compile time, do we still need this patch? The two patches I sent were supposed to be alternative solutions; see https://marc.info/?l=linux-scsi&m=152063671005295&w=2 for the introduction (I seem to have messed up the headers, so the mails didn’t end up threaded properly). The MAX_COMMAND_SIZE approach is nice and simple, but I thought it worth eliminating scsi_command_size_tbl while I was at it... Regards, Stephen [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] device_handler: remove VLAs 2018-03-10 13:14 ` Stephen Kitt @ 2018-03-12 15:41 ` Bart Van Assche 2018-03-12 19:26 ` Stephen Kitt 0 siblings, 1 reply; 14+ messages in thread From: Bart Van Assche @ 2018-03-12 15:41 UTC (permalink / raw) To: steve Cc: linux-kernel, linux-block, martin.petersen, axboe, linux-scsi, hare, kernel-hardening, jejb On Sat, 2018-03-10 at 14:14 +0100, Stephen Kitt wrote: > The two patches I sent were supposed to be alternative solutions; see > https://marc.info/?l=linux-scsi&m=152063671005295&w=2 for the introduction (I > seem to have messed up the headers, so the mails didn’t end up threaded > properly). The two patches arrived in my inbox several minutes before the cover letter. In the e-mail header of the cover letter I found the following: X-Greylist: delayed 1810 seconds by postgrey-1.27 at vger.kernel.org; Fri, 09 Mar 2018 18:05:08 EST Does this mean that the delay happened due to vger server's anti-spam algorithm? Bart. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] device_handler: remove VLAs 2018-03-12 15:41 ` Bart Van Assche @ 2018-03-12 19:26 ` Stephen Kitt 0 siblings, 0 replies; 14+ messages in thread From: Stephen Kitt @ 2018-03-12 19:26 UTC (permalink / raw) To: Bart Van Assche Cc: linux-kernel, linux-block, martin.petersen, axboe, linux-scsi, hare, kernel-hardening, jejb [-- Attachment #1: Type: text/plain, Size: 841 bytes --] On Mon, 12 Mar 2018 15:41:26 +0000, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Sat, 2018-03-10 at 14:14 +0100, Stephen Kitt wrote: > > The two patches I sent were supposed to be alternative solutions; see > > https://marc.info/?l=linux-scsi&m=152063671005295&w=2 for the > > introduction (I seem to have messed up the headers, so the mails didn’t > > end up threaded properly). > > The two patches arrived in my inbox several minutes before the cover > letter. In the e-mail header of the cover letter I found the following: > > X-Greylist: delayed 1810 seconds by postgrey-1.27 at vger.kernel.org; Fri, > 09 Mar 2018 18:05:08 EST > > Does this mean that the delay happened due to vger server's anti-spam > algorithm? That’s right, the greylisting part of its anti-spam measures. Regards, Stephen [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] device_handler: remove VLAs 2018-03-09 22:32 ` [PATCH] device_handler: remove VLAs Stephen Kitt 2018-03-09 22:48 ` Bart Van Assche @ 2018-03-12 6:25 ` Hannes Reinecke 2018-03-13 2:37 ` Martin K. Petersen 2 siblings, 0 replies; 14+ messages in thread From: Hannes Reinecke @ 2018-03-12 6:25 UTC (permalink / raw) To: Stephen Kitt, hare, axboe, jejb, martin.petersen Cc: linux-scsi, kernel-hardening, linux-kernel, linux-block On 03/09/2018 11:32 PM, Stephen Kitt wrote: > In preparation to enabling -Wvla, remove VLAs and replace them with > fixed-length arrays instead. > > scsi_dh_{alua,emc,rdac} use variable-length array declarations to > store command blocks, with the appropriate size as determined by > COMMAND_SIZE. This patch replaces these with fixed-sized arrays using > MAX_COMMAND_SIZE, so that the array size can be determined at compile > time. > > This was prompted by https://lkml.org/lkml/2018/3/7/621 > > Signed-off-by: Stephen Kitt <steve@sk2.org> > --- > drivers/scsi/device_handler/scsi_dh_alua.c | 8 ++++---- > drivers/scsi/device_handler/scsi_dh_emc.c | 2 +- > drivers/scsi/device_handler/scsi_dh_rdac.c | 2 +- > 3 files changed, 6 insertions(+), 6 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] device_handler: remove VLAs 2018-03-09 22:32 ` [PATCH] device_handler: remove VLAs Stephen Kitt 2018-03-09 22:48 ` Bart Van Assche 2018-03-12 6:25 ` Hannes Reinecke @ 2018-03-13 2:37 ` Martin K. Petersen 2 siblings, 0 replies; 14+ messages in thread From: Martin K. Petersen @ 2018-03-13 2:37 UTC (permalink / raw) To: Stephen Kitt Cc: hare, axboe, jejb, martin.petersen, linux-scsi, kernel-hardening, linux-kernel, linux-block Stephen, > In preparation to enabling -Wvla, remove VLAs and replace them with > fixed-length arrays instead. > > scsi_dh_{alua,emc,rdac} use variable-length array declarations to > store command blocks, with the appropriate size as determined by > COMMAND_SIZE. This patch replaces these with fixed-sized arrays using > MAX_COMMAND_SIZE, so that the array size can be determined at compile > time. Applied to 4.17/scsi-queue. Thank you! -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] scsi: resolve COMMAND_SIZE at compile time 2018-03-09 22:29 VLA removal, device_handler and COMMAND_SIZE Stephen Kitt 2018-03-09 22:32 ` [PATCH] device_handler: remove VLAs Stephen Kitt @ 2018-03-09 22:33 ` Stephen Kitt 2018-03-09 22:47 ` Bart Van Assche 2018-03-13 11:34 ` David Laight 1 sibling, 2 replies; 14+ messages in thread From: Stephen Kitt @ 2018-03-09 22:33 UTC (permalink / raw) To: hare, axboe, jejb, martin.petersen Cc: linux-scsi, kernel-hardening, linux-kernel, linux-block, Stephen Kitt COMMAND_SIZE currently uses an array of values in block/scsi_ioctl.c. A number of device_handler functions use this to initialise arrays, and this is flagged by -Wvla. This patch replaces COMMAND_SIZE with a variant using a formula which can be resolved at compile time in cases where the opcode is fixed, resolving the array size and avoiding the VLA. The downside is that the code is less maintainable and that some call sites end up having more complex generated code. Since scsi_command_size_tbl is dropped, we can remove the dependency on BLK_SCSI_REQUEST from drivers/target/Kconfig. This was prompted by https://lkml.org/lkml/2018/3/7/621 Signed-off-by: Stephen Kitt <steve@sk2.org> --- block/scsi_ioctl.c | 8 -------- drivers/target/Kconfig | 1 - include/scsi/scsi_common.h | 13 +++++++++++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c index 60b471f8621b..b9e176e537be 100644 --- a/block/scsi_ioctl.c +++ b/block/scsi_ioctl.c @@ -41,14 +41,6 @@ struct blk_cmd_filter { static struct blk_cmd_filter blk_default_cmd_filter; -/* Command group 3 is reserved and should never be used. */ -const unsigned char scsi_command_size_tbl[8] = -{ - 6, 10, 10, 12, - 16, 12, 10, 10 -}; -EXPORT_SYMBOL(scsi_command_size_tbl); - #include <scsi/sg.h> static int sg_get_version(int __user *p) diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig index 4c44d7bed01a..b5f05b60cf06 100644 --- a/drivers/target/Kconfig +++ b/drivers/target/Kconfig @@ -4,7 +4,6 @@ menuconfig TARGET_CORE depends on SCSI && BLOCK select CONFIGFS_FS select CRC_T10DIF - select BLK_SCSI_REQUEST # only for scsi_command_size_tbl.. select SGL_ALLOC default n help diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h index 731ac09ed231..48d950666376 100644 --- a/include/scsi/scsi_common.h +++ b/include/scsi/scsi_common.h @@ -15,8 +15,17 @@ scsi_varlen_cdb_length(const void *hdr) return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8; } -extern const unsigned char scsi_command_size_tbl[8]; -#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7] +/* + * SCSI command sizes are as follows, in bytes, for fixed size commands, per + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode + * determine its group. + * The size table is encoded into a 32-bit value by subtracting each value + * from 16, resulting in a value of 1715488362 + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + 10). + * Command group 3 is reserved and should never be used. + */ +#define COMMAND_SIZE(opcode) \ + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7))))) static inline unsigned scsi_command_size(const unsigned char *cmnd) -- 2.11.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time 2018-03-09 22:33 ` [PATCH] scsi: resolve COMMAND_SIZE at compile time Stephen Kitt @ 2018-03-09 22:47 ` Bart Van Assche 2018-03-10 13:29 ` Stephen Kitt 2018-03-13 11:34 ` David Laight 1 sibling, 1 reply; 14+ messages in thread From: Bart Van Assche @ 2018-03-09 22:47 UTC (permalink / raw) To: jejb, steve, hare, martin.petersen, axboe Cc: linux-scsi, linux-kernel, linux-block, kernel-hardening On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote: > +/* > + * SCSI command sizes are as follows, in bytes, for fixed size commands, per > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode > + * determine its group. > + * The size table is encoded into a 32-bit value by subtracting each value > + * from 16, resulting in a value of 1715488362 > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + 10). > + * Command group 3 is reserved and should never be used. > + */ > +#define COMMAND_SIZE(opcode) \ > + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7))))) To me this seems hard to read and hard to verify. Could this have been written as a combination of ternary expressions, e.g. using a gcc statement expression to ensure that opcode is evaluated once? Thanks, Bart. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time 2018-03-09 22:47 ` Bart Van Assche @ 2018-03-10 13:29 ` Stephen Kitt 2018-03-10 20:49 ` James Bottomley 0 siblings, 1 reply; 14+ messages in thread From: Stephen Kitt @ 2018-03-10 13:29 UTC (permalink / raw) To: Bart Van Assche Cc: jejb, hare, martin.petersen, axboe, linux-scsi, linux-kernel, linux-block, kernel-hardening [-- Attachment #1: Type: text/plain, Size: 1691 bytes --] Hi Bart, On Fri, 9 Mar 2018 22:47:12 +0000, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote: > > +/* > > + * SCSI command sizes are as follows, in bytes, for fixed size commands, > > per > > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode > > + * determine its group. > > + * The size table is encoded into a 32-bit value by subtracting each > > value > > + * from 16, resulting in a value of 1715488362 > > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + > > 10). > > + * Command group 3 is reserved and should never be used. > > + */ > > +#define COMMAND_SIZE(opcode) \ > > + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7))))) > > To me this seems hard to read and hard to verify. Could this have been > written as a combination of ternary expressions, e.g. using a gcc statement > expression to ensure that opcode is evaluated once? That’s what I’d tried initially, e.g. #define COMMAND_SIZE(opcode) ({ \ int index = ((opcode) >> 5) & 7; \ index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 : 10); \ }) But gcc still reckons that results in a VLA, defeating the initial purpose of the exercise. Does it help if I make the magic value construction clearer? #define SCSI_COMMAND_SIZE_TBL ( \ (16 - 6) \ + ((16 - 10) << 4) \ + ((16 - 10) << 8) \ + ((16 - 12) << 12) \ + ((16 - 16) << 16) \ + ((16 - 12) << 20) \ + ((16 - 10) << 24) \ + ((16 - 10) << 28)) #define COMMAND_SIZE(opcode) \ (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) & 7))))) Regards, Stephen [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time 2018-03-10 13:29 ` Stephen Kitt @ 2018-03-10 20:49 ` James Bottomley 2018-03-10 21:16 ` Douglas Gilbert 0 siblings, 1 reply; 14+ messages in thread From: James Bottomley @ 2018-03-10 20:49 UTC (permalink / raw) To: Stephen Kitt, Bart Van Assche Cc: hare, martin.petersen, axboe, linux-scsi, linux-kernel, linux-block, kernel-hardening [-- Attachment #1: Type: text/plain, Size: 2092 bytes --] On Sat, 2018-03-10 at 14:29 +0100, Stephen Kitt wrote: > Hi Bart, > > On Fri, 9 Mar 2018 22:47:12 +0000, Bart Van Assche <Bart.VanAssche@wd > c.com> > wrote: > > > > On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote: > > > > > > +/* > > > + * SCSI command sizes are as follows, in bytes, for fixed size > > > commands, > > > per > > > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of > > > an opcode > > > + * determine its group. > > > + * The size table is encoded into a 32-bit value by subtracting > > > each > > > value > > > + * from 16, resulting in a value of 1715488362 > > > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 > > > << 4 + > > > 10). > > > + * Command group 3 is reserved and should never be used. > > > + */ > > > +#define COMMAND_SIZE(opcode) \ > > > + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & > > > 7))))) > > > > To me this seems hard to read and hard to verify. Could this have > > been > > written as a combination of ternary expressions, e.g. using a gcc > > statement > > expression to ensure that opcode is evaluated once? > > That’s what I’d tried initially, e.g. > > #define COMMAND_SIZE(opcode) ({ \ > int index = ((opcode) >> 5) & 7; \ > index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 : > 10); \ > }) > > But gcc still reckons that results in a VLA, defeating the initial > purpose of > the exercise. > > Does it help if I make the magic value construction clearer? > > #define SCSI_COMMAND_SIZE_TBL ( \ > (16 - 6) \ > + ((16 - 10) << 4) \ > + ((16 - 10) << 8) \ > + ((16 - 12) << 12) \ > + ((16 - 16) << 16) \ > + ((16 - 12) << 20) \ > + ((16 - 10) << 24) \ > + ((16 - 10) << 28)) > > #define COMMAND_SIZE(opcode) > \ > (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) & > 7))))) Couldn't we do the less clever thing of making the array a static const and moving it to a header? That way the compiler should be able to work it out at compile time. James [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time 2018-03-10 20:49 ` James Bottomley @ 2018-03-10 21:16 ` Douglas Gilbert 0 siblings, 0 replies; 14+ messages in thread From: Douglas Gilbert @ 2018-03-10 21:16 UTC (permalink / raw) To: James Bottomley, Stephen Kitt, Bart Van Assche Cc: hare, martin.petersen, axboe, linux-scsi, linux-kernel, linux-block, kernel-hardening On 2018-03-10 03:49 PM, James Bottomley wrote: > On Sat, 2018-03-10 at 14:29 +0100, Stephen Kitt wrote: >> Hi Bart, >> >> On Fri, 9 Mar 2018 22:47:12 +0000, Bart Van Assche <Bart.VanAssche@wd >> c.com> >> wrote: >>> >>> On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote: >>>> >>>> +/* >>>> + * SCSI command sizes are as follows, in bytes, for fixed size >>>> commands, >>>> per >>>> + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of >>>> an opcode >>>> + * determine its group. >>>> + * The size table is encoded into a 32-bit value by subtracting >>>> each >>>> value >>>> + * from 16, resulting in a value of 1715488362 >>>> + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 >>>> << 4 + >>>> 10). >>>> + * Command group 3 is reserved and should never be used. >>>> + */ >>>> +#define COMMAND_SIZE(opcode) \ >>>> + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & >>>> 7))))) >>> >>> To me this seems hard to read and hard to verify. Could this have >>> been >>> written as a combination of ternary expressions, e.g. using a gcc >>> statement >>> expression to ensure that opcode is evaluated once? >> >> That’s what I’d tried initially, e.g. >> >> #define COMMAND_SIZE(opcode) ({ \ >> int index = ((opcode) >> 5) & 7; \ >> index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 : >> 10); \ >> }) >> >> But gcc still reckons that results in a VLA, defeating the initial >> purpose of >> the exercise. >> >> Does it help if I make the magic value construction clearer? >> >> #define SCSI_COMMAND_SIZE_TBL ( \ >> (16 - 6) \ >> + ((16 - 10) << 4) \ >> + ((16 - 10) << 8) \ >> + ((16 - 12) << 12) \ >> + ((16 - 16) << 16) \ >> + ((16 - 12) << 20) \ >> + ((16 - 10) << 24) \ >> + ((16 - 10) << 28)) >> >> #define COMMAND_SIZE(opcode) >> \ >> (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) & >> 7))))) > > Couldn't we do the less clever thing of making the array a static const > and moving it to a header? That way the compiler should be able to > work it out at compile time. And maybe add a comment that as of now (SPC-5 rev 19), COMMAND_SIZE is not valid for opcodes 0x7e and 0x7f plus everything above and including 0xc0. The latter ones are vendor specific and are loosely constrained, probably all even numbered lengths in the closed range: [6,260]. If the SCSI command sets want to keep up with NVMe, they may want to think about how they can gainfully use cdb_s that are > 64 bytes long. WRITE SCATTERED got into SBC-4 but READ GATHERED didn't, due to lack of interest. The READ GATHERED proposed was a bidi command, but it could have been a a simpler data-in command with a looong cdb (holding LBA, number_of_blocks pairs). Doug Gilbert ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH] scsi: resolve COMMAND_SIZE at compile time 2018-03-09 22:33 ` [PATCH] scsi: resolve COMMAND_SIZE at compile time Stephen Kitt 2018-03-09 22:47 ` Bart Van Assche @ 2018-03-13 11:34 ` David Laight 1 sibling, 0 replies; 14+ messages in thread From: David Laight @ 2018-03-13 11:34 UTC (permalink / raw) To: 'Stephen Kitt', hare, axboe, jejb, martin.petersen Cc: linux-scsi, kernel-hardening, linux-kernel, linux-block From: Stephen Kitt > Sent: 09 March 2018 22:34 > > COMMAND_SIZE currently uses an array of values in block/scsi_ioctl.c. > A number of device_handler functions use this to initialise arrays, > and this is flagged by -Wvla. > > This patch replaces COMMAND_SIZE with a variant using a formula which > can be resolved at compile time in cases where the opcode is fixed, > resolving the array size and avoiding the VLA. The downside is that > the code is less maintainable and that some call sites end up having > more complex generated code. > > Since scsi_command_size_tbl is dropped, we can remove the dependency > on BLK_SCSI_REQUEST from drivers/target/Kconfig. > > This was prompted by https://lkml.org/lkml/2018/3/7/621 > > Signed-off-by: Stephen Kitt <steve@sk2.org> > --- > block/scsi_ioctl.c | 8 -------- > drivers/target/Kconfig | 1 - > include/scsi/scsi_common.h | 13 +++++++++++-- > 3 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c > index 60b471f8621b..b9e176e537be 100644 > --- a/block/scsi_ioctl.c > +++ b/block/scsi_ioctl.c > @@ -41,14 +41,6 @@ struct blk_cmd_filter { > > static struct blk_cmd_filter blk_default_cmd_filter; > > -/* Command group 3 is reserved and should never be used. */ > -const unsigned char scsi_command_size_tbl[8] = > -{ > - 6, 10, 10, 12, > - 16, 12, 10, 10 > -}; > -EXPORT_SYMBOL(scsi_command_size_tbl); > - > #include <scsi/sg.h> > > static int sg_get_version(int __user *p) > diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig > index 4c44d7bed01a..b5f05b60cf06 100644 > --- a/drivers/target/Kconfig > +++ b/drivers/target/Kconfig > @@ -4,7 +4,6 @@ menuconfig TARGET_CORE > depends on SCSI && BLOCK > select CONFIGFS_FS > select CRC_T10DIF > - select BLK_SCSI_REQUEST # only for scsi_command_size_tbl.. > select SGL_ALLOC > default n > help > diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h > index 731ac09ed231..48d950666376 100644 > --- a/include/scsi/scsi_common.h > +++ b/include/scsi/scsi_common.h > @@ -15,8 +15,17 @@ scsi_varlen_cdb_length(const void *hdr) > return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8; > } > > -extern const unsigned char scsi_command_size_tbl[8]; > -#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7] > +/* > + * SCSI command sizes are as follows, in bytes, for fixed size commands, per > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode > + * determine its group. > + * The size table is encoded into a 32-bit value by subtracting each value > + * from 16, resulting in a value of 1715488362 > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + 10). > + * Command group 3 is reserved and should never be used. > + */ > +#define COMMAND_SIZE(opcode) \ > + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7))))) Why not: (6 + (15 & (0x446a6440u .... Specifying the constant in hex makes it more obvious. It is a shame about the 16, but an offset is easier than the negate. David ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-03-13 11:34 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-09 22:29 VLA removal, device_handler and COMMAND_SIZE Stephen Kitt 2018-03-09 22:32 ` [PATCH] device_handler: remove VLAs Stephen Kitt 2018-03-09 22:48 ` Bart Van Assche 2018-03-10 13:14 ` Stephen Kitt 2018-03-12 15:41 ` Bart Van Assche 2018-03-12 19:26 ` Stephen Kitt 2018-03-12 6:25 ` Hannes Reinecke 2018-03-13 2:37 ` Martin K. Petersen 2018-03-09 22:33 ` [PATCH] scsi: resolve COMMAND_SIZE at compile time Stephen Kitt 2018-03-09 22:47 ` Bart Van Assche 2018-03-10 13:29 ` Stephen Kitt 2018-03-10 20:49 ` James Bottomley 2018-03-10 21:16 ` Douglas Gilbert 2018-03-13 11:34 ` David Laight
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).