| Message ID | 20240708182736.8514-1-amishin@t-argos.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next,v3] ice: Adjust over allocation of memory in ice_sched_add_root_node() and ice_sched_add_node() | expand |
On 7/8/24 20:27, Aleksandr Mishin wrote: > 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. > > Adjust over allocation of memory by correcting types in devm_kcalloc() > sizeof() arguments. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Suggested-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Suggested-by tag is for when the Suggester is the raison d'etre of the commit, not just improved it via review. Here there is no need for this tag. As an example: We don't like the current state with the abuse of devm_ family of allocations in the ice driver, those are used in places where plain kzalloc() will fit better. With the previous paragraph you could go to review our driver and swap devm_kzalloc() for kzalloc() in some cases, "proving" that this is a correct change; and finally add my Suggested-by. You could've done the same without the tag if you will instead assume that this suggestion was too obvious or too broad or too trivial, but it is just an example:) > Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> I like the new code, so: Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > --- > v3: > - Update comment and use the correct entities as suggested by Przemek > v2: https://lore.kernel.org/all/20240706140518.9214-1-amishin@t-argos.ru/ > - 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@kernel.org/) > v1: https://lore.kernel.org/all/20240705163620.12429-1-amishin@t-argos.ru/ > > 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;
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@intel.com> > Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> > --- > v3: > - Update comment and use the correct entities as suggested by Przemek > v2: https://lore.kernel.org/all/20240706140518.9214-1-amishin@t-argos.ru/ > - 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@kernel.org/) > v1: https://lore.kernel.org/all/20240705163620.12429-1-amishin@t-argos.ru/ > > 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
[Cc: -anirudh.venkataramanan@intel.com (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@intel.com> >> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> >> --- >> v3: >> - Update comment and use the correct entities as suggested by Przemek >> v2: https://lore.kernel.org/all/20240706140518.9214-1-amishin@t-argos.ru/ >> - 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@kernel.org/) >> v1: >> https://lore.kernel.org/all/20240705163620.12429-1-amishin@t-argos.ru/ >> >> 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
On 7/9/24 10:54, Paul Menzel wrote: > [Cc: -anirudh.venkataramanan@intel.com (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. this version does not have any SHA mentioned :) >> >> `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. so, this is not a "fix" per definition of a fix: "your patch changes observable misbehavior" If the over-allocation would be counted in megabytes, then it will be a different case.
Dear Przemek, Thank you for your quick reply. Am 09.07.24 um 11:11 schrieb Przemek Kitszel: > On 7/9/24 10:54, Paul Menzel wrote: >> [Cc: -anirudh.venkataramanan@intel.com (Address rejected)] >> >> Am 09.07.24 um 10:49 schrieb Paul Menzel: >>> 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. > > this version does not have any SHA mentioned :) Sorry, I don’t understand your answer. What SHA do you mean? >>> `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. > > so, this is not a "fix" per definition of a fix: "your patch changes > observable misbehavior" > If the over-allocation would be counted in megabytes, then it will > be a different case. The quoted text just talks about “an issue”. What definition do you refer to? Kind regards, Paul
On 7/9/24 11:50, Paul Menzel wrote: > Dear Przemek, > > > Thank you for your quick reply. > > > Am 09.07.24 um 11:11 schrieb Przemek Kitszel: >> On 7/9/24 10:54, Paul Menzel wrote: >>> [Cc: -anirudh.venkataramanan@intel.com (Address rejected)] >>> >>> Am 09.07.24 um 10:49 schrieb Paul Menzel: > >>>> 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. >> >> this version does not have any SHA mentioned :) > > Sorry, I don’t understand your answer. What SHA do you mean? there is no commit cited by Aleksandr in v3, IIRC there was one in v1 I agree that mention would be valuable, and we still want v4 with my Suggested-by dropped anyway :) > >>>> `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. >> >> so, this is not a "fix" per definition of a fix: "your patch changes >> observable misbehavior" >> If the over-allocation would be counted in megabytes, then it will >> be a different case. > > The quoted text just talks about “an issue”. What definition do you > refer to? I mean that there is no issue (for the users), thus no fix. Example of recently merged "not fix", with more links to other "non- fixes": https://lore.kernel.org/all/b836eb8ca8abf2f64478da48d250405bb1d90ad5.camel@sipsolutions.net/T/ > > > Kind regards, > > Paul
On 09.07.2024 13:25, Przemek Kitszel wrote: > On 7/9/24 11:50, Paul Menzel wrote: >> Dear Przemek, >> >> >> Thank you for your quick reply. >> >> >> Am 09.07.24 um 11:11 schrieb Przemek Kitszel: >>> On 7/9/24 10:54, Paul Menzel wrote: >>>> [Cc: -anirudh.venkataramanan@intel.com (Address rejected)] >>>> >>>> Am 09.07.24 um 10:49 schrieb Paul Menzel: >> >>>>> 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. One pointer instance size is 8 bytes. One structure instance size is (approximately) 104 bytes. I'm not quite sure for that number, because structure is complex and includes another structure, which includes another etc. So I could make a mistake in calculation. Memory allocation is performed for multiple instances, so this ~96 bytes should be multiplied by a number of instances to get final memory overhead size. >>>>> >>>>>> 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. >>> >>> this version does not have any SHA mentioned :) >> >> Sorry, I don’t understand your answer. What SHA do you mean? > > there is no commit cited by Aleksandr in v3, IIRC there was one in v1 > > I agree that mention would be valuable, and we still want v4 with my > Suggested-by dropped anyway :) I'm working on v4, but I must wait 24 hours from v3 according to netdev rules: https://docs.kernel.org/process/maintainer-netdev.html. In v4 I'll drop "Suggested-by" :) But I'm a little confused whether to include "Fixes" tag into v4, because this is not an issue for the users as Simon and Przemek wrote? I would be grateful if you could tell me what else to change to avoid later v5 release :) > >> >>>>> `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. >>> >>> so, this is not a "fix" per definition of a fix: "your patch changes >>> observable misbehavior" >>> If the over-allocation would be counted in megabytes, then it will >>> be a different case. >> >> The quoted text just talks about “an issue”. What definition do you >> refer to? > > I mean that there is no issue (for the users), thus no fix. > Example of recently merged "not fix", with more links to other "non- > fixes": > https://lore.kernel.org/all/b836eb8ca8abf2f64478da48d250405bb1d90ad5.camel@sipsolutions.net/T/ > >> >> >> Kind regards, >> >> Paul >
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;
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. Adjust over allocation of memory by correcting types in devm_kcalloc() sizeof() arguments. Found by Linux Verification Center (linuxtesting.org) with SVACE. Suggested-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> --- v3: - Update comment and use the correct entities as suggested by Przemek v2: https://lore.kernel.org/all/20240706140518.9214-1-amishin@t-argos.ru/ - 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@kernel.org/) v1: https://lore.kernel.org/all/20240705163620.12429-1-amishin@t-argos.ru/ drivers/net/ethernet/intel/ice/ice_sched.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)