diff mbox series

[3/4] PCI/sysfs: Return -EINVAL consistently from "store" functions

Message ID 20210705212308.3050976-3-kw@linux.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series [1/4] PCI/sysfs: Move to kstrtobool() to handle user input | expand

Commit Message

Krzysztof Wilczyński July 5, 2021, 9:23 p.m. UTC 
At the moment, most of the "store" functions that handle input from the
userspace via the sysfs objects commonly returns the -EINVAL error code
should the value provided fail validation and/or type conversion.  This
error code is a clear message to the userspace that the provided value
is not a valid input.

However, some of the "show" functions return the error code as-is as
returned from the helper functions used to parse the input value, and in
which case the error code can be either -EINVAL or -ERANGE.  The former
would often be a return from the kstrtobool() function and the latter
will most likely be originating from other kstr*() functions family such
as e.g., kstrtou8(), kstrtou32(), etc.

The -EINVAL is commonly returned as the error code to indicate that the
value provided is invalid, and the -ERANGE code is not very useful in
the userspace, thus normalize the return error code to be -EINVAL for
when the validation and/or type conversion fails.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/pci/endpoint/functions/pci-epf-ntb.c | 18 ++++------
 drivers/pci/endpoint/pci-ep-cfs.c            | 35 +++++++-------------
 drivers/pci/iov.c                            | 10 +++---
 3 files changed, 22 insertions(+), 41 deletions(-)

Comments

kernel test robot July 6, 2021, 12:24 a.m. UTC | #1 
Hi "Krzysztof,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pci/next]
[also build test WARNING on v5.13 next-20210701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Krzysztof-Wilczy-ski/PCI-sysfs-Move-to-kstrtobool-to-handle-user-input/20210706-052334
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-a011-20210705 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 3f9bf9f42a9043e20c6d2a74dd4f47a90a7e2b41)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/f58c20f2790bdbbb20cc43b70d1517454d9ef86c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Krzysztof-Wilczy-ski/PCI-sysfs-Move-to-kstrtobool-to-handle-user-input/20210706-052334
        git checkout f58c20f2790bdbbb20cc43b70d1517454d9ef86c
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/pci/iov.c:389:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
           if (num_vfs == pdev->sriov->num_VFs)
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/iov.c:431:6: note: uninitialized use occurs here
           if (ret < 0)
               ^~~
   drivers/pci/iov.c:389:2: note: remove the 'if' if its condition is always false
           if (num_vfs == pdev->sriov->num_VFs)
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/pci/iov.c:378:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.


vim +389 drivers/pci/iov.c

aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  365  
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  366  /*
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  367   * num_vfs > 0; number of VFs to enable
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  368   * num_vfs = 0; disable all VFs
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  369   *
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  370   * Note: SRIOV spec does not allow partial VF
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  371   *	 disable, so it's all or none.
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  372   */
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  373  static ssize_t sriov_numvfs_store(struct device *dev,
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  374  				  struct device_attribute *attr,
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  375  				  const char *buf, size_t count)
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  376  {
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  377  	struct pci_dev *pdev = to_pci_dev(dev);
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  378  	int ret;
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  379  	u16 num_vfs;
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  380  
f58c20f2790bdb Krzysztof Wilczyński 2021-07-05  381  	if (kstrtou16(buf, 0, &num_vfs) < 0)
f58c20f2790bdb Krzysztof Wilczyński 2021-07-05  382  		return -EINVAL;
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  383  
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  384  	if (num_vfs > pci_sriov_get_totalvfs(pdev))
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  385  		return -ERANGE;
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  386  
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  387  	device_lock(&pdev->dev);
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  388  
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13 @389  	if (num_vfs == pdev->sriov->num_VFs)
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  390  		goto exit;
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  391  
e9c3bbd68ec7dc Moritz Fischer       2021-03-27  392  	/* is PF driver loaded */
e9c3bbd68ec7dc Moritz Fischer       2021-03-27  393  	if (!pdev->driver) {
e9c3bbd68ec7dc Moritz Fischer       2021-03-27  394  		pci_info(pdev, "no driver bound to device; cannot configure SR-IOV\n");
e9c3bbd68ec7dc Moritz Fischer       2021-03-27  395  		ret = -ENOENT;
e9c3bbd68ec7dc Moritz Fischer       2021-03-27  396  		goto exit;
e9c3bbd68ec7dc Moritz Fischer       2021-03-27  397  	}
e9c3bbd68ec7dc Moritz Fischer       2021-03-27  398  
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  399  	/* is PF driver loaded w/callback */
e9c3bbd68ec7dc Moritz Fischer       2021-03-27  400  	if (!pdev->driver->sriov_configure) {
e9c3bbd68ec7dc Moritz Fischer       2021-03-27  401  		pci_info(pdev, "driver does not support SR-IOV configuration via sysfs\n");
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  402  		ret = -ENOENT;
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  403  		goto exit;
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  404  	}
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  405  
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  406  	if (num_vfs == 0) {
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  407  		/* disable VFs */
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  408  		ret = pdev->driver->sriov_configure(pdev, 0);
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  409  		goto exit;
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  410  	}
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  411  
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  412  	/* enable VFs */
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  413  	if (pdev->sriov->num_VFs) {
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  414  		pci_warn(pdev, "%d VFs already enabled. Disable before enabling %d VFs\n",
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  415  			 pdev->sriov->num_VFs, num_vfs);
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  416  		ret = -EBUSY;
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  417  		goto exit;
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  418  	}
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  419  
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  420  	ret = pdev->driver->sriov_configure(pdev, num_vfs);
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  421  	if (ret < 0)
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  422  		goto exit;
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  423  
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  424  	if (ret != num_vfs)
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  425  		pci_warn(pdev, "%d VFs requested; only %d enabled\n",
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  426  			 num_vfs, ret);
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  427  
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  428  exit:
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  429  	device_unlock(&pdev->dev);
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  430  
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  431  	if (ret < 0)
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  432  		return ret;
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  433  
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  434  	return count;
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  435  }
aaee0c1ffd6399 Kelsey Skunberg      2019-08-13  436  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Krzysztof Wilczyński July 6, 2021, 12:35 a.m. UTC | #2 
Hi Robot,

[...]
>    drivers/pci/iov.c:378:9: note: initialize the variable 'ret' to silence this warning
>            int ret;
>                   ^
>                    = 0
>    1 warning generated.

Ah oh.  Good point!  I missed this one.

Thank you!

	Krzysztof
diff mbox series

Patch

diff --git a/drivers/pci/endpoint/functions/pci-epf-ntb.c b/drivers/pci/endpoint/functions/pci-epf-ntb.c
index bce274d02dcf..c5a9cfa4c4a4 100644
--- a/drivers/pci/endpoint/functions/pci-epf-ntb.c
+++ b/drivers/pci/endpoint/functions/pci-epf-ntb.c
@@ -1928,11 +1928,9 @@  static ssize_t epf_ntb_##_name##_store(struct config_item *item,	\
 	struct config_group *group = to_config_group(item);		\
 	struct epf_ntb *ntb = to_epf_ntb(group);			\
 	u32 val;							\
-	int ret;							\
 									\
-	ret = kstrtou32(page, 0, &val);					\
-	if (ret)							\
-		return ret;						\
+	if (kstrtou32(page, 0, &val) < 0)				\
+		return -EINVAL;						\
 									\
 	ntb->_name = val;						\
 									\
@@ -1961,11 +1959,9 @@  static ssize_t epf_ntb_##_name##_store(struct config_item *item,	\
 	struct device *dev = &ntb->epf->dev;				\
 	int win_no;							\
 	u64 val;							\
-	int ret;							\
 									\
