Messages in this thread | | | Date | Wed, 26 Jul 2023 09:38:52 +0200 | From | Simon Horman <> | Subject | Re: [PATCH net-next] flow_dissector: Add IPSEC dissectors |
| |
On Wed, Jul 26, 2023 at 06:34:34AM +0000, Ratheesh Kannoth wrote: > > From: Simon Horman <simon.horman@corigine.com> > > 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. > >
Hi Ratheesh,
> 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
I ran:
make C=2 net/core/flow_dissector.o
FWIIW, I also saw the warning using make C=2 net/core/ V=s
> sparse version is 0.6.4
I am using sparse git commit ce1a6720f69e, because at one point that was the newest revision at a time the packaged version I had wasn't working for me, and that was the latest commit at the time.
https://git.kernel.org/pub/scm/devel/sparse/sparse.git/commit/ce1a6720f69e
> 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.
I might go for u64. But yes, I suspect making the variable wider is a viable solution.
Given that it is widely used it would be good to inspect how it is used to make sure this change makes sense.
I would make this change a separate patch if it is more than one line.
Also, while you are there, can you fix the spelling mistake in the comment on the 'used_keys' line in struct flow_dissector ? It's the same line that needs to be changed to change the type of 'used_keys'.
| |