Fixes to mlxbf-pmc
Subsystems (2)
v4 cover Fixes to mlxbf-pmc
This submission contains 2 patches:
Patch 1 fixes an issue with matching an input event_name string with the list of supported events in the event_list due to the trailing newline char.
Patch 2 adds checks to validate the input for event and enable fields, and prevents the user from writing invalid values to these fields.
v1 -> v2
Split single patch into 2 patches addressing each fix separately
v2 -> v3
Patch 1: Remove the trailing newline character before comparing strings Patch 2: Use kstrtobool() instead of kstrtouint() which would validate the input
v3 -> v4
Patch 1: Use kstrdup_and_replace() to remove the newline char.
Shravan Kumar Ramani (2):
platform/mellanox: mlxbf-pmc: Remove newline char from event name
input
platform/mellanox: mlxbf-pmc: Validate event/enable input
drivers/platform/mellanox/mlxbf-pmc.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
Signature · 1 line
2.43.2
PATCH v4 1/2 platform/mellanox: mlxbf-pmc: Remove newline char from event name input
Since the input string passed via the command line appends a newline char, it needs to be removed before comparison with the event_list.
--- drivers/platform/mellanox/mlxbf-pmc.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/platform/mellanox/mlxbf-pmc.c +9 -1
@@ -15,6 +15,7 @@#include <linux/hwmon.h> #include <linux/platform_device.h> #include <linux/string.h> +#include <linux/string_helpers.h> #include <uapi/linux/psci.h> #define MLXBF_PMC_WRITE_REG_32 0x82000009
@@ -1784,6 +1785,7 @@ static ssize_t mlxbf_pmc_event_store(struct device *dev, attr, struct mlxbf_pmc_attribute, dev_attr);unsigned int blk_num, cnt_num;bool is_l3 = false; +char *evt_name;int evt_num;int err;
@@ -1791,8 +1793,14 @@ static ssize_t mlxbf_pmc_event_store(struct device *dev, cnt_num = attr_event->index;if (isalpha (buf[0 ])) { +/* Remove the trailing newline character if present */ + evt_name =kstrdup_and_replace (buf,'\n' ,'\0' ,GFP_KERNEL ); +if (!evt_name) +return -ENOMEM ; + evt_num =mlxbf_pmc_get_event_num (pmc->block_name[blk_num], - buf); + evt_name); +kfree (evt_name);if (evt_num <0 )return -EINVAL ; }else {
Signature · 1 line
2.43.2
PATCH v4 2/2 platform/mellanox: mlxbf-pmc: Validate event/enable input
In reply to Shravan Kumar RamaniBefore programming the event info, validate the event number received as input by checking if it exists in the event_list. Also fix a typo in the comment for mlxbf_pmc_get_event_name() to correctly mention that it returns the event name when taking the event number as input, and not the other way round. For setting the enable value, the input should be 0 or 1 only. Use kstrtobool() in place of kstrtoint() in mlxbf_pmc_enable_store() to accept only valid input.
--- drivers/platform/mellanox/mlxbf-pmc.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
drivers/platform/mellanox/mlxbf-pmc.c +8 -7
@@ -1223,7 +1223,7 @@ static int mlxbf_pmc_get_event_num(const char *blk, const char *evt)return -ENODEV ; } -/* Get the event number given the name */ +/* Get the event name given the number */ static char *mlxbf_pmc_get_event_name (const char *blk,u32 evt) {const struct mlxbf_pmc_events *events;
@@ -1807,6 +1807,9 @@ static ssize_t mlxbf_pmc_event_store(struct device *dev, err =kstrtouint (buf,0 , &evt_num);if (err <0 )return err; + +if (!mlxbf_pmc_get_event_name (pmc->block_name[blk_num], evt_num)) +return -EINVAL ; }if (strstr (pmc->block_name[blk_num],"l3cache" ))
@@ -1887,13 +1890,14 @@ static ssize_t mlxbf_pmc_enable_store(struct device *dev, {struct mlxbf_pmc_attribute *attr_enable =container_of ( attr, struct mlxbf_pmc_attribute, dev_attr); -unsigned int en, blk_num; +unsigned int blk_num;u32 word;int err; +bool en; blk_num = attr_enable->nr; - err =kstrtouint (buf,0 , &en); + err =kstrtobool (buf, &en);if (err <0 )return err;
@@ -1913,14 +1917,11 @@ static ssize_t mlxbf_pmc_enable_store(struct device *dev,MLXBF_PMC_CRSPACE_PERFMON_CTL (pmc->block[blk_num].counters), MLXBF_PMC_WRITE_REG_32, word); }else { -if (en && en !=1 ) -return -EINVAL ; - err =mlxbf_pmc_config_l3_counters (blk_num, false, !!en);if (err)return err; -if (en ==1 ) { +if (en) { err =mlxbf_pmc_config_l3_counters (blk_num, true, false);if (err)return err;
Signature · 1 line
2.43.2
Ilpo Järvinen
maintainerRe: [PATCH v4 2/2] platform/mellanox: mlxbf-pmc: Validate event/enable input
Quoted 61 lines · Shravan Kumar Ramani
Before programming the event info, validate the event number received as input by checking if it exists in the
event_list. Also fix a typo in the comment formlxbf_pmc_get_event_name()to correctly mention that it returns the event name when taking the event number as input, and not the other way round. For setting the enable value, the input should be 0 or 1 only. Usekstrtobool()in place ofkstrtoint()inmlxbf_pmc_enable_store()to accept only valid input.Fixes 423c3361855c ("platform/mellanox: mlxbf-pmc: Add support for BlueField-3")Signed-off-by Shravan Kumar Ramani <[email protected]>Reviewed-by David Thompson <[email protected]>--- drivers/platform/mellanox/mlxbf-pmc.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)drivers/platform/mellanox/mlxbf-pmc.c +7
-5@@ -1223,7 +1223,7 @@ static int mlxbf_pmc_get_event_num(const char *blk, const char *evt)return -ENODEV ; } -/* Get the event number given the name */ +/* Get the event name given the number */ static char *mlxbf_pmc_get_event_name (const char *blk,u32 evt) {const struct mlxbf_pmc_events *events;@@ -1807,6 +1807,9 @@ static ssize_t mlxbf_pmc_event_store(struct device *dev, err =kstrtouint (buf,0 , &evt_num);if (err <0 )return err; + +if (!mlxbf_pmc_get_event_name (pmc->block_name[blk_num], evt_num)) +return -EINVAL ; }if (strstr (pmc->block_name[blk_num],"l3cache" ))@@ -1887,13 +1890,14 @@ static ssize_t mlxbf_pmc_enable_store(struct device *dev, {struct mlxbf_pmc_attribute *attr_enable =container_of ( attr, struct mlxbf_pmc_attribute, dev_attr); -unsigned int en, blk_num; +unsigned int blk_num;u32 word;int err; +bool en; blk_num = attr_enable->nr; - err =kstrtouint (buf,0 , &en); + err =kstrtobool (buf, &en);if (err <0 )return err;@@ -1913,14 +1917,11 @@ static ssize_t mlxbf_pmc_enable_store(struct device *dev,MLXBF_PMC_CRSPACE_PERFMON_CTL (pmc->block[blk_num].counters), MLXBF_PMC_WRITE_REG_32, word); }else { -if (en && en !=1 ) -return -EINVAL ;
drivers/platform/mellanox/mlxbf-pmc.c static ssize_t mlxbf_pmc_enable_store(struct device *dev, · line 1927
Quoted 10 lines
drivers/platform/mellanox/mlxbf-pmc.c +1
-2- err =mlxbf_pmc_config_l3_counters (blk_num, false, !!en);if (err)return err; -if (en ==1 ) { +if (en) { err =mlxbf_pmc_config_l3_counters (blk_num, true, false);if (err)return err;
Hi,
I've applied this series to the review-ilpo-fixes branch but I had to split the kstrbool() change to own commit, it's very apparent these two changes should have been separate right from the start (and I even asked you to split this earlier).
Whenever making changes, especially fixes, please try hard put separate changes into own patches. That should be done even if the changes touch same file, and they may even look similar such as here, both are doing "input validation", but the cases were still clearly different.
It's easier to review, justify in the changelog, etc. when the change is very focused on a single problem only.
Signature · 1 line
i.