| Message ID | 20210304203822.GA102218@embeddedor (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [next] scsi: aacraid: Replace one-element array with flexible-array member | expand |
Hi Martin, On 3/24/21 20:18, Martin K. Petersen wrote: > > Hi Gustavo! > > Your changes and the original code do not appear to be functionally > equivalent. > >> @@ -1235,8 +1235,8 @@ static int aac_read_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3 >> if (ret < 0) >> return ret; >> command = ContainerRawIo2; >> - fibsize = sizeof(struct aac_raw_io2) + >> - ((le32_to_cpu(readcmd2->sgeCnt)-1) * sizeof(struct sge_ieee1212)); >> + fibsize = struct_size(readcmd2, sge, >> + le32_to_cpu(readcmd2->sgeCnt)); > > The old code allocated sgeCnt-1 elements (whether that was a mistake or > not I do not know) whereas the new code would send a larger fib to the > ASIC. I don't have any aacraid adapters and I am hesitant to merging > changes that have not been validated on real hardware. Precisely this sort of confusion is one of the things we want to avoid by using flexible-array members instead of one-element arrays. fibsize is actually the same for both the old and the new code. The difference is that in the original code, the one-element array _sge_ at the bottom of struct aac_raw_io2, contributes to the size of the structure, as it occupies at least as much space as a single object of its type. On the other hand, flexible-array members don't contribute to the size of the enclosing structure. See below... Old code: $ pahole -C aac_raw_io2 drivers/scsi/aacraid/aachba.o struct aac_raw_io2 { __le32 blockLow; /* 0 4 */ __le32 blockHigh; /* 4 4 */ __le32 byteCount; /* 8 4 */ __le16 cid; /* 12 2 */ __le16 flags; /* 14 2 */ __le32 sgeFirstSize; /* 16 4 */ __le32 sgeNominalSize; /* 20 4 */ u8 sgeCnt; /* 24 1 */ u8 bpTotal; /* 25 1 */ u8 bpComplete; /* 26 1 */ u8 sgeFirstIndex; /* 27 1 */ u8 unused[4]; /* 28 4 */ struct sge_ieee1212 sge[1]; /* 32 16 */ /* size: 48, cachelines: 1, members: 13 */ /* last cacheline: 48 bytes */ }; New code: $ pahole -C aac_raw_io2 drivers/scsi/aacraid/aachba.o struct aac_raw_io2 { __le32 blockLow; /* 0 4 */ __le32 blockHigh; /* 4 4 */ __le32 byteCount; /* 8 4 */ __le16 cid; /* 12 2 */ __le16 flags; /* 14 2 */ __le32 sgeFirstSize; /* 16 4 */ __le32 sgeNominalSize; /* 20 4 */ u8 sgeCnt; /* 24 1 */ u8 bpTotal; /* 25 1 */ u8 bpComplete; /* 26 1 */ u8 sgeFirstIndex; /* 27 1 */ u8 unused[4]; /* 28 4 */ struct sge_ieee1212 sge[]; /* 32 0 */ /* size: 32, cachelines: 1, members: 13 */ /* last cacheline: 32 bytes */ }; So, the old code allocates sgeCnt-1 elements because sizeof(struct aac_raw_io2) is already counting one element of the _sge_ array. Please, let me know if this is clear now. Thanks! -- Gustavo
Hi Gustavo! Your changes and the original code do not appear to be functionally equivalent. > @@ -1235,8 +1235,8 @@ static int aac_read_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3 > if (ret < 0) > return ret; > command = ContainerRawIo2; > - fibsize = sizeof(struct aac_raw_io2) + > - ((le32_to_cpu(readcmd2->sgeCnt)-1) * sizeof(struct sge_ieee1212)); > + fibsize = struct_size(readcmd2, sge, > + le32_to_cpu(readcmd2->sgeCnt)); The old code allocated sgeCnt-1 elements (whether that was a mistake or not I do not know) whereas the new code would send a larger fib to the ASIC. I don't have any aacraid adapters and I am hesitant to merging changes that have not been validated on real hardware.
Hi Martin, On 3/25/21 22:34, Martin K. Petersen wrote: > > Gustavo, > >> Precisely this sort of confusion is one of the things we want to avoid >> by using flexible-array members instead of one-element arrays. > > Ah, you're right! > > Now that I look at it again I also don't think that was the issue that > originally caused concern. > > @@ -4020,7 +4020,8 @@ static int aac_convert_sgraw2(struct aac_raw_io2 *rio2, int pages, int nseg, int > } > } > sge[pos] = rio2->sge[nseg-1]; > - memcpy(&rio2->sge[1], &sge[1], (nseg_new-1)*sizeof(struct sge_ieee1212)); > + memcpy(&rio2->sge[1], &sge[1], > + flex_array_size(rio2, sge, nseg_new - 1)); > > kfree(sge); > rio2->sgeCnt = cpu_to_le32(nseg_new); > > I find it counter-intuitive to use the type of the destination array to > size the amount of source data to copy. "Are source and destination same The destination and source arrays are of the same type. :) drivers/scsi/aacraid/aachba.c: 3999 struct sge_ieee1212 *sge; > type? Does flex_array_size() do the right thing given the ->sge[1] > destination offset?". It wasn't immediately obvious. To me, "copy this > many scatterlist entries" in the original is much more readable. Yeah; it does the right thing because flex_array_size() doesn't know about offsets. It just calculates the amount of bytes to be copied based on the type of the object passed as second argument and a "count" passed as third argument. So, in this case, the "count" is "nseg_new - 1", which in some way is already taking care of that sge[1] offset. -- Gustavo
Gustavo, > Precisely this sort of confusion is one of the things we want to avoid > by using flexible-array members instead of one-element arrays. Ah, you're right! Now that I look at it again I also don't think that was the issue that originally caused concern. @@ -4020,7 +4020,8 @@ static int aac_convert_sgraw2(struct aac_raw_io2 *rio2, int pages, int nseg, int } } sge[pos] = rio2->sge[nseg-1]; - memcpy(&rio2->sge[1], &sge[1], (nseg_new-1)*sizeof(struct sge_ieee1212)); + memcpy(&rio2->sge[1], &sge[1], + flex_array_size(rio2, sge, nseg_new - 1)); kfree(sge); rio2->sgeCnt = cpu_to_le32(nseg_new); I find it counter-intuitive to use the type of the destination array to size the amount of source data to copy. "Are source and destination same type? Does flex_array_size() do the right thing given the ->sge[1] destination offset?". It wasn't immediately obvious. To me, "copy this many scatterlist entries" in the original is much more readable. That said, this whole function makes my head hurt!
On Thu, Mar 04, 2021 at 02:38:22PM -0600, Gustavo A. R. Silva wrote: > There is a regular need in the kernel to provide a way to declare having > a dynamically sized set of trailing elements in a structure. Kernel code > should always use “flexible array members”[1] for these cases. The older > style of one-element or zero-length arrays should no longer be used[2]. > > Refactor the code according to the use of a flexible-array member in > struct aac_raw_io2 instead of one-element array, and use the > struct_size() and flex_array_size() helpers. > > Also, this helps with the ongoing efforts to enable -Warray-bounds by > fixing the following warnings: > > drivers/scsi/aacraid/aachba.c: In function ‘aac_build_sgraw2’: > drivers/scsi/aacraid/aachba.c:3970:18: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds] > 3970 | if (rio2->sge[j].length % (i*PAGE_SIZE)) { > | ~~~~~~~~~^~~ > drivers/scsi/aacraid/aachba.c:3974:27: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds] > 3974 | nseg_new += (rio2->sge[j].length / (i*PAGE_SIZE)); > | ~~~~~~~~~^~~ > drivers/scsi/aacraid/aachba.c:4011:28: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds] > 4011 | for (j = 0; j < rio2->sge[i].length / (pages * PAGE_SIZE); ++j) { > | ~~~~~~~~~^~~ > drivers/scsi/aacraid/aachba.c:4012:24: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds] > 4012 | addr_low = rio2->sge[i].addrLow + j * pages * PAGE_SIZE; > | ~~~~~~~~~^~~ > drivers/scsi/aacraid/aachba.c:4014:33: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds] > 4014 | sge[pos].addrHigh = rio2->sge[i].addrHigh; > | ~~~~~~~~~^~~ > drivers/scsi/aacraid/aachba.c:4015:28: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds] > 4015 | if (addr_low < rio2->sge[i].addrLow) > | ~~~~~~~~~^~~ > > [1] https://en.wikipedia.org/wiki/Flexible_array_member > [2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/109 > Build-tested-by: kernel test robot <lkp@intel.com> > Link: https://lore.kernel.org/lkml/60414244.ur4%2FkI+fBF1ohKZs%25lkp@intel.com/ > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > drivers/scsi/aacraid/aachba.c | 13 +++++++------ > drivers/scsi/aacraid/aacraid.h | 2 +- > 2 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c > index 4ca5e13a26a6..0f5617e40b94 100644 > --- a/drivers/scsi/aacraid/aachba.c > +++ b/drivers/scsi/aacraid/aachba.c > @@ -1235,8 +1235,8 @@ static int aac_read_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3 > if (ret < 0) > return ret; > command = ContainerRawIo2; > - fibsize = sizeof(struct aac_raw_io2) + > - ((le32_to_cpu(readcmd2->sgeCnt)-1) * sizeof(struct sge_ieee1212)); > + fibsize = struct_size(readcmd2, sge, > + le32_to_cpu(readcmd2->sgeCnt)); readcmd2 is struct aac_raw_io2, and sge is the struct sge_ieee1212 array, so this looks correct to me with the change to struct aac_raw_io2.. > } else { > struct aac_raw_io *readcmd; > readcmd = (struct aac_raw_io *) fib_data(fib); > @@ -1366,8 +1366,8 @@ static int aac_write_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u > if (ret < 0) > return ret; > command = ContainerRawIo2; > - fibsize = sizeof(struct aac_raw_io2) + > - ((le32_to_cpu(writecmd2->sgeCnt)-1) * sizeof(struct sge_ieee1212)); > + fibsize = struct_size(writecmd2, sge, > + le32_to_cpu(writecmd2->sgeCnt)); writecmd2 is struct aac_raw_io2, and sge is the struct sge_ieee1212 array, so this looks correct to me with the change to struct aac_raw_io2. > } else { > struct aac_raw_io *writecmd; > writecmd = (struct aac_raw_io *) fib_data(fib); > @@ -4003,7 +4003,7 @@ static int aac_convert_sgraw2(struct aac_raw_io2 *rio2, int pages, int nseg, int > if (aac_convert_sgl == 0) > return 0; > > - sge = kmalloc_array(nseg_new, sizeof(struct sge_ieee1212), GFP_ATOMIC); > + sge = kmalloc_array(nseg_new, sizeof(*sge), GFP_ATOMIC); Technically, this is unrelated (struct sge_ieee1212 has not changed), but sge is a struct sge_ieee1212 pointer, so this is good robustness change, IMO. > if (sge == NULL) > return -ENOMEM; > > @@ -4020,7 +4020,8 @@ static int aac_convert_sgraw2(struct aac_raw_io2 *rio2, int pages, int nseg, int > } > } > sge[pos] = rio2->sge[nseg-1]; > - memcpy(&rio2->sge[1], &sge[1], (nseg_new-1)*sizeof(struct sge_ieee1212)); > + memcpy(&rio2->sge[1], &sge[1], > + flex_array_size(rio2, sge, nseg_new - 1)); This was hard to validate, but looks correct to me. The flex array helper here is the same as the prior code (but now tied to the variables, which is more robust IMO). The use of seg[1] here appears to be just how this code works -- the loop above is rewriting the 1 through nseg_new - 1 array entries, and then this copies back the results. > > kfree(sge); > rio2->sgeCnt = cpu_to_le32(nseg_new); > diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h > index e3e4ecbea726..3733df77bc65 100644 > --- a/drivers/scsi/aacraid/aacraid.h > +++ b/drivers/scsi/aacraid/aacraid.h > @@ -1929,7 +1929,7 @@ struct aac_raw_io2 { > u8 bpComplete; /* reserved for F/W use */ > u8 sgeFirstIndex; /* reserved for F/W use */ > u8 unused[4]; > - struct sge_ieee1212 sge[1]; > + struct sge_ieee1212 sge[]; > }; > > #define CT_FLUSH_CACHE 129 > -- > 2.27.0 > Thanks! Reviewed-by: Kees Cook <keescook@chromium.org>
Hi Kees/Gustavo! >> @@ -4020,7 +4020,8 @@ static int aac_convert_sgraw2(struct aac_raw_io2 *rio2, int pages, int nseg, int >> } >> } >> sge[pos] = rio2->sge[nseg-1]; >> - memcpy(&rio2->sge[1], &sge[1], (nseg_new-1)*sizeof(struct sge_ieee1212)); >> + memcpy(&rio2->sge[1], &sge[1], >> + flex_array_size(rio2, sge, nseg_new - 1)); > > This was hard to validate, ... which is why I didn't apply this patch. I don't like changes which make the reader have to jump through hoops to figure out what the code actually does. I find the original much easier to understand. Silencing analyzer warnings shouldn't be done at the expense of human readers. If it is imperative to switch to flex_array_size() to quiesce checker warnings, please add a comment in the code explaining that the size evaluates to nseg_new-1 sge_ieee1212 structs.
Hi Martin, On 4/12/21 23:52, Martin K. Petersen wrote: > Silencing analyzer warnings shouldn't be done at the expense of human > readers. If it is imperative to switch to flex_array_size() to quiesce > checker warnings, please add a comment in the code explaining that the > size evaluates to nseg_new-1 sge_ieee1212 structs. Done: https://lore.kernel.org/lkml/20210413054032.GA276102@embeddedor/ Thanks! -- Gustavo
On Tue, 2021-04-13 at 00:45 -0500, Gustavo A. R. Silva wrote: > Hi Martin, > > On 4/12/21 23:52, Martin K. Petersen wrote: > > > Silencing analyzer warnings shouldn't be done at the expense of > > human > > readers. If it is imperative to switch to flex_array_size() to > > quiesce > > checker warnings, please add a comment in the code explaining that > > the > > size evaluates to nseg_new-1 sge_ieee1212 structs. > > Done: > > https://lore.kernel.org/lkml/20210413054032.GA276102@embeddedor/ I think the reason everyone gets confused is that they think the first argument should do something. If flex_array_size had been defined #define flex_array_size(p, count) \ array_size(count, \ sizeof(*(p)) + __must_be_array(p)) Then we could have used flex_array_size(sge, nseg_new - 1) or flex_array_size(rio->sge, nseg_new - 1) and everyone would have understood either expression. This would also have been useful, as the first example demonstrates, when we have a pointer rather than a flexible member ... although that means the macro likely needs a new name. However, perhaps just do array_size(nseg_new - 1, sizeof(*sge)); And lose the comment? James
diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c index 4ca5e13a26a6..0f5617e40b94 100644 --- a/drivers/scsi/aacraid/aachba.c +++ b/drivers/scsi/aacraid/aachba.c @@ -1235,8 +1235,8 @@ static int aac_read_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3 if (ret < 0) return ret; command = ContainerRawIo2; - fibsize = sizeof(struct aac_raw_io2) + - ((le32_to_cpu(readcmd2->sgeCnt)-1) * sizeof(struct sge_ieee1212)); + fibsize = struct_size(readcmd2, sge, + le32_to_cpu(readcmd2->sgeCnt)); } else { struct aac_raw_io *readcmd; readcmd = (struct aac_raw_io *) fib_data(fib); @@ -1366,8 +1366,8 @@ static int aac_write_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u if (ret < 0) return ret; command = ContainerRawIo2; - fibsize = sizeof(struct aac_raw_io2) + - ((le32_to_cpu(writecmd2->sgeCnt)-1) * sizeof(struct sge_ieee1212)); + fibsize = struct_size(writecmd2, sge, + le32_to_cpu(writecmd2->sgeCnt)); } else { struct aac_raw_io *writecmd; writecmd = (struct aac_raw_io *) fib_data(fib); @@ -4003,7 +4003,7 @@ static int aac_convert_sgraw2(struct aac_raw_io2 *rio2, int pages, int nseg, int if (aac_convert_sgl == 0) return 0; - sge = kmalloc_array(nseg_new, sizeof(struct sge_ieee1212), GFP_ATOMIC); + sge = kmalloc_array(nseg_new, sizeof(*sge), GFP_ATOMIC); if (sge == NULL) return -ENOMEM; @@ -4020,7 +4020,8 @@ static int aac_convert_sgraw2(struct aac_raw_io2 *rio2, int pages, int nseg, int } } sge[pos] = rio2->sge[nseg-1]; - memcpy(&rio2->sge[1], &sge[1], (nseg_new-1)*sizeof(struct sge_ieee1212)); + memcpy(&rio2->sge[1], &sge[1], + flex_array_size(rio2, sge, nseg_new - 1)); kfree(sge); rio2->sgeCnt = cpu_to_le32(nseg_new); diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h index e3e4ecbea726..3733df77bc65 100644 --- a/drivers/scsi/aacraid/aacraid.h +++ b/drivers/scsi/aacraid/aacraid.h @@ -1929,7 +1929,7 @@ struct aac_raw_io2 { u8 bpComplete; /* reserved for F/W use */ u8 sgeFirstIndex; /* reserved for F/W use */ u8 unused[4]; - struct sge_ieee1212 sge[1]; + struct sge_ieee1212 sge[]; }; #define CT_FLUSH_CACHE 129
There is a regular need in the kernel to provide a way to declare having a dynamically sized set of trailing elements in a structure. Kernel code should always use “flexible array members”[1] for these cases. The older style of one-element or zero-length arrays should no longer be used[2]. Refactor the code according to the use of a flexible-array member in struct aac_raw_io2 instead of one-element array, and use the struct_size() and flex_array_size() helpers. Also, this helps with the ongoing efforts to enable -Warray-bounds by fixing the following warnings: drivers/scsi/aacraid/aachba.c: In function ‘aac_build_sgraw2’: drivers/scsi/aacraid/aachba.c:3970:18: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds] 3970 | if (rio2->sge[j].length % (i*PAGE_SIZE)) { | ~~~~~~~~~^~~ drivers/scsi/aacraid/aachba.c:3974:27: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds] 3974 | nseg_new += (rio2->sge[j].length / (i*PAGE_SIZE)); | ~~~~~~~~~^~~ drivers/scsi/aacraid/aachba.c:4011:28: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds] 4011 | for (j = 0; j < rio2->sge[i].length / (pages * PAGE_SIZE); ++j) { | ~~~~~~~~~^~~ drivers/scsi/aacraid/aachba.c:4012:24: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds] 4012 | addr_low = rio2->sge[i].addrLow + j * pages * PAGE_SIZE; | ~~~~~~~~~^~~ drivers/scsi/aacraid/aachba.c:4014:33: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds] 4014 | sge[pos].addrHigh = rio2->sge[i].addrHigh; | ~~~~~~~~~^~~ drivers/scsi/aacraid/aachba.c:4015:28: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds] 4015 | if (addr_low < rio2->sge[i].addrLow) | ~~~~~~~~~^~~ [1] https://en.wikipedia.org/wiki/Flexible_array_member [2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays Link: https://github.com/KSPP/linux/issues/79 Link: https://github.com/KSPP/linux/issues/109 Build-tested-by: kernel test robot <lkp@intel.com> Link: https://lore.kernel.org/lkml/60414244.ur4%2FkI+fBF1ohKZs%25lkp@intel.com/ Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- drivers/scsi/aacraid/aachba.c | 13 +++++++------ drivers/scsi/aacraid/aacraid.h | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-)