mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net-next v3] ice: Adjust over allocation of memory in ice_sched_add_root_node() and ice_sched_add_node()
@ 2024-07-08 18:27 Aleksandr Mishin
  2024-07-09  8:02 ` Przemek Kitszel
  2024-07-09  8:49 ` [Intel-wired-lan] " Paul Menzel
  0 siblings, 2 replies; 8+ messages in thread
From: Aleksandr Mishin @ 2024-07-08 18:27 UTC (permalink / raw)
  To: Anirudh Venkataramanan
  Cc: Aleksandr Mishin, Jesse Brandeburg, Tony Nguyen, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, intel-wired-lan,
	netdev, linux-kernel, lvc-project, Przemek Kitszel

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(-)

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;
-- 
2.30.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next v3] ice: Adjust over allocation of memory in ice_sched_add_root_node() and ice_sched_add_node()
  2024-07-08 18:27 [PATCH net-next v3] ice: Adjust over allocation of memory in ice_sched_add_root_node() and ice_sched_add_node() Aleksandr Mishin
@ 2024-07-09  8:02 ` Przemek Kitszel
  2024-07-09  8:49 ` [Intel-wired-lan] " Paul Menzel
  1 sibling, 0 replies; 8+ messages in thread
From: Przemek Kitszel @ 2024-07-09  8:02 UTC (permalink / raw)
  To: Aleksandr Mishin
  Cc: Jesse Brandeburg, Tony Nguyen, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, intel-wired-lan, netdev,
	linux-kernel, lvc-project

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;


^ permalink raw reply	[flat|nested] 8+ messages in thread

* 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()
  2024-07-08 18:27 [PATCH net-next v3] ice: Adjust over allocation of memory in ice_sched_add_root_node() and ice_sched_add_node() Aleksandr Mishin
  2024-07-09  8:02 ` Przemek Kitszel
@ 2024-07-09  8:49 ` Paul Menzel
  2024-07-09  8:54   ` Paul Menzel
  1 sibling, 1 reply; 8+ messages in thread
From: Paul Menzel @ 2024-07-09  8:49 UTC (permalink / raw)
  To: Aleksandr Mishin
  Cc: Anirudh Venkataramanan, lvc-project, intel-wired-lan,
	linux-kernel, Przemek Kitszel, Eric Dumazet, Tony Nguyen, netdev,
	Jakub Kicinski, Paolo Abeni, David S. Miller, Simon Horman

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* 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()
  2024-07-09  8:49 ` [Intel-wired-lan] " Paul Menzel
@ 2024-07-09  8:54   ` Paul Menzel
  2024-07-09  9:11     ` Przemek Kitszel
  0 siblings, 1 reply; 8+ messages in thread
From: Paul Menzel @ 2024-07-09  8:54 UTC (permalink / raw)
  To: Aleksandr Mishin
  Cc: lvc-project, Przemek Kitszel, Eric Dumazet, linux-kernel,
	Jakub Kicinski, intel-wired-lan, Simon Horman, netdev,
	Tony Nguyen, Paolo Abeni, David S. Miller

[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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* 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()
  2024-07-09  8:54   ` Paul Menzel
@ 2024-07-09  9:11     ` Przemek Kitszel
  2024-07-09  9:50       ` Paul Menzel
  0 siblings, 1 reply; 8+ messages in thread
From: Przemek Kitszel @ 2024-07-09  9:11 UTC (permalink / raw)
  To: Paul Menzel, Aleksandr Mishin
  Cc: lvc-project, Eric Dumazet, linux-kernel, Jakub Kicinski,
	intel-wired-lan, Simon Horman, netdev, Tony Nguyen, Paolo Abeni,
	David S. Miller

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* 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()
  2024-07-09  9:11     ` Przemek Kitszel
@ 2024-07-09  9:50       ` Paul Menzel
  2024-07-09 10:25         ` Przemek Kitszel
  0 siblings, 1 reply; 8+ messages in thread
From: Paul Menzel @ 2024-07-09  9:50 UTC (permalink / raw)
  To: Przemek Kitszel, Aleksandr Mishin
  Cc: lvc-project, Tony Nguyen, netdev, linux-kernel, Eric Dumazet,
	intel-wired-lan, Simon Horman, Jakub Kicinski, Paolo Abeni,
	David S. Miller

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* 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()
  2024-07-09  9:50       ` Paul Menzel
@ 2024-07-09 10:25         ` Przemek Kitszel
  2024-07-09 11:35           ` Aleksandr Mishin
  0 siblings, 1 reply; 8+ messages in thread
From: Przemek Kitszel @ 2024-07-09 10:25 UTC (permalink / raw)
  To: Paul Menzel, Aleksandr Mishin
  Cc: lvc-project, Tony Nguyen, netdev, linux-kernel, Eric Dumazet,
	intel-wired-lan, Simon Horman, Jakub Kicinski, Paolo Abeni,
	David S. Miller

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* 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()
  2024-07-09 10:25         ` Przemek Kitszel
@ 2024-07-09 11:35           ` Aleksandr Mishin
  0 siblings, 0 replies; 8+ messages in thread
From: Aleksandr Mishin @ 2024-07-09 11:35 UTC (permalink / raw)
  To: Przemek Kitszel, Paul Menzel
  Cc: lvc-project, Tony Nguyen, netdev, linux-kernel, Eric Dumazet,
	intel-wired-lan, Simon Horman, Jakub Kicinski, Paolo Abeni,
	David S. Miller



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
> 

-- 
Kind regards
Aleksandr

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-07-09 11:36 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-07-08 18:27 [PATCH net-next v3] ice: Adjust over allocation of memory in ice_sched_add_root_node() and ice_sched_add_node() Aleksandr Mishin
2024-07-09  8:02 ` Przemek Kitszel
2024-07-09  8:49 ` [Intel-wired-lan] " Paul Menzel
2024-07-09  8:54   ` Paul Menzel
2024-07-09  9:11     ` Przemek Kitszel
2024-07-09  9:50       ` Paul Menzel
2024-07-09 10:25         ` Przemek Kitszel
2024-07-09 11:35           ` Aleksandr Mishin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®