This series first converts the debugfs files in the pinctrl subsystem to
octal permissions and then adds a new debugfs file "pinmux-select".
Function name and group name can be written to "pinmux-select" which
will cause the function and group to be activated on the pin controller.
Notes for PATCH v4:
- correct the commit message in the second patch to reference function
and group name instead of integer selectors. Apologies for not fixing
that in v3
- fix typos in cover letter
Notes for PATCH v3:
- add Suggested-by: Andy Shevchenko to the "pinctrl: use to octal
permissions for debugfs files" patch
- change the octal permissions from 0400 to 0444 to correctly match the
symbolic permissions (thanks to Joe Perches and Geert Uytterhoeven)
- note that S_IFREG flag is added to the mode in __debugfs_create_file()
(thanks to Andy for highlighting this and Joe for suggesting I should
add a note to the commit message)
- fix order of the goto labels so that the buffers are freed correctly
as suggested by Dan Carpenter
- move from devm_kzalloc() to kzalloc() as the buffers are only used
inside the pinmux_select() function and not related to the lifetime
of the pin controller device (thanks to Andy for pointing this out)
- correct the pinmux-select example in commit message to use the
function and group name instead of selector (thanks to Geert)
Notes for PATCH v2:
- create patch series that includes patch to switch all the debugfs
files in pinctrl subsystem over to octal permission
- write function name and group name, instead of error-prone selector
numbers, to the 'pinmux-select' file
- switch from static to dynamic allocation for the kernel buffer filled
by strncpy_from_user()
- look up function selector from function name using
pinmux_func_name_to_selector()
- validate group name with get_function_groups() and match_string()
- look up selector for group name with pinctrl_get_group_selector()
Notes for PATCH v1:
- posted seperate patch to switch all the debugfs files in pinctrl
subsystem over to octal permission [1]
- there is no existing documentation for any of the debugfs enteries for
pinctrl, so it seemed to have a bigger scope than just this patch. I
also noticed that rst documentation is confusingly named "pinctl" (no
'r') and started thread about that [2]. Linus suggested chaning that
to 'pin-control'. Thus I am planning a seperate documentation patch
series where the file is renamed, references changed and a section on
the pinctrl debugfs files is added.
Notes for RFC v2 [3]:
- rename debugfs file "pinmux-set" to "pinmux-select"
- renmae pinmux_set_write() to pinmux_select()
- switch from memdup_user_nul() to strncpy_from_user()
- switch from pr_warn() to dev_err()
[1] https://lore.kernel.org/linux-gpio/[email protected]/
[2] https://lore.kernel.org/linux-gpio/20210126050817.GA187797@x1/
[3] https://lore.kernel.org/linux-gpio/[email protected]/
Drew Fustini (2):
pinctrl: use to octal permissions for debugfs files
pinctrl: pinmux: Add pinmux-select debugfs file
drivers/pinctrl/core.c | 6 +--
drivers/pinctrl/pinconf.c | 4 +-
drivers/pinctrl/pinmux.c | 111 +++++++++++++++++++++++++++++++++++++-
3 files changed, 114 insertions(+), 7 deletions(-)
--
2.25.1
Switch over pinctrl debugfs files to use octal permissions as they are
preferred over symbolic permissions. Refer to commit f90774e1fd27
("checkpatch: look for symbolic permissions and suggest octal instead").
Note: S_IFREG flag is added to the mode by __debugfs_create_file()
in fs/debugfs/inode.c
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Drew Fustini <[email protected]>
---
drivers/pinctrl/core.c | 6 +++---
drivers/pinctrl/pinconf.c | 4 ++--
drivers/pinctrl/pinmux.c | 4 ++--
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 3663d87f51a0..02f8710afb9c 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1914,11 +1914,11 @@ static void pinctrl_init_debugfs(void)
return;
}
- debugfs_create_file("pinctrl-devices", S_IFREG | S_IRUGO,
+ debugfs_create_file("pinctrl-devices", 0444,
debugfs_root, NULL, &pinctrl_devices_fops);
- debugfs_create_file("pinctrl-maps", S_IFREG | S_IRUGO,
+ debugfs_create_file("pinctrl-maps", 0444,
debugfs_root, NULL, &pinctrl_maps_fops);
- debugfs_create_file("pinctrl-handles", S_IFREG | S_IRUGO,
+ debugfs_create_file("pinctrl-handles", 0444,
debugfs_root, NULL, &pinctrl_fops);
}
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index 02c075cc010b..d9d54065472e 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -370,9 +370,9 @@ DEFINE_SHOW_ATTRIBUTE(pinconf_groups);
void pinconf_init_device_debugfs(struct dentry *devroot,
struct pinctrl_dev *pctldev)
{
- debugfs_create_file("pinconf-pins", S_IFREG | S_IRUGO,
+ debugfs_create_file("pinconf-pins", 0444,
devroot, pctldev, &pinconf_pins_fops);
- debugfs_create_file("pinconf-groups", S_IFREG | S_IRUGO,
+ debugfs_create_file("pinconf-groups", 0444,
devroot, pctldev, &pinconf_groups_fops);
}
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index bab888fe3f8e..c651b2db0925 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -676,9 +676,9 @@ DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
void pinmux_init_device_debugfs(struct dentry *devroot,
struct pinctrl_dev *pctldev)
{
- debugfs_create_file("pinmux-functions", S_IFREG | S_IRUGO,
+ debugfs_create_file("pinmux-functions", 0444,
devroot, pctldev, &pinmux_functions_fops);
- debugfs_create_file("pinmux-pins", S_IFREG | S_IRUGO,
+ debugfs_create_file("pinmux-pins", 0444,
devroot, pctldev, &pinmux_pins_fops);
}
--
2.25.1
Add "pinmux-select" to debugfs which will activate a function and group
when "<function-name group-name>" are written to the file. The write
operation pinmux_select() handles this by checking that the names map to
valid selectors and then calling ops->set_mux().
The existing "pinmux-functions" debugfs file lists the pin functions
registered for the pin controller. For example:
function: pinmux-uart0, groups = [ pinmux-uart0-pins ]
function: pinmux-mmc0, groups = [ pinmux-mmc0-pins ]
function: pinmux-mmc1, groups = [ pinmux-mmc1-pins ]
function: pinmux-i2c0, groups = [ pinmux-i2c0-pins ]
function: pinmux-i2c1, groups = [ pinmux-i2c1-pins ]
function: pinmux-spi1, groups = [ pinmux-spi1-pins ]
To activate function pinmux-i2c1 and group pinmux-i2c1-pins:
echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select
Signed-off-by: Drew Fustini <[email protected]>
---
drivers/pinctrl/pinmux.c | 107 +++++++++++++++++++++++++++++++++++++++
1 file changed, 107 insertions(+)
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index c651b2db0925..23fa32f0a067 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -673,6 +673,111 @@ void pinmux_show_setting(struct seq_file *s,
DEFINE_SHOW_ATTRIBUTE(pinmux_functions);
DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
+#define PINMUX_MAX_NAME 64
+static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
+ size_t len, loff_t *ppos)
+{
+ struct seq_file *sfile = file->private_data;
+ struct pinctrl_dev *pctldev = sfile->private;
+ const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
+ const char *const *groups;
+ char *buf, *fname, *gname;
+ unsigned int num_groups;
+ int fsel, gsel, ret;
+
+ if (len > (PINMUX_MAX_NAME * 2)) {
+ dev_err(pctldev->dev, "write too big for buffer");
+ return -EINVAL;
+ }
+
+ buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ fname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
+ if (!fname) {
+ ret = -ENOMEM;
+ goto free_buf;
+ }
+
+ gname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto free_fname;
+ }
+
+ ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
+ if (ret < 0) {
+ dev_err(pctldev->dev, "failed to copy buffer from userspace");
+ goto free_gname;
+ }
+ buf[len-1] = '\0';
+
+ ret = sscanf(buf, "%s %s", fname, gname);
+ if (ret != 2) {
+ dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
+ goto free_gname;
+ }
+
+ fsel = pinmux_func_name_to_selector(pctldev, fname);
+ if (fsel < 0) {
+ dev_err(pctldev->dev, "invalid function %s in map table\n", fname);
+ ret = -EINVAL;
+ goto free_gname;
+ }
+
+ ret = pmxops->get_function_groups(pctldev, fsel, &groups, &num_groups);
+ if (ret) {
+ dev_err(pctldev->dev, "no groups for function %d (%s)", fsel, fname);
+ goto free_gname;
+
+ }
+
+ ret = match_string(groups, num_groups, gname);
+ if (ret < 0) {
+ dev_err(pctldev->dev, "invalid group %s", gname);
+ goto free_gname;
+ }
+
+ ret = pinctrl_get_group_selector(pctldev, gname);
+ if (ret < 0) {
+ dev_err(pctldev->dev, "failed to get group selectorL %s", gname);
+ goto free_gname;
+ }
+ gsel = ret;
+
+ ret = pmxops->set_mux(pctldev, fsel, gsel);
+ if (ret) {
+ dev_err(pctldev->dev, "set_mux() failed: %d", ret);
+ goto free_gname;
+ }
+
+ return len;
+
+free_gname:
+ devm_kfree(pctldev->dev, gname);
+free_fname:
+ devm_kfree(pctldev->dev, fname);
+free_buf:
+ devm_kfree(pctldev->dev, buf);
+
+ return ret;
+}
+
+static int pinmux_select_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, NULL, inode->i_private);
+}
+
+static const struct file_operations pinmux_select_ops = {
+ .owner = THIS_MODULE,
+ .open = pinmux_select_open,
+ .read = seq_read,
+ .write = pinmux_select,
+ .llseek = no_llseek,
+ .release = single_release,
+};
+
void pinmux_init_device_debugfs(struct dentry *devroot,
struct pinctrl_dev *pctldev)
{
@@ -680,6 +785,8 @@ void pinmux_init_device_debugfs(struct dentry *devroot,
devroot, pctldev, &pinmux_functions_fops);
debugfs_create_file("pinmux-pins", 0444,
devroot, pctldev, &pinmux_pins_fops);
+ debugfs_create_file("pinmux-select", 0200,
+ devroot, pctldev, &pinmux_select_ops);
}
#endif /* CONFIG_DEBUG_FS */
--
2.25.1
On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> + ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> + if (ret < 0) {
> + dev_err(pctldev->dev, "failed to copy buffer from userspace");
> + goto free_gname;
> + }
> + buf[len-1] = '\0';
> +
> + ret = sscanf(buf, "%s %s", fname, gname);
> + if (ret != 2) {
> + dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> + goto free_gname;
We need a "ret = -EINVAL;" before the goto. sscanf doesn't return error
codes. Normally we would write it like so:
if (sscanf(buf, "%s %s", fname, gname) != 2) {
dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
ret = -EINVAL;
goto free_gname;
}
I'm going to write a Smatch check for this today.
> + }
> +
> + fsel = pinmux_func_name_to_selector(pctldev, fname);
> + if (fsel < 0) {
> + dev_err(pctldev->dev, "invalid function %s in map table\n", fname);
> + ret = -EINVAL;
ret = fsel;
> + goto free_gname;
> + }
> +
regards,
dan carpenter
On Thu, 2021-02-11 at 10:11 +0300, Dan Carpenter wrote:
> On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> > + ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> > + if (ret < 0) {
> > + dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > + goto free_gname;
> > + }
> > + buf[len-1] = '\0';
> > +
> > + ret = sscanf(buf, "%s %s", fname, gname);
> > + if (ret != 2) {
> > + dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > + goto free_gname;
>
> We need a "ret = -EINVAL;" before the goto. sscanf doesn't return error
> codes. Normally we would write it like so:
>
> if (sscanf(buf, "%s %s", fname, gname) != 2) {
> dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> ret = -EINVAL;
> goto free_gname;
> }
>
> I'm going to write a Smatch check for this today.
It's a pretty frequently used style:
$ git grep -P '\w+\s*=\s+sscanf\b' | wc -l
327
A grep with -A5 seems to show most use some additional error assignment
when checking the return value.
$ git grep -P -A5 '\w+\s*=\s+sscanf\b' | grep -P '(?:return|=)\s*\-E' | wc -l
174
On Wed, 2021-02-10 at 14:28 -0800, Drew Fustini wrote:
> Switch over pinctrl debugfs files to use octal permissions as they are
> preferred over symbolic permissions. Refer to commit f90774e1fd27
> ("checkpatch: look for symbolic permissions and suggest octal instead").
>
> Note: S_IFREG flag is added to the mode by __debugfs_create_file()
> in fs/debugfs/inode.c
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Drew Fustini <[email protected]>
> ---
> ?drivers/pinctrl/core.c | 6 +++---
> ?drivers/pinctrl/pinconf.c | 4 ++--
> ?drivers/pinctrl/pinmux.c | 4 ++--
> ?3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 3663d87f51a0..02f8710afb9c 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -1914,11 +1914,11 @@ static void pinctrl_init_debugfs(void)
> ? return;
> ? }
> ?
>
> - debugfs_create_file("pinctrl-devices", S_IFREG | S_IRUGO,
> + debugfs_create_file("pinctrl-devices", 0444,
> ? debugfs_root, NULL, &pinctrl_devices_fops);
> - debugfs_create_file("pinctrl-maps", S_IFREG | S_IRUGO,
> + debugfs_create_file("pinctrl-maps", 0444,
> ? debugfs_root, NULL, &pinctrl_maps_fops);
> - debugfs_create_file("pinctrl-handles", S_IFREG | S_IRUGO,
> + debugfs_create_file("pinctrl-handles", 0444,
> ? debugfs_root, NULL, &pinctrl_fops);
> ?}
Why aren't you also converting this block in the same file?
@@ -1890,11 +1890,11 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
dev_name(pctldev->dev));
return;
}
- debugfs_create_file("pins", S_IFREG | S_IRUGO,
+ debugfs_create_file("pins", S_IFREG | 0444,
device_root, pctldev, &pinctrl_pins_fops);
- debugfs_create_file("pingroups", S_IFREG | S_IRUGO,
+ debugfs_create_file("pingroups", S_IFREG | 0444,
device_root, pctldev, &pinctrl_groups_fops);
- debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
+ debugfs_create_file("gpio-ranges", S_IFREG | 0444,
device_root, pctldev, &pinctrl_gpioranges_fops);
if (pctldev->desc->pmxops)
pinmux_init_device_debugfs(device_root, pctldev);
On Wed, Feb 10, 2021 at 11:24:23PM -0800, Joe Perches wrote:
> On Thu, 2021-02-11 at 10:11 +0300, Dan Carpenter wrote:
> > On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> > > + ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> > > + if (ret < 0) {
> > > + dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > > + goto free_gname;
> > > + }
> > > + buf[len-1] = '\0';
> > > +
> > > + ret = sscanf(buf, "%s %s", fname, gname);
> > > + if (ret != 2) {
> > > + dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > > + goto free_gname;
> >
> > We need a "ret = -EINVAL;" before the goto. sscanf doesn't return error
> > codes. Normally we would write it like so:
> >
> > if (sscanf(buf, "%s %s", fname, gname) != 2) {
> > dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > ret = -EINVAL;
> > goto free_gname;
> > }
> >
> > I'm going to write a Smatch check for this today.
>
> It's a pretty frequently used style:
>
> $ git grep -P '\w+\s*=\s+sscanf\b' | wc -l
> 327
Yeah. That's true. I looked through a couple of those and they were
fine. (Sample size 2) But the other format is more common.
$ git grep sscanf | grep = | wc -l
803
I have written a Smatch check to complain whenever we propogate the
return value from sscanf. I'll let you know tomorrow how that goes.
I should write another check which says "On this error path, we know
sscanf was not equal to the value we wanted but we are still returning
success".
regards,
dan carpenter
On Wed, Feb 10, 2021 at 11:36:39PM -0800, Joe Perches wrote:
> On Wed, 2021-02-10 at 14:28 -0800, Drew Fustini wrote:
> > Switch over pinctrl debugfs files to use octal permissions as they are
> > preferred over symbolic permissions. Refer to commit f90774e1fd27
> > ("checkpatch: look for symbolic permissions and suggest octal instead").
> >
> > Note: S_IFREG flag is added to the mode by __debugfs_create_file()
> > in fs/debugfs/inode.c
> >
> > Suggested-by: Andy Shevchenko <[email protected]>
> > Signed-off-by: Drew Fustini <[email protected]>
> > ---
> > ?drivers/pinctrl/core.c | 6 +++---
> > ?drivers/pinctrl/pinconf.c | 4 ++--
> > ?drivers/pinctrl/pinmux.c | 4 ++--
> > ?3 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> > index 3663d87f51a0..02f8710afb9c 100644
> > --- a/drivers/pinctrl/core.c
> > +++ b/drivers/pinctrl/core.c
> > @@ -1914,11 +1914,11 @@ static void pinctrl_init_debugfs(void)
> > ? return;
> > ? }
> > ?
> >
> > - debugfs_create_file("pinctrl-devices", S_IFREG | S_IRUGO,
> > + debugfs_create_file("pinctrl-devices", 0444,
> > ? debugfs_root, NULL, &pinctrl_devices_fops);
> > - debugfs_create_file("pinctrl-maps", S_IFREG | S_IRUGO,
> > + debugfs_create_file("pinctrl-maps", 0444,
> > ? debugfs_root, NULL, &pinctrl_maps_fops);
> > - debugfs_create_file("pinctrl-handles", S_IFREG | S_IRUGO,
> > + debugfs_create_file("pinctrl-handles", 0444,
> > ? debugfs_root, NULL, &pinctrl_fops);
> > ?}
>
> Why aren't you also converting this block in the same file?
>
> @@ -1890,11 +1890,11 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
> dev_name(pctldev->dev));
> return;
> }
> - debugfs_create_file("pins", S_IFREG | S_IRUGO,
> + debugfs_create_file("pins", S_IFREG | 0444,
> device_root, pctldev, &pinctrl_pins_fops);
> - debugfs_create_file("pingroups", S_IFREG | S_IRUGO,
> + debugfs_create_file("pingroups", S_IFREG | 0444,
> device_root, pctldev, &pinctrl_groups_fops);
> - debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
> + debugfs_create_file("gpio-ranges", S_IFREG | 0444,
> device_root, pctldev, &pinctrl_gpioranges_fops);
> if (pctldev->desc->pmxops)
> pinmux_init_device_debugfs(device_root, pctldev);
>
>
>
Thank you, that is a very good point. I should have included those
calls to debugfs_create_file() in the patch as well. I will fix that
in the next revision. It looks like I also need to change how sscanf()
is being handle per the other thread of discussion.
Thanks,
Drew
Hi Drew,
On Wed, Feb 10, 2021 at 11:33 PM Drew Fustini <[email protected]> wrote:
> Add "pinmux-select" to debugfs which will activate a function and group
> when "<function-name group-name>" are written to the file. The write
> operation pinmux_select() handles this by checking that the names map to
> valid selectors and then calling ops->set_mux().
>
> The existing "pinmux-functions" debugfs file lists the pin functions
> registered for the pin controller. For example:
>
> function: pinmux-uart0, groups = [ pinmux-uart0-pins ]
> function: pinmux-mmc0, groups = [ pinmux-mmc0-pins ]
> function: pinmux-mmc1, groups = [ pinmux-mmc1-pins ]
> function: pinmux-i2c0, groups = [ pinmux-i2c0-pins ]
> function: pinmux-i2c1, groups = [ pinmux-i2c1-pins ]
> function: pinmux-spi1, groups = [ pinmux-spi1-pins ]
>
> To activate function pinmux-i2c1 and group pinmux-i2c1-pins:
>
> echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select
>
> Signed-off-by: Drew Fustini <[email protected]>
Thanks for your patch!
> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c
> @@ -673,6 +673,111 @@ void pinmux_show_setting(struct seq_file *s,
> DEFINE_SHOW_ATTRIBUTE(pinmux_functions);
> DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
>
> +#define PINMUX_MAX_NAME 64
> +static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
> + size_t len, loff_t *ppos)
> +{
> + struct seq_file *sfile = file->private_data;
> + struct pinctrl_dev *pctldev = sfile->private;
> + const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
> + const char *const *groups;
> + char *buf, *fname, *gname;
> + unsigned int num_groups;
> + int fsel, gsel, ret;
> +
> + if (len > (PINMUX_MAX_NAME * 2)) {
> + dev_err(pctldev->dev, "write too big for buffer");
> + return -EINVAL;
> + }
> +
> + buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + fname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> + if (!fname) {
> + ret = -ENOMEM;
> + goto free_buf;
> + }
> +
> + gname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> + if (!buf) {
> + ret = -ENOMEM;
> + goto free_fname;
> + }
> +
> + ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
While this guarantees buf is not overflowed...
> + if (ret < 0) {
> + dev_err(pctldev->dev, "failed to copy buffer from userspace");
> + goto free_gname;
> + }
> + buf[len-1] = '\0';
> +
> + ret = sscanf(buf, "%s %s", fname, gname);
... one of the two strings can still be longer than PINMUX_MAX_NAME,
thus overflowing fname or gname.
As buf is already a copy, it may be easier to just find the strings in
buf, write the NUL terminators into buf, and set up fname and gname
to point to the strings inside buf.
> + if (ret != 2) {
> + dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> + goto free_gname;
> + }
> +static const struct file_operations pinmux_select_ops = {
> + .owner = THIS_MODULE,
> + .open = pinmux_select_open,
> + .read = seq_read,
I don't think you need to fill in .read for a write-only file.
> + .write = pinmux_select,
> + .llseek = no_llseek,
> + .release = single_release,
> +};
> +
> void pinmux_init_device_debugfs(struct dentry *devroot,
> struct pinctrl_dev *pctldev)
> {
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Feb 11, 2021 at 10:09 AM Geert Uytterhoeven
<[email protected]> wrote:
> On Wed, Feb 10, 2021 at 11:33 PM Drew Fustini <[email protected]> wrote:
> > +#define PINMUX_MAX_NAME 64
> > + if (len > (PINMUX_MAX_NAME * 2)) {
> > + dev_err(pctldev->dev, "write too big for buffer");
> > + return -EINVAL;
> > + }
> > +
> > + buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
> > + ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
>
> While this guarantees buf is not overflowed...
>
> > + if (ret < 0) {
> > + dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > + goto free_gname;
> > + }
> > + buf[len-1] = '\0';
> > +
> > + ret = sscanf(buf, "%s %s", fname, gname);
>
> ... one of the two strings can still be longer than PINMUX_MAX_NAME,
> thus overflowing fname or gname.
>
> As buf is already a copy, it may be easier to just find the strings in
> buf, write the NUL terminators into buf, and set up fname and gname
> to point to the strings inside buf.
You beat me up to it. I was about to comment the same.
So, indeed, instead of sscanf it's simply better and faster to do just
something like
fname = strstrip(buf) ;
if (*fname == '\0') {
...
return -EINVAL;
}
gname = strchr(fname, ' ');
if (!gname) {
...
return -EINVAL;
}
*gname++ = '\0';
on top of the buf pointer.
> > + if (ret != 2) {
> > + dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > + goto free_gname;
> > + }
--
With Best Regards,
Andy Shevchenko
On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> Add "pinmux-select" to debugfs which will activate a function and group
> when "<function-name group-name>" are written to the file. The write
> operation pinmux_select() handles this by checking that the names map to
> valid selectors and then calling ops->set_mux().
>
> The existing "pinmux-functions" debugfs file lists the pin functions
> registered for the pin controller. For example:
>
> function: pinmux-uart0, groups = [ pinmux-uart0-pins ]
> function: pinmux-mmc0, groups = [ pinmux-mmc0-pins ]
> function: pinmux-mmc1, groups = [ pinmux-mmc1-pins ]
> function: pinmux-i2c0, groups = [ pinmux-i2c0-pins ]
> function: pinmux-i2c1, groups = [ pinmux-i2c1-pins ]
> function: pinmux-spi1, groups = [ pinmux-spi1-pins ]
>
> To activate function pinmux-i2c1 and group pinmux-i2c1-pins:
>
> echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select
>
> Signed-off-by: Drew Fustini <[email protected]>
> ---
> drivers/pinctrl/pinmux.c | 107 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 107 insertions(+)
>
> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> index c651b2db0925..23fa32f0a067 100644
> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c
> @@ -673,6 +673,111 @@ void pinmux_show_setting(struct seq_file *s,
> DEFINE_SHOW_ATTRIBUTE(pinmux_functions);
> DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
>
> +#define PINMUX_MAX_NAME 64
> +static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
> + size_t len, loff_t *ppos)
> +{
> + struct seq_file *sfile = file->private_data;
> + struct pinctrl_dev *pctldev = sfile->private;
> + const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
> + const char *const *groups;
> + char *buf, *fname, *gname;
> + unsigned int num_groups;
> + int fsel, gsel, ret;
> +
> + if (len > (PINMUX_MAX_NAME * 2)) {
> + dev_err(pctldev->dev, "write too big for buffer");
> + return -EINVAL;
> + }
> +
> + buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + fname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> + if (!fname) {
> + ret = -ENOMEM;
> + goto free_buf;
> + }
> +
> + gname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> + if (!buf) {
> + ret = -ENOMEM;
> + goto free_fname;
> + }
> +
> + ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> + if (ret < 0) {
> + dev_err(pctldev->dev, "failed to copy buffer from userspace");
> + goto free_gname;
> + }
> + buf[len-1] = '\0';
> +
> + ret = sscanf(buf, "%s %s", fname, gname);
> + if (ret != 2) {
> + dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> + goto free_gname;
> + }
> +
> + fsel = pinmux_func_name_to_selector(pctldev, fname);
> + if (fsel < 0) {
> + dev_err(pctldev->dev, "invalid function %s in map table\n", fname);
> + ret = -EINVAL;
> + goto free_gname;
> + }
> +
> + ret = pmxops->get_function_groups(pctldev, fsel, &groups, &num_groups);
> + if (ret) {
> + dev_err(pctldev->dev, "no groups for function %d (%s)", fsel, fname);
> + goto free_gname;
> +
> + }
> +
> + ret = match_string(groups, num_groups, gname);
> + if (ret < 0) {
> + dev_err(pctldev->dev, "invalid group %s", gname);
> + goto free_gname;
> + }
> +
> + ret = pinctrl_get_group_selector(pctldev, gname);
> + if (ret < 0) {
> + dev_err(pctldev->dev, "failed to get group selectorL %s", gname);
> + goto free_gname;
> + }
> + gsel = ret;
> +
> + ret = pmxops->set_mux(pctldev, fsel, gsel);
> + if (ret) {
> + dev_err(pctldev->dev, "set_mux() failed: %d", ret);
> + goto free_gname;
> + }
> +
> + return len;
> +
> +free_gname:
> + devm_kfree(pctldev->dev, gname);
> +free_fname:
> + devm_kfree(pctldev->dev, fname);
> +free_buf:
> + devm_kfree(pctldev->dev, buf);
Ugh... I honestly thought Smatch was supposed to print a warning when
you used devm_kfree() on kzalloc()ed memory, but I guess the warning is
only the other way around.
Smatch does complain about it as a leak because it was expecting a
regular free.
drivers/pinctrl/pinmux.c:330 pinmux_func_name_to_selector() warn: potential NULL parameter dereference 'fname'
drivers/pinctrl/pinmux.c:764 pinmux_select() warn: possible memory leak of 'gname'
drivers/pinctrl/pinmux.c:764 pinmux_select() warn: sscanf doesn't return error codes
drivers/pinctrl/pinmux.c:764 pinmux_select() warn: returning success when sscanf failed
And what about the success path? Shouldn't we free these on the success
path as well?
regards,
dan carpenter
On Thu, Feb 11, 2021 at 10:39:38AM +0300, Dan Carpenter wrote:
> On Wed, Feb 10, 2021 at 11:24:23PM -0800, Joe Perches wrote:
> > On Thu, 2021-02-11 at 10:11 +0300, Dan Carpenter wrote:
> > > On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> > > > + ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> > > > + if (ret < 0) {
> > > > + dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > > > + goto free_gname;
> > > > + }
> > > > + buf[len-1] = '\0';
> > > > +
> > > > + ret = sscanf(buf, "%s %s", fname, gname);
> > > > + if (ret != 2) {
> > > > + dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > > > + goto free_gname;
> > >
> > > We need a "ret = -EINVAL;" before the goto. sscanf doesn't return error
> > > codes. Normally we would write it like so:
> > >
> > > if (sscanf(buf, "%s %s", fname, gname) != 2) {
> > > dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > > ret = -EINVAL;
> > > goto free_gname;
> > > }
> > >
> > > I'm going to write a Smatch check for this today.
> >
> > It's a pretty frequently used style:
> >
> > $ git grep -P '\w+\s*=\s+sscanf\b' | wc -l
> > 327
>
> Yeah. That's true. I looked through a couple of those and they were
> fine. (Sample size 2) But the other format is more common.
>
> $ git grep sscanf | grep = | wc -l
> 803
>
> I have written a Smatch check to complain whenever we propogate the
> return value from sscanf. I'll let you know tomorrow how that goes.
>
> I should write another check which says "On this error path, we know
> sscanf was not equal to the value we wanted but we are still returning
> success".
>
> regards,
> dan carpenter
>
Thank you for comments regarding sscanf(). And also thank you for the
LF mentorship session on smatch this morning. It helped me understand
it much better.
Based on further comments, it seems there are better ways for me to pull
the strings out of the write buffer, but I will keep this in mind if I
need to use sscanf() in the future.
-Drew
On Thu, Feb 11, 2021 at 09:09:03AM +0100, Geert Uytterhoeven wrote:
> Hi Drew,
>
> On Wed, Feb 10, 2021 at 11:33 PM Drew Fustini <[email protected]> wrote:
> > Add "pinmux-select" to debugfs which will activate a function and group
> > when "<function-name group-name>" are written to the file. The write
> > operation pinmux_select() handles this by checking that the names map to
> > valid selectors and then calling ops->set_mux().
> >
> > The existing "pinmux-functions" debugfs file lists the pin functions
> > registered for the pin controller. For example:
> >
> > function: pinmux-uart0, groups = [ pinmux-uart0-pins ]
> > function: pinmux-mmc0, groups = [ pinmux-mmc0-pins ]
> > function: pinmux-mmc1, groups = [ pinmux-mmc1-pins ]
> > function: pinmux-i2c0, groups = [ pinmux-i2c0-pins ]
> > function: pinmux-i2c1, groups = [ pinmux-i2c1-pins ]
> > function: pinmux-spi1, groups = [ pinmux-spi1-pins ]
> >
> > To activate function pinmux-i2c1 and group pinmux-i2c1-pins:
> >
> > echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select
> >
> > Signed-off-by: Drew Fustini <[email protected]>
>
> Thanks for your patch!
>
> > --- a/drivers/pinctrl/pinmux.c
> > +++ b/drivers/pinctrl/pinmux.c
> > @@ -673,6 +673,111 @@ void pinmux_show_setting(struct seq_file *s,
> > DEFINE_SHOW_ATTRIBUTE(pinmux_functions);
> > DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
> >
> > +#define PINMUX_MAX_NAME 64
> > +static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
> > + size_t len, loff_t *ppos)
> > +{
> > + struct seq_file *sfile = file->private_data;
> > + struct pinctrl_dev *pctldev = sfile->private;
> > + const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
> > + const char *const *groups;
> > + char *buf, *fname, *gname;
> > + unsigned int num_groups;
> > + int fsel, gsel, ret;
> > +
> > + if (len > (PINMUX_MAX_NAME * 2)) {
> > + dev_err(pctldev->dev, "write too big for buffer");
> > + return -EINVAL;
> > + }
> > +
> > + buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + fname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> > + if (!fname) {
> > + ret = -ENOMEM;
> > + goto free_buf;
> > + }
> > +
> > + gname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> > + if (!buf) {
> > + ret = -ENOMEM;
> > + goto free_fname;
> > + }
> > +
> > + ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
>
> While this guarantees buf is not overflowed...
>
> > + if (ret < 0) {
> > + dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > + goto free_gname;
> > + }
> > + buf[len-1] = '\0';
> > +
> > + ret = sscanf(buf, "%s %s", fname, gname);
>
> ... one of the two strings can still be longer than PINMUX_MAX_NAME,
> thus overflowing fname or gname.
>
> As buf is already a copy, it may be easier to just find the strings in
> buf, write the NUL terminators into buf, and set up fname and gname
> to point to the strings inside buf.
Thank you for pointing this out. I should have considered that one name
could be much larger than the other name. I see Andy suggested
alternative to sscanf() that gets around having to allocate seperate
buffers for each name.
> > + if (ret != 2) {
> > + dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > + goto free_gname;
> > + }
>
> > +static const struct file_operations pinmux_select_ops = {
> > + .owner = THIS_MODULE,
> > + .open = pinmux_select_open,
> > + .read = seq_read,
>
> I don't think you need to fill in .read for a write-only file.
Thanks, I'll remove it.
>
> > + .write = pinmux_select,
> > + .llseek = no_llseek,
> > + .release = single_release,
> > +};
> > +
> > void pinmux_init_device_debugfs(struct dentry *devroot,
> > struct pinctrl_dev *pctldev)
> > {
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
On Thu, Feb 11, 2021 at 11:53:24AM +0200, Andy Shevchenko wrote:
> On Thu, Feb 11, 2021 at 10:09 AM Geert Uytterhoeven
> <[email protected]> wrote:
> > On Wed, Feb 10, 2021 at 11:33 PM Drew Fustini <[email protected]> wrote:
>
>
> > > +#define PINMUX_MAX_NAME 64
>
> > > + if (len > (PINMUX_MAX_NAME * 2)) {
> > > + dev_err(pctldev->dev, "write too big for buffer");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
>
> > > + ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> >
> > While this guarantees buf is not overflowed...
> >
> > > + if (ret < 0) {
> > > + dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > > + goto free_gname;
> > > + }
> > > + buf[len-1] = '\0';
> > > +
> > > + ret = sscanf(buf, "%s %s", fname, gname);
> >
> > ... one of the two strings can still be longer than PINMUX_MAX_NAME,
> > thus overflowing fname or gname.
> >
> > As buf is already a copy, it may be easier to just find the strings in
> > buf, write the NUL terminators into buf, and set up fname and gname
> > to point to the strings inside buf.
>
> You beat me up to it. I was about to comment the same.
>
> So, indeed, instead of sscanf it's simply better and faster to do just
> something like
>
> fname = strstrip(buf) ;
> if (*fname == '\0') {
> ...
> return -EINVAL;
> }
>
> gname = strchr(fname, ' ');
> if (!gname) {
> ...
> return -EINVAL;
> }
> *gname++ = '\0';
>
> on top of the buf pointer.
>
Thank you for the suggestion about how to implement this. I'll use that
in the next revision.
-Drew
On Thu, Feb 11, 2021 at 03:00:51PM +0300, Dan Carpenter wrote:
> On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> > Add "pinmux-select" to debugfs which will activate a function and group
> > when "<function-name group-name>" are written to the file. The write
> > operation pinmux_select() handles this by checking that the names map to
> > valid selectors and then calling ops->set_mux().
> >
> > The existing "pinmux-functions" debugfs file lists the pin functions
> > registered for the pin controller. For example:
> >
> > function: pinmux-uart0, groups = [ pinmux-uart0-pins ]
> > function: pinmux-mmc0, groups = [ pinmux-mmc0-pins ]
> > function: pinmux-mmc1, groups = [ pinmux-mmc1-pins ]
> > function: pinmux-i2c0, groups = [ pinmux-i2c0-pins ]
> > function: pinmux-i2c1, groups = [ pinmux-i2c1-pins ]
> > function: pinmux-spi1, groups = [ pinmux-spi1-pins ]
> >
> > To activate function pinmux-i2c1 and group pinmux-i2c1-pins:
> >
> > echo "pinmux-i2c1 pinmux-i2c1-pins" > pinmux-select
> >
> > Signed-off-by: Drew Fustini <[email protected]>
> > ---
> > drivers/pinctrl/pinmux.c | 107 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 107 insertions(+)
> >
> > diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> > index c651b2db0925..23fa32f0a067 100644
> > --- a/drivers/pinctrl/pinmux.c
> > +++ b/drivers/pinctrl/pinmux.c
> > @@ -673,6 +673,111 @@ void pinmux_show_setting(struct seq_file *s,
> > DEFINE_SHOW_ATTRIBUTE(pinmux_functions);
> > DEFINE_SHOW_ATTRIBUTE(pinmux_pins);
> >
> > +#define PINMUX_MAX_NAME 64
> > +static ssize_t pinmux_select(struct file *file, const char __user *user_buf,
> > + size_t len, loff_t *ppos)
> > +{
> > + struct seq_file *sfile = file->private_data;
> > + struct pinctrl_dev *pctldev = sfile->private;
> > + const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
> > + const char *const *groups;
> > + char *buf, *fname, *gname;
> > + unsigned int num_groups;
> > + int fsel, gsel, ret;
> > +
> > + if (len > (PINMUX_MAX_NAME * 2)) {
> > + dev_err(pctldev->dev, "write too big for buffer");
> > + return -EINVAL;
> > + }
> > +
> > + buf = kzalloc(PINMUX_MAX_NAME * 2, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + fname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> > + if (!fname) {
> > + ret = -ENOMEM;
> > + goto free_buf;
> > + }
> > +
> > + gname = kzalloc(PINMUX_MAX_NAME, GFP_KERNEL);
> > + if (!buf) {
> > + ret = -ENOMEM;
> > + goto free_fname;
> > + }
> > +
> > + ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> > + if (ret < 0) {
> > + dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > + goto free_gname;
> > + }
> > + buf[len-1] = '\0';
> > +
> > + ret = sscanf(buf, "%s %s", fname, gname);
> > + if (ret != 2) {
> > + dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > + goto free_gname;
> > + }
> > +
> > + fsel = pinmux_func_name_to_selector(pctldev, fname);
> > + if (fsel < 0) {
> > + dev_err(pctldev->dev, "invalid function %s in map table\n", fname);
> > + ret = -EINVAL;
> > + goto free_gname;
> > + }
> > +
> > + ret = pmxops->get_function_groups(pctldev, fsel, &groups, &num_groups);
> > + if (ret) {
> > + dev_err(pctldev->dev, "no groups for function %d (%s)", fsel, fname);
> > + goto free_gname;
> > +
> > + }
> > +
> > + ret = match_string(groups, num_groups, gname);
> > + if (ret < 0) {
> > + dev_err(pctldev->dev, "invalid group %s", gname);
> > + goto free_gname;
> > + }
> > +
> > + ret = pinctrl_get_group_selector(pctldev, gname);
> > + if (ret < 0) {
> > + dev_err(pctldev->dev, "failed to get group selectorL %s", gname);
> > + goto free_gname;
> > + }
> > + gsel = ret;
> > +
> > + ret = pmxops->set_mux(pctldev, fsel, gsel);
> > + if (ret) {
> > + dev_err(pctldev->dev, "set_mux() failed: %d", ret);
> > + goto free_gname;
> > + }
> > +
> > + return len;
> > +
> > +free_gname:
> > + devm_kfree(pctldev->dev, gname);
> > +free_fname:
> > + devm_kfree(pctldev->dev, fname);
> > +free_buf:
> > + devm_kfree(pctldev->dev, buf);
>
> Ugh... I honestly thought Smatch was supposed to print a warning when
> you used devm_kfree() on kzalloc()ed memory, but I guess the warning is
> only the other way around.
>
> Smatch does complain about it as a leak because it was expecting a
> regular free.
>
> drivers/pinctrl/pinmux.c:330 pinmux_func_name_to_selector() warn: potential NULL parameter dereference 'fname'
> drivers/pinctrl/pinmux.c:764 pinmux_select() warn: possible memory leak of 'gname'
> drivers/pinctrl/pinmux.c:764 pinmux_select() warn: sscanf doesn't return error codes
> drivers/pinctrl/pinmux.c:764 pinmux_select() warn: returning success when sscanf failed
Thank you pointing this out. I should have switched back to normal free
now that I no longer using a device managed buffer.
>
> And what about the success path? Shouldn't we free these on the success
> path as well?
Good point. That is my fault. I will update it so that it correctly
free's buffer no matter what happens in the function.
thanks,
drew
On Thu, Feb 11, 2021 at 07:35:33PM -0800, Drew Fustini wrote:
> On Thu, Feb 11, 2021 at 10:39:38AM +0300, Dan Carpenter wrote:
> > On Wed, Feb 10, 2021 at 11:24:23PM -0800, Joe Perches wrote:
> > > On Thu, 2021-02-11 at 10:11 +0300, Dan Carpenter wrote:
> > > > On Wed, Feb 10, 2021 at 02:28:54PM -0800, Drew Fustini wrote:
> > > > > + ret = strncpy_from_user(buf, user_buf, PINMUX_MAX_NAME * 2);
> > > > > + if (ret < 0) {
> > > > > + dev_err(pctldev->dev, "failed to copy buffer from userspace");
> > > > > + goto free_gname;
> > > > > + }
> > > > > + buf[len-1] = '\0';
> > > > > +
> > > > > + ret = sscanf(buf, "%s %s", fname, gname);
> > > > > + if (ret != 2) {
> > > > > + dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > > > > + goto free_gname;
> > > >
> > > > We need a "ret = -EINVAL;" before the goto. sscanf doesn't return error
> > > > codes. Normally we would write it like so:
> > > >
> > > > if (sscanf(buf, "%s %s", fname, gname) != 2) {
> > > > dev_err(pctldev->dev, "expected format: <function-name> <group-name>");
> > > > ret = -EINVAL;
> > > > goto free_gname;
> > > > }
> > > >
> > > > I'm going to write a Smatch check for this today.
> > >
> > > It's a pretty frequently used style:
> > >
> > > $ git grep -P '\w+\s*=\s+sscanf\b' | wc -l
> > > 327
> >
> > Yeah. That's true. I looked through a couple of those and they were
> > fine. (Sample size 2) But the other format is more common.
> >
> > $ git grep sscanf | grep = | wc -l
> > 803
> >
> > I have written a Smatch check to complain whenever we propogate the
> > return value from sscanf. I'll let you know tomorrow how that goes.
> >
> > I should write another check which says "On this error path, we know
> > sscanf was not equal to the value we wanted but we are still returning
> > success".
> >
> > regards,
> > dan carpenter
> >
>
> Thank you for comments regarding sscanf(). And also thank you for the
> LF mentorship session on smatch this morning. It helped me understand
> it much better.
Good deal!
The warning about propagating errors from sscanf caught a couple bugs.
The one about returning success if sscanf failed didn't catch anything.
The sscanf overflow patch didn't find anything either, but I think we've
had those bugs in the past and so I expect some in the future so I will
keep that one in my private tests without pushing it.
regards,
dan carpenter