-	ret = kstrtou64(page, 0, &val);					\
-	if (ret)							\
-		return ret;						\
+	if (kstrtou64(page, 0, &val) < 0)				\
+		return -EINVAL;						\
 									\
 	if (sscanf(#_name, "mw%d", &win_no) != 1)			\
 		return -EINVAL;						\
@@ -1986,11 +1982,9 @@  static ssize_t epf_ntb_num_mws_store(struct config_item *item,
 	struct config_group *group = to_config_group(item);
 	struct epf_ntb *ntb = to_epf_ntb(group);
 	u32 val;
-	int ret;
 
-	ret = kstrtou32(page, 0, &val);
-	if (ret)
-		return ret;
+	if (kstrtou32(page, 0, &val) < 0)
+		return -EINVAL;
 
 	if (val > MAX_MW)
 		return -EINVAL;
diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c
index f3a8b833b479..c77459048ef7 100644
--- a/drivers/pci/endpoint/pci-ep-cfs.c
+++ b/drivers/pci/endpoint/pci-ep-cfs.c
@@ -175,9 +175,8 @@  static ssize_t pci_epc_start_store(struct config_item *item, const char *page,
 
 	epc = epc_group->epc;
 
-	ret = kstrtobool(page, &start);
-	if (ret)
-		return ret;
+	if (kstrtobool(page, &start) < 0)
+		return -EINVAL;
 
 	if (!start) {
 		pci_epc_stop(epc);
@@ -329,13 +328,11 @@  static ssize_t pci_epf_##_name##_store(struct config_item *item,	       \
 				       const char *page, size_t len)	       \
 {									       \
 	u32 val;							       \
-	int ret;							       \
 	struct pci_epf *epf = to_pci_epf_group(item)->epf;		       \
 	if (WARN_ON_ONCE(!epf->header))					       \
 		return -EINVAL;						       \
-	ret = kstrtou32(page, 0, &val);					       \
-	if (ret)							       \
-		return ret;						       \
+	if (kstrtou32(page, 0, &val) < 0)				       \
+		return -EINVAL;						       \
 	epf->header->_name = val;					       \
 	return len;							       \
 }
@@ -345,13 +342,11 @@  static ssize_t pci_epf_##_name##_store(struct config_item *item,	       \
 				       const char *page, size_t len)	       \
 {									       \
 	u16 val;							       \
-	int ret;							       \
 	struct pci_epf *epf = to_pci_epf_group(item)->epf;		       \
 	if (WARN_ON_ONCE(!epf->header))					       \
 		return -EINVAL;						       \
-	ret = kstrtou16(page, 0, &val);					       \
-	if (ret)							       \
-		return ret;						       \
+	if (kstrtou16(page, 0, &val) < 0)				       \
+		return -EINVAL;						       \
 	epf->header->_name = val;					       \
 	return len;							       \
 }
@@ -361,13 +356,11 @@  static ssize_t pci_epf_##_name##_store(struct config_item *item,	       \
 				       const char *page, size_t len)	       \
 {									       \
 	u8 val;								       \
-	int ret;							       \
 	struct pci_epf *epf = to_pci_epf_group(item)->epf;		       \
 	if (WARN_ON_ONCE(!epf->header))					       \
 		return -EINVAL;						       \
-	ret = kstrtou8(page, 0, &val);					       \
-	if (ret)							       \
-		return ret;						       \
+	if (kstrtou8(page, 0, &val) < 0)				       \
+		return -EINVAL;						       \
 	epf->header->_name = val;					       \
 	return len;							       \
 }
@@ -376,11 +369,9 @@  static ssize_t pci_epf_msi_interrupts_store(struct config_item *item,
 					    const char *page, size_t len)
 {
 	u8 val;
-	int ret;
 
-	ret = kstrtou8(page, 0, &val);
-	if (ret)
-		return ret;
+	if (kstrtou8(page, 0, &val) < 0)
+		return -EINVAL;
 
 	to_pci_epf_group(item)->epf->msi_interrupts = val;
 
@@ -398,11 +389,9 @@  static ssize_t pci_epf_msix_interrupts_store(struct config_item *item,
 					     const char *page, size_t len)
 {
 	u16 val;
-	int ret;
 
-	ret = kstrtou16(page, 0, &val);
-	if (ret)
-		return ret;
+	if (kstrtou16(page, 0, &val) < 0)
+		return -EINVAL;
 
 	to_pci_epf_group(item)->epf->msix_interrupts = val;
 
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index afc06e6ce115..aa775c7b46fd 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -185,9 +185,8 @@  static ssize_t sriov_vf_msix_count_store(struct device *dev,
 	struct pci_dev *pdev = pci_physfn(vf_dev);
 	int val, ret;
 
-	ret = kstrtoint(buf, 0, &val);
-	if (ret)
-		return ret;
+	if (kstrtoint(buf, 0, &val) < 0)
+		return -EINVAL;
 
 	if (val < 0)
 		return -EINVAL;
@@ -379,9 +378,8 @@  static ssize_t sriov_numvfs_store(struct device *dev,
 	int ret;
 	u16 num_vfs;
 
-	ret = kstrtou16(buf, 0, &num_vfs);
-	if (ret < 0)
-		return ret;
+	if (kstrtou16(buf, 0, &num_vfs) < 0)
+		return -EINVAL;
 
 	if (num_vfs > pci_sriov_get_totalvfs(pdev))
 		return -ERANGE;