On 5/11/2021 2:59 AM, Wang, Haiyue wrote: >> -----Original Message----- >> From: Yigit, Ferruh <[email protected]> >> Sent: Tuesday, May 11, 2021 01:28 >> To: Wang, Haiyue <[email protected]>; Yang, Qiming >> <[email protected]>; Zhang, Qi Z >> <[email protected]>; Stillwell Jr, Paul M >> <[email protected]>; Lu, Wenzhuo >> <[email protected]>; Rong, Leyi <[email protected]>; Shukla, Shivanshu >> <[email protected]> >> Cc: [email protected]; [email protected]; Kevin Traynor <[email protected]>; Ajit >> Khaparde >> <[email protected]> >> Subject: Re: [dpdk-dev] [PATCH 3/4] net/ice/base: fix build with gcc11 >> >> On 5/10/2021 6:04 PM, Wang, Haiyue wrote: >>>> -----Original Message----- >>>> From: dev <[email protected]> On Behalf Of Ferruh Yigit >>>> Sent: Monday, May 10, 2021 23:03 >>>> To: Yang, Qiming <[email protected]>; Zhang, Qi Z >>>> <[email protected]>; Stillwell Jr, Paul M >>>> <[email protected]>; Lu, Wenzhuo <[email protected]>; Rong, >>>> Leyi >> <[email protected]>; >>>> Shukla, Shivanshu <[email protected]> >>>> Cc: Yigit, Ferruh <[email protected]>; [email protected]; [email protected]; >>>> Kevin Traynor >>>> <[email protected]>; Ajit Khaparde <[email protected]> >>>> Subject: [dpdk-dev] [PATCH 3/4] net/ice/base: fix build with gcc11 >>>> >>>> Reproduced with '--buildtype=debugoptimized' config, >>>> compiler version: gcc (GCC) 12.0.0 20210509 (experimental) >>>> >>>> There are multiple build errors, like: >>>> ../drivers/net/ice/base/ice_switch.c: In function ‘ice_add_marker_act’: >>>> ../drivers/net/ice/base/ice_switch.c:3727:15: >>>> warning: array subscript ‘struct ice_aqc_sw_rules_elem[0]’ >>>> is partly outside array bounds of ‘unsigned char[52]’ >>>> [-Warray-bounds] >>>> 3727 | lg_act->type = CPU_TO_LE16(ICE_AQC_SW_RULES_T_LG_ACT); >>>> | ^~ >>>> In file included from ../drivers/net/ice/base/ice_type.h:52, >>>> from ../drivers/net/ice/base/ice_common.h:8, >>>> from ../drivers/net/ice/base/ice_switch.h:8, >>>> from ../drivers/net/ice/base/ice_switch.c:5: >>>> ../drivers/net/ice/base/ice_osdep.h:209:29: >>>> note: referencing an object of size 52 allocated by ‘rte_zmalloc’ >>>> 209 | #define ice_malloc(h, s) rte_zmalloc(NULL, s, 0) >>>> | ^~~~~~~~~~~~~~~~~~~~~~~ >>>> ../drivers/net/ice/base/ice_switch.c:3720:50: >>>> note: in expansion of macro ‘ice_malloc’ >>>> lg_act = (struct ice_aqc_sw_rules_elem *)ice_malloc(hw, rules_size); >>>> >>>> These errors are mainly because allocated memory is cast to >>>> "struct ice_aqc_sw_rules_elem *" but allocated size is less than the size >>>> of "struct ice_aqc_sw_rules_elem". >>>> >>>> "struct ice_aqc_sw_rules_elem" has multiple other structs has unions, >>>> based on which one is used allocated memory being less than the size of >>>> "struct ice_aqc_sw_rules_elem" is logically correct but compiler is >>>> complaining about it. >>>> >>>> As a solution making sure allocated memory size is at least size of >>>> "struct ice_aqc_sw_rules_elem". >>>> The function to use the struct is 'ice_aq_sw_rules()', and it already has >>>> parameter for size of the rule, allocating more than needed shouldn't >>>> cause any problem. >>>> >>>> Fixes: c7dd15931183 ("net/ice/base: add virtual switch code") >>>> Fixes: 02acdce2f553 ("net/ice/base: add MAC filter with marker and >>>> counter") >>>> Fixes: f89aa3affa9e ("net/ice/base: support removing advanced rule") >>>> Cc: [email protected] >>>> >>>> Signed-off-by: Ferruh Yigit <[email protected]> >>>> --- >>>> Cc: [email protected] >>>> Cc: [email protected] >>>> Cc: [email protected] >>>> Cc: Kevin Traynor <[email protected]> >>>> Cc: Ajit Khaparde <[email protected]> >>>> --- >>>> drivers/net/ice/base/ice_switch.c | 30 +++++++++++++++++++++++------- >>>> 1 file changed, 23 insertions(+), 7 deletions(-) >>> >>> GCC bug ? >>> >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98266 >>> >>> Bug 98266 - [11 Regression] bogus array subscript is partly outside array >>> bounds on virtual >> inheritance >>> >> >> I am not sure if this is a gcc defect. >> >> Here there is a memory allocated and assigned to "struct >> ice_aqc_sw_rules_elem >> *", but allocated memory size is less than the struct size. As far as I >> understand this is the reason of compiler warning. >> >> For this case it may not be problem logically since both who allocates memory >> and who uses the memory follows a contract, but there is a mismatch between >> pointer type and object. If some other function wants to access all fields of >> the struct, it will be out of bound access. >> > > That's why they are static function, not API. These functions are designed for > internally using to handle the compact memory layout with different types. > > These XXX_SIZE is a helper to get the different memory type size. Of course, > this should be very careful, in other words, as you said: follows a contract. > ;-) >
As said above the code is logically correct (at least as far as I can see), but it can be dangerous from programming perspective. > > #define DUMMY_ETH_HDR_LEN 16 > #define ICE_SW_RULE_RX_TX_ETH_HDR_SIZE \ > (offsetof(struct ice_aqc_sw_rules_elem, pdata.lkup_tx_rx.hdr) + \ > (DUMMY_ETH_HDR_LEN * \ > sizeof(((struct ice_sw_rule_lkup_rx_tx *)0)->hdr[0]))) > #define ICE_SW_RULE_RX_TX_NO_HDR_SIZE \ > (offsetof(struct ice_aqc_sw_rules_elem, pdata.lkup_tx_rx.hdr)) > #define ICE_SW_RULE_LG_ACT_SIZE(n) \ > (offsetof(struct ice_aqc_sw_rules_elem, pdata.lg_act.act) + \ > ((n) * sizeof(((struct ice_sw_rule_lg_act *)0)->act[0]))) > #define ICE_SW_RULE_VSI_LIST_SIZE(n) \ > (offsetof(struct ice_aqc_sw_rules_elem, pdata.vsi_list.vsi) + \ > ((n) * sizeof(((struct ice_sw_rule_vsi_list *)0)->vsi[0]))) > > > >> >> >
