Re: [PATCH 2/4] dma-debug: add debugfs file for driver filter

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 28 May 2009 17:19:29 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> This patch adds the dma-api/driver_filter file to debugfs. The root user
> can write a driver name into this file to see only dma-api errors for
> that particular driver in the kernel log. Writing an empty string to
> that file disables the driver filter.
> 
>
> ...
>
> +static ssize_t filter_read(struct file *file, char __user *user_buf,
> +			   size_t count, loff_t *ppos)
> +{
> +	unsigned long flags;
> +	char buf[NAME_MAX_LEN + 1];
> +	int len;
> +
> +	if (!current_driver_name[0])
> +		return 0;
> +
> +	/*
> +	 * We can't copy to userspace directly because current_driver_name can
> +	 * only be read under the driver_name_lock with irqs disabled. So
> +	 * create a temporary copy first.
> +	 */
> +	read_lock_irqsave(&driver_name_lock, flags);
> +	strncpy(buf, current_driver_name, NAME_MAX_LEN - 1);

If you use strlcpy() here,

> +	read_unlock_irqrestore(&driver_name_lock, flags);
> +
> +	/* make it look nice :-) */
> +	len = strnlen(buf, NAME_MAX_LEN - 1);
> +	buf[len]     = '\n';
> +	buf[len + 1] = 0;

the above can all be replaced with a simple strcat().  I think.

Perhaps the whole lot can be replaced with a single scnprintf(), if
it's known that the input string has appropriate length.

> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len + 1);
> +}
> +
> +static ssize_t filter_write(struct file *file, const char __user *userbuf,
> +			    size_t count, loff_t *ppos)
> +{
> +	unsigned long flags;
> +	char buf[NAME_MAX_LEN];
> +	size_t len = NAME_MAX_LEN - 1;
> +	int i;
> +
> +	/*
> +	 * We can't copy from userspace directly. Access to
> +	 * current_driver_name is protected with a write_lock with irqs
> +	 * disabled. Since copy_from_user can fault and may sleep we
> +	 * need to copy to temporary buffer first
> +	 */
> +	len = min(count, len);
> +	if (copy_from_user(buf, userbuf, len))
> +		return -EFAULT;
> +
> +	buf[NAME_MAX_LEN - 1] = 0;

Might be able to use strncpy_from_user() here.

> +	write_lock_irqsave(&driver_name_lock, flags);
> +	if (!isalnum(buf[0])) {
> +		if (current_driver_name[0])
> +			printk(KERN_INFO "DMA-API: switching off dma-debug "
> +					 "driver filter\n");
> +		current_driver_name[0] = 0;
> +		current_driver = NULL;
> +		goto out_unlock;
> +	}
> +
> +	for (i = 0; i < NAME_MAX_LEN; ++i) {
> +		current_driver_name[i] = buf[i];
> +		if (isspace(buf[i]) || buf[i] == ' ' ||
> +		    buf[i] == '\t' || buf[i] == 0)
> +			break;
> +	}
> +	current_driver_name[i] = 0;
> +	current_driver = NULL;
> +
> +	printk(KERN_INFO "DMA-API: enable driver filter for driver [%s]\n",
> +	       current_driver_name);
> +
> +out_unlock:
> +	write_unlock_irqrestore(&driver_name_lock, flags);
> +
> +	return count;
> +}

It's unobvious what user interface this code is attempting to implement.
The change to DMA-API.txt in the fourth patch doesn't illuminate the
issue either. Some code comments explaining wtf this is doing and why
would help, please.

I'd have thought that the test for isspace() makes the tests for ' '
and '\t' redundant?


> +const struct file_operations filter_fops = {
> +	.read  = filter_read,
> +	.write = filter_write,
> +};
> +
>  static int dma_debug_fs_init(void)
>  {
>  	dma_debug_dent = debugfs_create_dir("dma-api", NULL);
> @@ -497,6 +581,11 @@ static int dma_debug_fs_init(void)
>  	if (!min_free_entries_dent)
>  		goto out_err;
>  
> +	filter_dent = debugfs_create_file("driver_filter", 0644,
> +					  dma_debug_dent, NULL, &filter_fops);
> +	if (!filter_dent)
> +		goto out_err;
> +
>  	return 0;
>  
>  out_err: