On Fri, Sep 01, 2023 at 04:42:48PM +0300, Andy Shevchenko wrote:
> Improve readability and maintainability by replacing a hardcoded string
> allocation and formatting by the use of the kasprintf() helper.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/parport/procfs.c | 53 +++++++---------------------------------
> drivers/parport/share.c | 15 +++++-------
> include/linux/parport.h | 2 --
> 3 files changed, 15 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
> index 4e5b972c3e26..7aa99c65b934 100644
> --- a/drivers/parport/procfs.c
> +++ b/drivers/parport/procfs.c
> @@ -32,13 +32,6 @@
> #define PARPORT_MAX_TIMESLICE_VALUE ((unsigned long) HZ)
> #define PARPORT_MIN_SPINTIME_VALUE 1
> #define PARPORT_MAX_SPINTIME_VALUE 1000
> -/*
> - * PARPORT_BASE_* is the size of the known parts of the sysctl path
> - * in dev/partport/%s/devices/%s. "dev/parport/"(12), "/devices/"(9
> - * and null char(1).
> - */
> -#define PARPORT_BASE_PATH_SIZE 13
> -#define PARPORT_BASE_DEVICES_PATH_SIZE 22
>
> static int do_active_device(struct ctl_table *table, int write,
> void *result, size_t *lenp, loff_t *ppos)
> @@ -431,8 +424,7 @@ int parport_proc_register(struct parport *port)
> {
> struct parport_sysctl_table *t;
> char *tmp_dir_path;
> - size_t tmp_path_len, port_name_len;
> - int bytes_written, i, err = 0;
> + int i, err = 0;
>
> t = kmemdup(&parport_sysctl_template, sizeof(*t), GFP_KERNEL);
> if (t == NULL)
> @@ -446,35 +438,23 @@ int parport_proc_register(struct parport *port)
For this function I would even go a step further and start with the two
kasprintf calls so we can then free them in the reverse order. And then
leave the rest as it is. I attached an untested diff that applies on
top of your changes to show you what I mean.
> t->vars[5 + i].extra2 = &port->probe_info[i];
> }
>
> - port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> - /*
> - * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
> - * We calculate for the second as that will give us enough for the first.
> - */
> - tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
> - tmp_dir_path = kzalloc(tmp_path_len, GFP_KERNEL);
> + tmp_dir_path = kasprintf(GFP_KERNEL, "dev/parport/%s/devices", port->name);
> if (!tmp_dir_path) {
> err = -ENOMEM;
> goto exit_free_t;
> }
>
> - bytes_written = snprintf(tmp_dir_path, tmp_path_len,
> - "dev/parport/%s/devices", port->name);
> - if (tmp_path_len <= bytes_written) {
> - err = -ENOENT;
> - goto exit_free_tmp_dir_path;
> - }
> t->devices_header = register_sysctl(tmp_dir_path, t->device_dir);
> if (t->devices_header == NULL) {
> err = -ENOENT;
> goto exit_free_tmp_dir_path;
> }
>
> - tmp_path_len = PARPORT_BASE_PATH_SIZE + port_name_len;
> - bytes_written = snprintf(tmp_dir_path, tmp_path_len,
> - "dev/parport/%s", port->name);
> - if (tmp_path_len <= bytes_written) {
> - err = -ENOENT;
> + kfree(tmp_dir_path);
> +
> + tmp_dir_path = kasprintf(GFP_KERNEL, "dev/parport/%s", port->name);
> + if (!tmp_dir_path) {
> + err = -ENOMEM;
> goto unregister_devices_h;
> }
>
> @@ -514,34 +494,22 @@ int parport_proc_unregister(struct parport *port)
>
> int parport_device_proc_register(struct pardevice *device)
> {
> - int bytes_written, err = 0;
> struct parport_device_sysctl_table *t;
> struct parport * port = device->port;
> - size_t port_name_len, device_name_len, tmp_dir_path_len;
> char *tmp_dir_path;
...
> diff --git a/include/linux/parport.h b/include/linux/parport.h
> index 999eddd619b7..fff39bc30629 100644
> --- a/include/linux/parport.h
> +++ b/include/linux/parport.h
> @@ -180,8 +180,6 @@ struct ieee1284_info {
> struct semaphore irq;
> };
>
> -#define PARPORT_NAME_MAX_LEN 15
This variable protected against port->name not ending in '\0'. Anyone
worried that kasprintf could be unbounded?
> -
> /* A parallel port */
> struct parport {
> unsigned long base; /* base address */
> --
> 2.40.0.1.gaa8946217a0b
>
--
Joel Granados