Patchwork [net-next,06/10] ixgbe: eliminate Smatch warnings in ixgbe_debugfs.c

login
register
mail settings
Submitter Jeff Kirsher
Date Nov. 21, 2012, 10:47 a.m.
Message ID <1353494856-12344-7-git-send-email-jeffrey.t.kirsher@intel.com>
Download mbox | patch
Permalink /patch/200639/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Jeff Kirsher - Nov. 21, 2012, 10:47 a.m.
From: Josh Hay <joshua.a.hay@intel.com>

This patch replaces calls to copy_to_user, copy_from_user, and the associated
logic, with calls to simple_read_from_buffer and simple_write_to_buffer
respectively.  This was done to eliminate warnings generated by the Smatch
static analysis tool.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
CC: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Josh Hay <joshua.a.hay@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c | 83 +++++++++++++-----------
 1 file changed, 46 insertions(+), 37 deletions(-)
Dan Carpenter - Nov. 21, 2012, 11:04 a.m.
On Wed, Nov 21, 2012 at 02:47:32AM -0800, Jeff Kirsher wrote:
> +	len = simple_write_to_buffer(ixgbe_dbg_reg_ops_buf,
> +				     sizeof(ixgbe_dbg_reg_ops_buf)-1,
> +				     ppos,
> +				     buffer,
> +				     count);
> +	if (len < 0)
> +		return -EFAULT;

Any negative return is bad.

	if (len)
		return len;

> +
> +	ixgbe_dbg_reg_ops_buf[len] = '\0';
>  
>  	if (strncmp(ixgbe_dbg_reg_ops_buf, "write", 5) == 0) {
>  		u32 reg, value;
> @@ -187,15 +196,15 @@ static ssize_t ixgbe_dbg_netdev_ops_write(struct file *filp,
>  	if (count >= sizeof(ixgbe_dbg_netdev_ops_buf))
>  		return -ENOSPC;
>  
> -	bytes_not_copied = copy_from_user(ixgbe_dbg_netdev_ops_buf,
> -					  buffer, count);
> -	if (bytes_not_copied < 0)
> -		return bytes_not_copied;
> -	else if (bytes_not_copied < count)
> -		count -= bytes_not_copied;
> -	else
> -		return -ENOSPC;
> -	ixgbe_dbg_netdev_ops_buf[count] = '\0';
> +	len = simple_write_to_buffer(ixgbe_dbg_netdev_ops_buf,
> +				     sizeof(ixgbe_dbg_netdev_ops_buf)-1,
> +				     ppos,
> +				     buffer,
> +				     count);
> +	if (len < 0)
> +		return -EFAULT;

Same.

> +
> +	ixgbe_dbg_netdev_ops_buf[len] = '\0';

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 21, 2012, 5:04 p.m.
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Wed, 21 Nov 2012 14:04:09 +0300

> On Wed, Nov 21, 2012 at 02:47:32AM -0800, Jeff Kirsher wrote:
>> +	len = simple_write_to_buffer(ixgbe_dbg_reg_ops_buf,
>> +				     sizeof(ixgbe_dbg_reg_ops_buf)-1,
>> +				     ppos,
>> +				     buffer,
>> +				     count);
>> +	if (len < 0)
>> +		return -EFAULT;
> 
> Any negative return is bad.
> 
> 	if (len)
> 		return len;
 ...
>> @@ -187,15 +196,15 @@ static ssize_t ixgbe_dbg_netdev_ops_write(struct file *filp,
 ...
>> +	if (len < 0)
>> +		return -EFAULT;
> 
> Same.

Agreed.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hay, Joshua A - Nov. 27, 2012, 1:06 a.m.
The return value will be changed to len to preserve error codes returned from simple_write_to_buffer.

However, changing the logic preceding this return breaks these functions.  If simple_write_to_buffer returns a positive value, other actions are performed with this value.  With this patch, the function will return immediately with that value instead.  This will effectively break the ixgbe_debugfs write operations.

So ultimately, the change should be: 
> +	len = simple_write_to_buffer(ixgbe_dbg_reg_ops_buf,
> +				     sizeof(ixgbe_dbg_reg_ops_buf)-1,
> +				     ppos,
> +				     buffer,
> +				     count);
> +	if (len < 0)
> +		return -EFAULT;
	
	if (len < 0)
		return len;

Thanks,
Josh Hay

-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter@oracle.com] 
Sent: Wednesday, November 21, 2012 3:04 AM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Hay, Joshua A; netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com
Subject: Re: [net-next 06/10] ixgbe: eliminate Smatch warnings in ixgbe_debugfs.c

On Wed, Nov 21, 2012 at 02:47:32AM -0800, Jeff Kirsher wrote:
> +	len = simple_write_to_buffer(ixgbe_dbg_reg_ops_buf,
> +				     sizeof(ixgbe_dbg_reg_ops_buf)-1,
> +				     ppos,
> +				     buffer,
> +				     count);
> +	if (len < 0)
> +		return -EFAULT;

Any negative return is bad.

	if (len)
		return len;

> +
> +	ixgbe_dbg_reg_ops_buf[len] = '\0';
>  
>  	if (strncmp(ixgbe_dbg_reg_ops_buf, "write", 5) == 0) {
>  		u32 reg, value;
> @@ -187,15 +196,15 @@ static ssize_t ixgbe_dbg_netdev_ops_write(struct file *filp,
>  	if (count >= sizeof(ixgbe_dbg_netdev_ops_buf))
>  		return -ENOSPC;
>  
> -	bytes_not_copied = copy_from_user(ixgbe_dbg_netdev_ops_buf,
> -					  buffer, count);
> -	if (bytes_not_copied < 0)
> -		return bytes_not_copied;
> -	else if (bytes_not_copied < count)
> -		count -= bytes_not_copied;
> -	else
> -		return -ENOSPC;
> -	ixgbe_dbg_netdev_ops_buf[count] = '\0';
> +	len = simple_write_to_buffer(ixgbe_dbg_netdev_ops_buf,
> +				     sizeof(ixgbe_dbg_netdev_ops_buf)-1,
> +				     ppos,
> +				     buffer,
> +				     count);
> +	if (len < 0)
> +		return -EFAULT;

Same.

> +
> +	ixgbe_dbg_netdev_ops_buf[len] = '\0';

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter - Nov. 27, 2012, 7:18 a.m.
On Tue, Nov 27, 2012 at 01:06:17AM +0000, Hay, Joshua A wrote:
> The return value will be changed to len to preserve error codes returned from simple_write_to_buffer.
> 
> However, changing the logic preceding this return breaks these functions.  If simple_write_to_buffer returns a positive value, other actions are performed with this value.  With this patch, the function will return immediately with that value instead.  This will effectively break the ixgbe_debugfs write operations.
> 
> So ultimately, the change should be: 
> > +	len = simple_write_to_buffer(ixgbe_dbg_reg_ops_buf,
> > +				     sizeof(ixgbe_dbg_reg_ops_buf)-1,
> > +				     ppos,
> > +				     buffer,
> > +				     count);
> > +	if (len < 0)
> > +		return -EFAULT;
> 	
> 	if (len < 0)
> 		return len;
> 

Yes.  Sorry, I wasn't reading carefully before.  That looks fine.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
index efaf9a7..a717baa 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
@@ -47,23 +47,27 @@  static ssize_t ixgbe_dbg_reg_ops_read(struct file *filp, char __user *buffer,
 				    size_t count, loff_t *ppos)
 {
 	struct ixgbe_adapter *adapter = filp->private_data;
-	char buf[256];
-	int bytes_not_copied;
+	char *buf;
 	int len;
 
 	/* don't allow partial reads */
 	if (*ppos != 0)
 		return 0;
 
-	len = snprintf(buf, sizeof(buf), "%s: %s\n",
-		       adapter->netdev->name, ixgbe_dbg_reg_ops_buf);
-	if (count < len)
+	buf = kasprintf(GFP_KERNEL, "%s: %s\n",
+			adapter->netdev->name,
+			ixgbe_dbg_reg_ops_buf);
+	if (!buf)
+		return -ENOMEM;
+
+	if (count < strlen(buf)) {
+		kfree(buf);
 		return -ENOSPC;
-	bytes_not_copied = copy_to_user(buffer, buf, len);
-	if (bytes_not_copied < 0)
-		return bytes_not_copied;
+	}
+
+	len = simple_read_from_buffer(buffer, count, ppos, buf, strlen(buf));
 
-	*ppos = len;
+	kfree(buf);
 	return len;
 }
 
@@ -79,7 +83,7 @@  static ssize_t ixgbe_dbg_reg_ops_write(struct file *filp,
 				     size_t count, loff_t *ppos)
 {
 	struct ixgbe_adapter *adapter = filp->private_data;
-	int bytes_not_copied;
+	int len;
 
 	/* don't allow partial writes */
 	if (*ppos != 0)
@@ -87,14 +91,15 @@  static ssize_t ixgbe_dbg_reg_ops_write(struct file *filp,
 	if (count >= sizeof(ixgbe_dbg_reg_ops_buf))
 		return -ENOSPC;
 
-	bytes_not_copied = copy_from_user(ixgbe_dbg_reg_ops_buf, buffer, count);
-	if (bytes_not_copied < 0)
-		return bytes_not_copied;
-	else if (bytes_not_copied < count)
-		count -= bytes_not_copied;
-	else
-		return -ENOSPC;
-	ixgbe_dbg_reg_ops_buf[count] = '\0';
+	len = simple_write_to_buffer(ixgbe_dbg_reg_ops_buf,
+				     sizeof(ixgbe_dbg_reg_ops_buf)-1,
+				     ppos,
+				     buffer,
+				     count);
+	if (len < 0)
+		return -EFAULT;
+
+	ixgbe_dbg_reg_ops_buf[len] = '\0';
 
 	if (strncmp(ixgbe_dbg_reg_ops_buf, "write", 5) == 0) {
 		u32 reg, value;
@@ -147,23 +152,27 @@  static ssize_t ixgbe_dbg_netdev_ops_read(struct file *filp,
 					 size_t count, loff_t *ppos)
 {
 	struct ixgbe_adapter *adapter = filp->private_data;
-	char buf[256];
-	int bytes_not_copied;
+	char *buf;
 	int len;
 
 	/* don't allow partial reads */
 	if (*ppos != 0)
 		return 0;
 
-	len = snprintf(buf, sizeof(buf), "%s: %s\n",
-		       adapter->netdev->name, ixgbe_dbg_netdev_ops_buf);
-	if (count < len)
+	buf = kasprintf(GFP_KERNEL, "%s: %s\n",
+			adapter->netdev->name,
+			ixgbe_dbg_netdev_ops_buf);
+	if (!buf)
+		return -ENOMEM;
+
+	if (count < strlen(buf)) {
+		kfree(buf);
 		return -ENOSPC;
-	bytes_not_copied = copy_to_user(buffer, buf, len);
-	if (bytes_not_copied < 0)
-		return bytes_not_copied;
+	}
+
+	len = simple_read_from_buffer(buffer, count, ppos, buf, strlen(buf));
 
-	*ppos = len;
+	kfree(buf);
 	return len;
 }
 
@@ -179,7 +188,7 @@  static ssize_t ixgbe_dbg_netdev_ops_write(struct file *filp,
 					  size_t count, loff_t *ppos)
 {
 	struct ixgbe_adapter *adapter = filp->private_data;
-	int bytes_not_copied;
+	int len;
 
 	/* don't allow partial writes */
 	if (*ppos != 0)
@@ -187,15 +196,15 @@  static ssize_t ixgbe_dbg_netdev_ops_write(struct file *filp,
 	if (count >= sizeof(ixgbe_dbg_netdev_ops_buf))
 		return -ENOSPC;
 
-	bytes_not_copied = copy_from_user(ixgbe_dbg_netdev_ops_buf,
-					  buffer, count);
-	if (bytes_not_copied < 0)
-		return bytes_not_copied;
-	else if (bytes_not_copied < count)
-		count -= bytes_not_copied;
-	else
-		return -ENOSPC;
-	ixgbe_dbg_netdev_ops_buf[count] = '\0';
+	len = simple_write_to_buffer(ixgbe_dbg_netdev_ops_buf,
+				     sizeof(ixgbe_dbg_netdev_ops_buf)-1,
+				     ppos,
+				     buffer,
+				     count);
+	if (len < 0)
+		return -EFAULT;
+
+	ixgbe_dbg_netdev_ops_buf[len] = '\0';
 
 	if (strncmp(ixgbe_dbg_netdev_ops_buf, "tx_timeout", 10) == 0) {
 		adapter->netdev->netdev_ops->ndo_tx_timeout(adapter->netdev);