RE: Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors

[Ratheesh Kannoth]

> From: Simon Horman <simon.horman@xxxxxxxxxxxx>
> Sent: Wednesday, July 26, 2023 1:21 AM
> Subject: [EXT] Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors


> >  	FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct
> flow_dissector_key_num_of_vlans */
> >  	FLOW_DISSECTOR_KEY_PPPOE, /* struct flow_dissector_key_pppoe
> */
> >  	FLOW_DISSECTOR_KEY_L2TPV3, /* struct flow_dissector_key_l2tpv3
> */
> > +	FLOW_DISSECTOR_KEY_IPSEC, /* struct flow_dissector_key_ipsec */
> >  	FLOW_DISSECTOR_KEY_CFM, /* struct flow_dissector_key_cfm */
> >
> >  	FLOW_DISSECTOR_KEY_MAX,
> 
> ...
> 
> Hi Ratheesh,
> 
> With this change, this enum now has 33 values, excluding
> FLOW_DISSECTOR_KEY_MAX.  I.e the range of values is from 0 to 32.
> 
> But dissector_uses_key() looks like this:
> 
> 
> static inline bool dissector_uses_key(const struct flow_dissector
> *flow_dissector,
>                                       enum flow_dissector_key_id key_id) {
>         return flow_dissector->used_keys & (1 << key_id); }
> 
> And the type of the used_keys field of struct flow_dissector is unsigned int, a
> 32bit entity.
> 
> So an overflow will now occur if key_id is FLOW_DISSECTOR_KEY_CFM.
> 
> This is flagged by Sparse.
> 

Thank you !
1)  How did you run sparse to detect this error. When I ran  below command, it did not throw this error/warning ?
       make  C=2 net/core/ V=s
      sparse version is 0.6.4
2)   Is it okay to change  variable type of  "used_keys"  from "unsigned int" to "unsigned long long" to accommodate this.
       This variable is used at lot of places in the code. 

-Ratheesh 

> --
> pw-bot: changes-requested