Re: [PATCH v6 3/6] PCI/sysfs: Fix trailing newline handling of resource_alignment_param

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Jun 03, 2021 at 12:01:09AM +0000, Krzysztof Wilczyński wrote:
> The value of the "resource_alignment" can be specified using a kernel
> command-line argument (using the "pci=resource_alignment=") or through
> the corresponding sysfs object under the /sys/bus/pci path.
> 
> Currently, when the value is set via the kernel command-line argument,
> and then subsequently accessed through sysfs object, the value read back
> will not be correct, as per:
> 
>   # grep -oE 'pci=resource_alignment.+' /proc/cmdline
>   pci=resource_alignment=20@00:1f.2
>   # cat /sys/bus/pci/resource_alignment
>   20@00:1f.
> 
> This is also true when the value is set through the sysfs object, but
> the trailing newline has not been included, as per:
> 
>   # echo -n 20@00:1f.2 > /sys/bus/pci/resource_alignment
>   # cat /sys/bus/pci/resource_alignment
>   20@00:1f.
> 
> When the value set through the sysfs object includes the trailing
> newline, then reading it back will work as intended, as per:
> 
>   # echo 20@00:1f.2 > /sys/bus/pci/resource_alignment
>   # cat /sys/bus/pci/resource_alignment
>   20@00:1f.2
> 
> To fix this inconsistency, append a trailing newline in the show()
> function and strip the trailing line in the store() function if one is
> present.
> 
> Also, allow for the value previously set using either a command-line
> argument or through the sysfs object to be cleared at run-time.
> 
> Fixes: e499081da1a2 ("PCI: Force trailing new line to resource_alignment_param in sysfs")
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  drivers/pci/pci.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 5ed316ea5831..b46445a49543 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6439,34 +6439,40 @@ static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
>  
>  	spin_lock(&resource_alignment_lock);
>  	if (resource_alignment_param)
> -		count = sysfs_emit(buf, "%s", resource_alignment_param);
> +		count = sysfs_emit(buf, "%s\n", resource_alignment_param);
>  	spin_unlock(&resource_alignment_lock);
>  
> -	/*
> -	 * When set by the command line, resource_alignment_param will not
> -	 * have a trailing line feed, which is ugly. So conditionally add
> -	 * it here.
> -	 */
> -	if (count >= 2 && buf[count - 2] != '\n' && count < PAGE_SIZE - 1) {
> -		buf[count - 1] = '\n';
> -		buf[count++] = 0;
> -	}
> -
>  	return count;
>  }
>  
>  static ssize_t resource_alignment_store(struct bus_type *bus,
>  					const char *buf, size_t count)
>  {
> -	char *param = kstrndup(buf, count, GFP_KERNEL);
> +	char *param, *old, *end;
> +
> +	if (count >= (PAGE_SIZE - 1))
> +		return -EINVAL;
>  
> +	param = kstrndup(buf, count, GFP_KERNEL);
>  	if (!param)
>  		return -ENOMEM;
>  
> +	end = strchr(param, '\n');
> +	if (end)
> +		*end = '\0';
> +
>  	spin_lock(&resource_alignment_lock);
> -	kfree(resource_alignment_param);
> -	resource_alignment_param = param;
> +	old = resource_alignment_param;
> +	if (strlen(param)) {
> +		resource_alignment_param = param;
> +	} else {
> +		kfree(resource_alignment_param);

When "strlen(param) == 0", don't we kfree resource_alignment_param
twice?  Once here,

> +		resource_alignment_param = NULL;
> +	}
>  	spin_unlock(&resource_alignment_lock);
> +
> +	kfree(old);

and again here?

>  	return count;
>  }
>  
> -- 
> 2.31.1
>