Hi Sven, On Tue, Apr 02, 2024 at 09:06:35PM +0200, Sven Eckelmann wrote: > On Tuesday, 2 April 2024 19:23:01 CEST Erick Archer wrote: > > The "struct batadv_tvlv_tt_data" uses a dynamically sized set of > > trailing elements. Specifically, it uses an array of structures of type > > "batadv_tvlv_tt_vlan_data". So, use the preferred way in the kernel > > declaring a flexible array [1]. > > > > The order in which the structure batadv_tvlv_tt_data and the structure > > batadv_tvlv_tt_vlan_data are defined must be swap to avoid an incomplete > > type error. > > > > Also, avoid the open-coded arithmetic in memory allocator functions [2] > > using the "struct_size" macro and use the "flex_array_size" helper to > > clarify some calculations, when possible. > > > > Moreover, the new structure member also allow us to avoid the open-coded > > arithmetic on pointers in some situations. Take advantage of this. > > > > This code was detected with the help of Coccinelle, and audited and > > modified manually. > > > > Link: > > https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays > > [1] > > Link: > > https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > > [2] > > Signed-off-by: Erick Archer <[email protected]> > > > --- > > Hi, > > > > I would like to add the "__counted_by(num_vlan)" tag to the new flex member > > but I don't know if this line can affect it. > > > > ntohs(tt_data->num_vlan) > > > Yes, num_vlan is a __be16. I could only identify the kernel-doc related > scripts as consumer. But maybe they are more - so I would defer this question > to kernel-hardening.
Thanks for the info. > > And with this change, I get a lot of additional warnings (-Wsparse-all) > > > cfg: BLA=n DAT=y DEBUG=y TRACING=n NC=y MCAST=n BATMAN_V=n > net/batman-adv/translation-table.c:574:21: warning: using sizeof on a > flexible structure > net/batman-adv/translation-table.c:859:25: warning: using sizeof on a > flexible structure > net/batman-adv/translation-table.c:859:25: warning: using sizeof on a > flexible structure > net/batman-adv/translation-table.c:938:25: warning: using sizeof on a > flexible structure > net/batman-adv/translation-table.c:938:25: warning: using sizeof on a > flexible structure > net/batman-adv/translation-table.c:2932:16: warning: using sizeof on a > flexible structure > net/batman-adv/translation-table.c:2932:16: warning: using sizeof on a > flexible structure > net/batman-adv/translation-table.c:3378:21: warning: using sizeof on a > flexible structure > net/batman-adv/translation-table.c:3378:21: warning: using sizeof on a > flexible structure > net/batman-adv/translation-table.c:3982:30: warning: using sizeof on a > flexible structure > net/batman-adv/translation-table.c:3986:27: warning: using sizeof on a > flexible structure > net/batman-adv/translation-table.c:4026:30: warning: using sizeof on a > flexible structure > net/batman-adv/translation-table.c:4030:27: warning: using sizeof on a > flexible structure > net/batman-adv/translation-table.c:4032:23: warning: cast from restricted > __be16 > net/batman-adv/translation-table.c:4032:23: warning: restricted __be16 > degrades to integer > net/batman-adv/translation-table.c:4032:23: warning: incorrect type in > argument 1 (different base types) > net/batman-adv/translation-table.c:4032:23: expected unsigned long > [usertype] factor1 > net/batman-adv/translation-table.c:4032:23: got restricted __be16 > [usertype] num_vlan > > [...] I will work on this for the next version. Thanks for share these warnings. > > num_vlan = ntohs(tt_data->num_vlan); > > > > - if (tvlv_value_len < sizeof(*tt_vlan) * num_vlan) > > + flex_size = flex_array_size(tt_data, vlan_data, num_vlan); > > + if (tvlv_value_len < flex_size) > > return; > > This helper would need an #include of <linux/overflow.h> in > net/batman-adv/translation-table.c Understood. > > [....] > > /** > > @@ -4039,8 +4029,7 @@ static int batadv_tt_tvlv_unicast_handler_v1(struct > > batadv_priv *bat_priv, > > tt_data = tvlv_value; > > tvlv_value_len -= sizeof(*tt_data); > > > > - tt_vlan_len = sizeof(struct batadv_tvlv_tt_vlan_data); > > - tt_vlan_len *= ntohs(tt_data->num_vlan); > > + tt_vlan_len = flex_array_size(tt_data, vlan_data, tt_data->num_vlan); > > This is definitely wrong on little endian systems. You first need to convert > num_vlan from network (big endian) to host order. I'm sorry. My bad. I forgot to add the "ntohs". I will fix it for the next version. > > Kind regards, > Sven Regards, Erick
