The size of "i40e_dbg_command_buf" is 256, the size of "name"
depends on "IFNAMSIZ", plus a null character and format size,
the total size is more than 256.
Improve readability and maintainability by replacing a hardcoded string
allocation and formatting by the use of the kasprintf() helper.
Fixes: 02e9c290814c ("i40e: debugfs interface")
Suggested-by: Simon Horman <[email protected]>
Suggested-by: Alexander Lobakin <[email protected]>
Suggested-by: Tony Nguyen <[email protected]>
Cc: Kunwu Chan <[email protected]>
Signed-off-by: Kunwu Chan <[email protected]>
---
v2
- Update the size calculation with IFNAMSIZ and sizeof(i40e_dbg_command_buf)
v3
- Use kasprintf to improve readability and maintainability
v4
- Fix memory leak in error path
v5
- Change the order of labels
---
.../net/ethernet/intel/i40e/i40e_debugfs.c | 20 ++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
index 88240571721a..78a7200211b2 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
@@ -72,29 +72,31 @@ static ssize_t i40e_dbg_command_read(struct file *filp, char __user *buffer,
{
struct i40e_pf *pf = filp->private_data;
int bytes_not_copied;
- int buf_size = 256;
char *buf;
int len;
/* don't allow partial reads */
if (*ppos != 0)
return 0;
- if (count < buf_size)
- return -ENOSPC;
- buf = kzalloc(buf_size, GFP_KERNEL);
+ buf = kasprintf(GFP_KERNEL, "%s: %s\n",
+ pf->vsi[pf->lan_vsi]->netdev->name,
+ i40e_dbg_command_buf);
if (!buf)
return -ENOSPC;
- len = snprintf(buf, buf_size, "%s: %s\n",
- pf->vsi[pf->lan_vsi]->netdev->name,
- i40e_dbg_command_buf);
+ len = strlen(buf) + 1;
+ if (count < len)
+ bytes_not_copied = -ENOSPC;
+ else if (copy_to_user(buffer, buf, len))
+ bytes_not_copied = -EFAULT;
+ else
+ bytes_not_copied = 0;
- bytes_not_copied = copy_to_user(buffer, buf, len);
kfree(buf);
if (bytes_not_copied)
- return -EFAULT;
+ return bytes_not_copied;
*ppos = len;
return len;
--
2.39.2
From: Kunwu Chan <[email protected]>
Date: Fri, 8 Dec 2023 11:19:50 +0800
> The size of "i40e_dbg_command_buf" is 256, the size of "name"
> depends on "IFNAMSIZ", plus a null character and format size,
> the total size is more than 256.
>
> Improve readability and maintainability by replacing a hardcoded string
> allocation and formatting by the use of the kasprintf() helper.
>
> Fixes: 02e9c290814c ("i40e: debugfs interface")
> Suggested-by: Simon Horman <[email protected]>
> Suggested-by: Alexander Lobakin <[email protected]>
> Suggested-by: Tony Nguyen <[email protected]>
> Cc: Kunwu Chan <[email protected]>
> Signed-off-by: Kunwu Chan <[email protected]>
Reviewed-by: Alexander Lobakin <[email protected]>
Thanks,
Olek
On Fri, Dec 08, 2023 at 11:19:50AM +0800, Kunwu Chan wrote:
> The size of "i40e_dbg_command_buf" is 256, the size of "name"
> depends on "IFNAMSIZ", plus a null character and format size,
> the total size is more than 256.
>
> Improve readability and maintainability by replacing a hardcoded string
> allocation and formatting by the use of the kasprintf() helper.
>
> Fixes: 02e9c290814c ("i40e: debugfs interface")
> Suggested-by: Simon Horman <[email protected]>
> Suggested-by: Alexander Lobakin <[email protected]>
> Suggested-by: Tony Nguyen <[email protected]>
> Cc: Kunwu Chan <[email protected]>
> Signed-off-by: Kunwu Chan <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
> -----Original Message-----
> From: Intel-wired-lan <[email protected]> On Behalf Of Kunwu Chan
> Sent: Friday, December 8, 2023 8:50 AM
> To: Brandeburg, Jesse <[email protected]>; Nguyen, Anthony L <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
> Cc: Kunwu Chan <[email protected]>; Kunwu Chan <[email protected]>; [email protected]; [email protected]; Lobakin, Aleksander <[email protected]>; [email protected]; Simon Horman <[email protected]>
> Subject: [Intel-wired-lan] [PATCH v5 iwl-next] i40e: Use correct buffer size in i40e_dbg_command_read
>
> The size of "i40e_dbg_command_buf" is 256, the size of "name"
> depends on "IFNAMSIZ", plus a null character and format size,
> the total size is more than 256.
>
> Improve readability and maintainability by replacing a hardcoded string
> allocation and formatting by the use of the kasprintf() helper.
>
> Fixes: 02e9c290814c ("i40e: debugfs interface")
> Suggested-by: Simon Horman <[email protected]>
> Suggested-by: Alexander Lobakin <[email protected]>
> Suggested-by: Tony Nguyen <[email protected]>
> Cc: Kunwu Chan <[email protected]>
> Signed-off-by: Kunwu Chan <[email protected]>
> ---
> v2
> - Update the size calculation with IFNAMSIZ and sizeof(i40e_dbg_command_buf)
> v3
> - Use kasprintf to improve readability and maintainability
> v4
> - Fix memory leak in error path
> v5
> - Change the order of labels
> ---
> .../net/ethernet/intel/i40e/i40e_debugfs.c | 20 ++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <[email protected]> (A Contingent worker at Intel)
On 12/17/2023 9:54 PM, Pucha, HimasekharX Reddy wrote:
>
>> -----Original Message-----
>> From: Intel-wired-lan <[email protected]> On Behalf Of Kunwu Chan
>> Sent: Friday, December 8, 2023 8:50 AM
>> To: Brandeburg, Jesse <[email protected]>; Nguyen, Anthony L <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
>> Cc: Kunwu Chan <[email protected]>; Kunwu Chan <[email protected]>; [email protected]; [email protected]; Lobakin, Aleksander <[email protected]>; [email protected]; Simon Horman <[email protected]>
>> Subject: [Intel-wired-lan] [PATCH v5 iwl-next] i40e: Use correct buffer size in i40e_dbg_command_read
>>
>> The size of "i40e_dbg_command_buf" is 256, the size of "name"
>> depends on "IFNAMSIZ", plus a null character and format size,
>> the total size is more than 256.
>>
>> Improve readability and maintainability by replacing a hardcoded string
>> allocation and formatting by the use of the kasprintf() helper.
>>
>> Fixes: 02e9c290814c ("i40e: debugfs interface")
>> Suggested-by: Simon Horman <[email protected]>
>> Suggested-by: Alexander Lobakin <[email protected]>
>> Suggested-by: Tony Nguyen <[email protected]>
>> Cc: Kunwu Chan <[email protected]>
>> Signed-off-by: Kunwu Chan <[email protected]>
>> ---
>> v2
>> - Update the size calculation with IFNAMSIZ and sizeof(i40e_dbg_command_buf)
>> v3
>> - Use kasprintf to improve readability and maintainability
>> v4
>> - Fix memory leak in error path
>> v5
>> - Change the order of labels
>> ---
>> .../net/ethernet/intel/i40e/i40e_debugfs.c | 20 ++++++++++---------
>> 1 file changed, 11 insertions(+), 9 deletions(-)
>>
>
> Tested-by: Pucha Himasekhar Reddy <[email protected]> (A Contingent worker at Intel)
>
Much of this debugfs command code was an early driver hack that probably
never should have gone upstream in the form that it did. The
i40e_dbg_command_buf itself was originally meant as a scratchpad to put
the 'last command processed', which was not really very useful, and as a
static global that might be written by any number of instances of i40e
devices, was problematic from the beginning. Now, unless I'm mistaken,
it looks like nothing is writing to the buffer at all anymore, so the
buffer and the i40e_dbg_command_read() callback probably should just all
go away rather than trying to pretty up some useless code.
sln
On 12/19/2023 5:32 PM, Nelson, Shannon wrote:
> On 12/17/2023 9:54 PM, Pucha, HimasekharX Reddy wrote:
>>
>>> -----Original Message-----
>>> From: Intel-wired-lan <[email protected]> On Behalf
>>> Of Kunwu Chan
>>> Subject: [Intel-wired-lan] [PATCH v5 iwl-next] i40e: Use correct
>>> buffer size in i40e_dbg_command_read
>>>
>>> The size of "i40e_dbg_command_buf" is 256, the size of "name"
>>> depends on "IFNAMSIZ", plus a null character and format size,
>>> the total size is more than 256.
>>>
>>> Improve readability and maintainability by replacing a hardcoded string
>>> allocation and formatting by the use of the kasprintf() helper.
>>>
>>> Fixes: 02e9c290814c ("i40e: debugfs interface")
>>> Suggested-by: Simon Horman <[email protected]>
>>> Suggested-by: Alexander Lobakin <[email protected]>
>>> Suggested-by: Tony Nguyen <[email protected]>
>>> Cc: Kunwu Chan <[email protected]>
>>> Signed-off-by: Kunwu Chan <[email protected]>
>>> ---
...
> Much of this debugfs command code was an early driver hack that probably
> never should have gone upstream in the form that it did. The
> i40e_dbg_command_buf itself was originally meant as a scratchpad to put
> the 'last command processed', which was not really very useful, and as a
> static global that might be written by any number of instances of i40e
> devices, was problematic from the beginning. Now, unless I'm mistaken,
> it looks like nothing is writing to the buffer at all anymore, so the
> buffer and the i40e_dbg_command_read() callback probably should just all
> go away rather than trying to pretty up some useless code.
Thanks for the history Shannon. I'm not seeing the buffer used either
so, I agree, we should remove it altogether.
Thanks,
Tony
> sln