Re: [PATCH][next] scsi: aacraid: Replace one-element array with flexible-array member
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
Re: [PATCH][next] scsi: aacraid: Replace one-element array with flexible-array member
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
Re: [PATCH][next] scsi: aacraid: Replace one-element array with flexible-array member
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(>sge[1], [1], (nseg_new-1)*sizeof(struct >> sge_ieee1212)); >> +memcpy(>sge[1], [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. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH][next] scsi: aacraid: Replace one-element array with flexible-array member
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
> Link:
> https://lore.kernel.org/lkml/60414244.ur4%2fki+fbf1ohkzs%25...@intel.com/
> Signed-off-by: Gustavo A. R. Silva
> ---
> 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;
>
> @@
Re: [PATCH][next] scsi: aacraid: Replace one-element array with flexible-array member
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(>sge[1], [1], (nseg_new-1)*sizeof(struct > sge_ieee1212)); > + memcpy(>sge[1], [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
Re: [PATCH][next] scsi: aacraid: Replace one-element array with flexible-array member
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(>sge[1], [1], (nseg_new-1)*sizeof(struct sge_ieee1212)); + memcpy(>sge[1], [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! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH][next] scsi: aacraid: Replace one-element array with flexible-array member
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_ieee1212sge[1]; /*3216 */
/* 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_ieee1212sge[];/*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
Re: [PATCH][next] scsi: aacraid: Replace one-element array with flexible-array member
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. -- Martin K. Petersen Oracle Linux Engineering
[PATCH][next] scsi: aacraid: Replace one-element array with flexible-array member
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
Link: https://lore.kernel.org/lkml/60414244.ur4%2fki+fbf1ohkzs%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
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));
} 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(>sge[1], [1], (nseg_new-1)*sizeof(struct
sge_ieee1212));
+ memcpy(>sge[1], [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
