| Message ID | 1353494856-12344-7-git-send-email-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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);