| Message ID | 20210210222851.232374-3-drew@beagleboard.org |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: pinmux: Add pinmux-select debugfs file | expand |
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, 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
Hi Drew, On Wed, Feb 10, 2021 at 11:33 PM Drew Fustini <drew@beagleboard.org> 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 <drew@beagleboard.org> 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
On Thu, Feb 11, 2021 at 10:09 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Feb 10, 2021 at 11:33 PM Drew Fustini <drew@beagleboard.org> 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; > > + }
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 <drew@beagleboard.org> > --- > 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 <drew@beagleboard.org> 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 <drew@beagleboard.org> > > 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 -- geert@linux-m68k.org > > 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 > <geert@linux-m68k.org> wrote: > > On Wed, Feb 10, 2021 at 11:33 PM Drew Fustini <drew@beagleboard.org> 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 <drew@beagleboard.org> > > --- > > 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
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 */
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 <drew@beagleboard.org> --- drivers/pinctrl/pinmux.c | 107 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+)