| Message ID | 20210211001044.GA69612@embeddedor |
|---|---|
| State | New |
| Headers | show |
| Series | [next] media: venus: hfi_msgs.h: Replace one-element arrays with flexible-array members | expand |
Hi all, Friendly ping: who can take this, please? Thanks -- Gustavo On Wed, Feb 10, 2021 at 06:10:44PM -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]. > > Use flexible-array members in struct hfi_msg_sys_property_info_pkt and > hfi_msg_session_property_info_pkt instead of one-element arrays. > > Also, this helps with the ongoing efforts to enable -Warray-bounds by > fixing the following warnings: > > CC [M] drivers/media/platform/qcom/venus/hfi_msgs.o > drivers/media/platform/qcom/venus/hfi_msgs.c: In function ‘hfi_sys_property_info’: > drivers/media/platform/qcom/venus/hfi_msgs.c:246:35: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds] > 246 | if (req_bytes < 128 || !pkt->data[1] || pkt->num_properties > 1) > | ~~~~~~~~~^~~ > drivers/media/platform/qcom/venus/hfi_msgs.c: In function ‘hfi_session_prop_info’: > drivers/media/platform/qcom/venus/hfi_msgs.c:342:62: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds] > 342 | if (!req_bytes || req_bytes % sizeof(*buf_req) || !pkt->data[1]) > | ~~~~~~~~~^~~ > > [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: <lkp@intel.com> > Link: https://lore.kernel.org/lkml/6023dd80.MmTeFf8SzwX0iK7%2F%25lkp@intel.com/ > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > drivers/media/platform/qcom/venus/hfi_msgs.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.h b/drivers/media/platform/qcom/venus/hfi_msgs.h > index 526d9f5b487b..e2d2ccfbdd24 100644 > --- a/drivers/media/platform/qcom/venus/hfi_msgs.h > +++ b/drivers/media/platform/qcom/venus/hfi_msgs.h > @@ -113,7 +113,7 @@ struct hfi_msg_sys_ping_ack_pkt { > struct hfi_msg_sys_property_info_pkt { > struct hfi_pkt_hdr hdr; > u32 num_properties; > - u32 data[1]; > + u32 data[]; > }; > > struct hfi_msg_session_load_resources_done_pkt { > @@ -233,7 +233,7 @@ struct hfi_msg_session_parse_sequence_header_done_pkt { > struct hfi_msg_session_property_info_pkt { > struct hfi_session_hdr_pkt shdr; > u32 num_properties; > - u32 data[1]; > + u32 data[]; > }; > > struct hfi_msg_session_release_resources_done_pkt { > -- > 2.27.0 >
Hi all, Friendly ping: We are about to be able to globally enable -Warray-bounds and, these are one of the last out-of-bounds warnings in linux-next. Could someone take this, please? Thanks -- Gustavo On 2/10/21 18:10, 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]. > > Use flexible-array members in struct hfi_msg_sys_property_info_pkt and > hfi_msg_session_property_info_pkt instead of one-element arrays. > > Also, this helps with the ongoing efforts to enable -Warray-bounds by > fixing the following warnings: > > CC [M] drivers/media/platform/qcom/venus/hfi_msgs.o > drivers/media/platform/qcom/venus/hfi_msgs.c: In function ‘hfi_sys_property_info’: > drivers/media/platform/qcom/venus/hfi_msgs.c:246:35: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds] > 246 | if (req_bytes < 128 || !pkt->data[1] || pkt->num_properties > 1) > | ~~~~~~~~~^~~ > drivers/media/platform/qcom/venus/hfi_msgs.c: In function ‘hfi_session_prop_info’: > drivers/media/platform/qcom/venus/hfi_msgs.c:342:62: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds] > 342 | if (!req_bytes || req_bytes % sizeof(*buf_req) || !pkt->data[1]) > | ~~~~~~~~~^~~ > > [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: <lkp@intel.com> > Link: https://lore.kernel.org/lkml/6023dd80.MmTeFf8SzwX0iK7%2F%25lkp@intel.com/ > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > drivers/media/platform/qcom/venus/hfi_msgs.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.h b/drivers/media/platform/qcom/venus/hfi_msgs.h > index 526d9f5b487b..e2d2ccfbdd24 100644 > --- a/drivers/media/platform/qcom/venus/hfi_msgs.h > +++ b/drivers/media/platform/qcom/venus/hfi_msgs.h > @@ -113,7 +113,7 @@ struct hfi_msg_sys_ping_ack_pkt { > struct hfi_msg_sys_property_info_pkt { > struct hfi_pkt_hdr hdr; > u32 num_properties; > - u32 data[1]; > + u32 data[]; > }; > > struct hfi_msg_session_load_resources_done_pkt { > @@ -233,7 +233,7 @@ struct hfi_msg_session_parse_sequence_header_done_pkt { > struct hfi_msg_session_property_info_pkt { > struct hfi_session_hdr_pkt shdr; > u32 num_properties; > - u32 data[1]; > + u32 data[]; > }; > > struct hfi_msg_session_release_resources_done_pkt { >
Hi, On 5/11/21 6:46 PM, Gustavo A. R. Silva wrote: > Hi all, > > Friendly ping: > > We are about to be able to globally enable -Warray-bounds and, these are one of > the last out-of-bounds warnings in linux-next. > > Could someone take this, please? This one introduces regressions, so I cannot take it. It needs some more work. > > Thanks > -- > Gustavo > > On 2/10/21 18:10, 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]. >> >> Use flexible-array members in struct hfi_msg_sys_property_info_pkt and >> hfi_msg_session_property_info_pkt instead of one-element arrays. >> >> Also, this helps with the ongoing efforts to enable -Warray-bounds by >> fixing the following warnings: >> >> CC [M] drivers/media/platform/qcom/venus/hfi_msgs.o >> drivers/media/platform/qcom/venus/hfi_msgs.c: In function ‘hfi_sys_property_info’: >> drivers/media/platform/qcom/venus/hfi_msgs.c:246:35: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds] >> 246 | if (req_bytes < 128 || !pkt->data[1] || pkt->num_properties > 1) >> | ~~~~~~~~~^~~ >> drivers/media/platform/qcom/venus/hfi_msgs.c: In function ‘hfi_session_prop_info’: >> drivers/media/platform/qcom/venus/hfi_msgs.c:342:62: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds] >> 342 | if (!req_bytes || req_bytes % sizeof(*buf_req) || !pkt->data[1]) >> | ~~~~~~~~~^~~ >> >> [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: <lkp@intel.com> >> Link: https://lore.kernel.org/lkml/6023dd80.MmTeFf8SzwX0iK7%2F%25lkp@intel.com/ >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >> --- >> drivers/media/platform/qcom/venus/hfi_msgs.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.h b/drivers/media/platform/qcom/venus/hfi_msgs.h >> index 526d9f5b487b..e2d2ccfbdd24 100644 >> --- a/drivers/media/platform/qcom/venus/hfi_msgs.h >> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.h >> @@ -113,7 +113,7 @@ struct hfi_msg_sys_ping_ack_pkt { >> struct hfi_msg_sys_property_info_pkt { >> struct hfi_pkt_hdr hdr; >> u32 num_properties; >> - u32 data[1]; >> + u32 data[]; >> }; >> >> struct hfi_msg_session_load_resources_done_pkt { >> @@ -233,7 +233,7 @@ struct hfi_msg_session_parse_sequence_header_done_pkt { >> struct hfi_msg_session_property_info_pkt { >> struct hfi_session_hdr_pkt shdr; >> u32 num_properties; >> - u32 data[1]; >> + u32 data[]; >> }; >> >> struct hfi_msg_session_release_resources_done_pkt { >> -- regards, Stan
On 5/12/21 06:39, Stanimir Varbanov wrote: > Hi, > > On 5/11/21 6:46 PM, Gustavo A. R. Silva wrote: >> Hi all, >> >> Friendly ping: >> >> We are about to be able to globally enable -Warray-bounds and, these are one of >> the last out-of-bounds warnings in linux-next. >> >> Could someone take this, please? > > This one introduces regressions, so I cannot take it. It needs some more > work. Please, share with me the errors or warnings you see with this. So, I can have an idea of what is going on. Unfortunately, I don't have access to the test suite or hardware to test this. Thanks! -- Gustavo >> >> Thanks >> -- >> Gustavo >> >> On 2/10/21 18:10, 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]. >>> >>> Use flexible-array members in struct hfi_msg_sys_property_info_pkt and >>> hfi_msg_session_property_info_pkt instead of one-element arrays. >>> >>> Also, this helps with the ongoing efforts to enable -Warray-bounds by >>> fixing the following warnings: >>> >>> CC [M] drivers/media/platform/qcom/venus/hfi_msgs.o >>> drivers/media/platform/qcom/venus/hfi_msgs.c: In function ‘hfi_sys_property_info’: >>> drivers/media/platform/qcom/venus/hfi_msgs.c:246:35: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds] >>> 246 | if (req_bytes < 128 || !pkt->data[1] || pkt->num_properties > 1) >>> | ~~~~~~~~~^~~ >>> drivers/media/platform/qcom/venus/hfi_msgs.c: In function ‘hfi_session_prop_info’: >>> drivers/media/platform/qcom/venus/hfi_msgs.c:342:62: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds] >>> 342 | if (!req_bytes || req_bytes % sizeof(*buf_req) || !pkt->data[1]) >>> | ~~~~~~~~~^~~ >>> >>> [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: <lkp@intel.com> >>> Link: https://lore.kernel.org/lkml/6023dd80.MmTeFf8SzwX0iK7%2F%25lkp@intel.com/ >>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >>> --- >>> drivers/media/platform/qcom/venus/hfi_msgs.h | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.h b/drivers/media/platform/qcom/venus/hfi_msgs.h >>> index 526d9f5b487b..e2d2ccfbdd24 100644 >>> --- a/drivers/media/platform/qcom/venus/hfi_msgs.h >>> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.h >>> @@ -113,7 +113,7 @@ struct hfi_msg_sys_ping_ack_pkt { >>> struct hfi_msg_sys_property_info_pkt { >>> struct hfi_pkt_hdr hdr; >>> u32 num_properties; >>> - u32 data[1]; >>> + u32 data[]; >>> }; >>> >>> struct hfi_msg_session_load_resources_done_pkt { >>> @@ -233,7 +233,7 @@ struct hfi_msg_session_parse_sequence_header_done_pkt { >>> struct hfi_msg_session_property_info_pkt { >>> struct hfi_session_hdr_pkt shdr; >>> u32 num_properties; >>> - u32 data[1]; >>> + u32 data[]; >>> }; >>> >>> struct hfi_msg_session_release_resources_done_pkt { >>> >
Hi, On 5/12/21 7:37 PM, Gustavo A. R. Silva wrote: > > > On 5/12/21 06:39, Stanimir Varbanov wrote: >> Hi, >> >> On 5/11/21 6:46 PM, Gustavo A. R. Silva wrote: >>> Hi all, >>> >>> Friendly ping: >>> >>> We are about to be able to globally enable -Warray-bounds and, these are one of >>> the last out-of-bounds warnings in linux-next. >>> >>> Could someone take this, please? >> >> This one introduces regressions, so I cannot take it. It needs some more >> work. > > Please, share with me the errors or warnings you see with this. So, I can > have an idea of what is going on. Unfortunately, I don't have access to the > test suite or hardware to test this. I guess it needs more debugging, but the simple answer is that the driver refuse to start streaming with this patch. > > Thanks! > -- > Gustavo > > > >>> >>> Thanks >>> -- >>> Gustavo >>> >>> On 2/10/21 18:10, 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]. >>>> >>>> Use flexible-array members in struct hfi_msg_sys_property_info_pkt and >>>> hfi_msg_session_property_info_pkt instead of one-element arrays. >>>> >>>> Also, this helps with the ongoing efforts to enable -Warray-bounds by >>>> fixing the following warnings: >>>> >>>> CC [M] drivers/media/platform/qcom/venus/hfi_msgs.o >>>> drivers/media/platform/qcom/venus/hfi_msgs.c: In function ‘hfi_sys_property_info’: >>>> drivers/media/platform/qcom/venus/hfi_msgs.c:246:35: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds] >>>> 246 | if (req_bytes < 128 || !pkt->data[1] || pkt->num_properties > 1) >>>> | ~~~~~~~~~^~~ >>>> drivers/media/platform/qcom/venus/hfi_msgs.c: In function ‘hfi_session_prop_info’: >>>> drivers/media/platform/qcom/venus/hfi_msgs.c:342:62: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds] >>>> 342 | if (!req_bytes || req_bytes % sizeof(*buf_req) || !pkt->data[1]) >>>> | ~~~~~~~~~^~~ >>>> >>>> [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: <lkp@intel.com> >>>> Link: https://lore.kernel.org/lkml/6023dd80.MmTeFf8SzwX0iK7%2F%25lkp@intel.com/ >>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >>>> --- >>>> drivers/media/platform/qcom/venus/hfi_msgs.h | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.h b/drivers/media/platform/qcom/venus/hfi_msgs.h >>>> index 526d9f5b487b..e2d2ccfbdd24 100644 >>>> --- a/drivers/media/platform/qcom/venus/hfi_msgs.h >>>> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.h >>>> @@ -113,7 +113,7 @@ struct hfi_msg_sys_ping_ack_pkt { >>>> struct hfi_msg_sys_property_info_pkt { >>>> struct hfi_pkt_hdr hdr; >>>> u32 num_properties; >>>> - u32 data[1]; >>>> + u32 data[]; >>>> }; >>>> >>>> struct hfi_msg_session_load_resources_done_pkt { >>>> @@ -233,7 +233,7 @@ struct hfi_msg_session_parse_sequence_header_done_pkt { >>>> struct hfi_msg_session_property_info_pkt { >>>> struct hfi_session_hdr_pkt shdr; >>>> u32 num_properties; >>>> - u32 data[1]; >>>> + u32 data[]; >>>> }; >>>> >>>> struct hfi_msg_session_release_resources_done_pkt { >>>> >>
Hi Stanimir, On 5/17/21 06:01, Stanimir Varbanov wrote: >>> This one introduces regressions, so I cannot take it. It needs some more >>> work. >> >> Please, share with me the errors or warnings you see with this. So, I can >> have an idea of what is going on. Unfortunately, I don't have access to the >> test suite or hardware to test this. > > I guess it needs more debugging, but the simple answer is that the > driver refuse to start streaming with this patch. I see. Please, share with me the details about your test setup. Are you using qemu or actual hardware for this? Please, also help me by sending me your .config file and dmesg output, as well. So, I can try to debug this. Thanks! -- Gustavo
diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.h b/drivers/media/platform/qcom/venus/hfi_msgs.h index 526d9f5b487b..e2d2ccfbdd24 100644 --- a/drivers/media/platform/qcom/venus/hfi_msgs.h +++ b/drivers/media/platform/qcom/venus/hfi_msgs.h @@ -113,7 +113,7 @@ struct hfi_msg_sys_ping_ack_pkt { struct hfi_msg_sys_property_info_pkt { struct hfi_pkt_hdr hdr; u32 num_properties; - u32 data[1]; + u32 data[]; }; struct hfi_msg_session_load_resources_done_pkt { @@ -233,7 +233,7 @@ struct hfi_msg_session_parse_sequence_header_done_pkt { struct hfi_msg_session_property_info_pkt { struct hfi_session_hdr_pkt shdr; u32 num_properties; - u32 data[1]; + u32 data[]; }; struct hfi_msg_session_release_resources_done_pkt {
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]. Use flexible-array members in struct hfi_msg_sys_property_info_pkt and hfi_msg_session_property_info_pkt instead of one-element arrays. Also, this helps with the ongoing efforts to enable -Warray-bounds by fixing the following warnings: CC [M] drivers/media/platform/qcom/venus/hfi_msgs.o drivers/media/platform/qcom/venus/hfi_msgs.c: In function ‘hfi_sys_property_info’: drivers/media/platform/qcom/venus/hfi_msgs.c:246:35: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds] 246 | if (req_bytes < 128 || !pkt->data[1] || pkt->num_properties > 1) | ~~~~~~~~~^~~ drivers/media/platform/qcom/venus/hfi_msgs.c: In function ‘hfi_session_prop_info’: drivers/media/platform/qcom/venus/hfi_msgs.c:342:62: warning: array subscript 1 is above array bounds of ‘u32[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds] 342 | if (!req_bytes || req_bytes % sizeof(*buf_req) || !pkt->data[1]) | ~~~~~~~~~^~~ [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: <lkp@intel.com> Link: https://lore.kernel.org/lkml/6023dd80.MmTeFf8SzwX0iK7%2F%25lkp@intel.com/ Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- drivers/media/platform/qcom/venus/hfi_msgs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)