Re: [Intel-wired-lan] [PATCH net-next v3] ice: Adjust over allocation of memory in ice_sched_add_root_node() and ice_sched_add_node()

[Paul Menzel]

[Cc: -anirudh.venkataramanan@xxxxxxxxx (Address rejected)]

Am 09.07.24 um 10:49 schrieb Paul Menzel:
> Dear Aleksandr,
>
>
> Thank you for your patch.
>
>
> Am 08.07.24 um 20:27 schrieb Aleksandr Mishin:
> > In ice_sched_add_root_node() and ice_sched_add_node() there are calls to
> > devm_kcalloc() in order to allocate memory for array of pointers to
> > 'ice_sched_node' structure. But incorrect types are used as sizeof()
> > arguments in these calls (structures instead of pointers) which leads to
> > over allocation of memory.
>
> If you have the explicit size at hand, it’d be great if you added those 
> to the commit message.
>
> > Adjust over allocation of memory by correcting types in devm_kcalloc()
> > sizeof() arguments.
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Maybe mention, that Coverity found that too, and the warning was 
> disabled, and use that commit in Fixes: tag? That’d be commit 
> b36c598c999c (ice: Updates to Tx scheduler code), different from the one 
> you used.
>
> `Documentation/process/submitting-patches.rst` says:
>
> > A Fixes: tag indicates that the patch fixes an issue in a previous
> > commit. It is used to make it easy to determine where a bug
> > originated, which can help review a bug fix. This tag also assists
> > the stable kernel team in determining which stable kernel versions
> > should receive your fix. This is the preferred method for indicating
> > a bug fixed by the patch.
>
>
> > Suggested-by: Przemek Kitszel <przemyslaw.kitszel@xxxxxxxxx>
> > Signed-off-by: Aleksandr Mishin <amishin@xxxxxxxxxx>
> > ---
> > v3:
> >    - Update comment and use the correct entities as suggested by Przemek
> > v2: https://lore.kernel.org/all/20240706140518.9214-1-amishin@xxxxxxxxxx/
> >    - Update comment, remove 'Fixes' tag and change the tree from 'net' to
> >      'net-next' as suggested by Simon
> >      (https://lore.kernel.org/all/20240706095258.GB1481495@xxxxxxxxxx/)
> > v1: 
> > https://lore.kernel.org/all/20240705163620.12429-1-amishin@xxxxxxxxxx/
> >
> >   drivers/net/ethernet/intel/ice/ice_sched.c | 6 ++----
> >   1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c 
> > b/drivers/net/ethernet/intel/ice/ice_sched.c
> > index ecf8f5d60292..6ca13c5dcb14 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_sched.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_sched.c
> > @@ -28,9 +28,8 @@ ice_sched_add_root_node(struct ice_port_info *pi,
> >       if (!root)
> >           return -ENOMEM;
> > -    /* coverity[suspicious_sizeof] */
> >       root->children = devm_kcalloc(ice_hw_to_dev(hw), hw->max_children[0],
> > -                      sizeof(*root), GFP_KERNEL);
> > +                      sizeof(*root->children), GFP_KERNEL);
> >       if (!root->children) {
> >           devm_kfree(ice_hw_to_dev(hw), root);
> >           return -ENOMEM;
> > @@ -186,10 +185,9 @@ ice_sched_add_node(struct ice_port_info *pi, u8 
> > layer,
> >       if (!node)
> >           return -ENOMEM;
> >       if (hw->max_children[layer]) {
> > -        /* coverity[suspicious_sizeof] */
> >           node->children = devm_kcalloc(ice_hw_to_dev(hw),
> >                             hw->max_children[layer],
> > -                          sizeof(*node), GFP_KERNEL);
> > +                          sizeof(*node->children), GFP_KERNEL);
> >           if (!node->children) {
> >               devm_kfree(ice_hw_to_dev(hw), node);
> >               return -ENOMEM;
>
>
> Kind regards,
>
> Paul