* [PATCH 1/4] PCI/sysfs: Move to kstrtobool() to handle user input
@ 2021-07-05 21:23 Krzysztof Wilczyński
2021-07-05 21:23 ` [PATCH 2/4] PCI/sysfs: Check CAP_SYS_ADMIN before parsing " Krzysztof Wilczyński
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Krzysztof Wilczyński @ 2021-07-05 21:23 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Lorenzo Pieralisi, Kishon Vijay Abraham I, linux-pci
A common use case for many PCI sysfs objects is to either enable some
functionality or trigger an action following a write to a given
attribute where the value is written would be simply either "1" or "0"
synonymous with either disabling or enabling something.
Parsing and validation of the input values are currently done using the
kstrtoul() function to convert anything in the string buffer into an
integer value - anything non-zero would be accepted as "enable" and zero
simply as "disable".
For a while now, the kernel offers another function called kstrtobool()
which was created to parse common user inputs into a boolean value, so
that a range of values such as "y", "n", "1", "0", "on" and "off"
handled in a case-insensitive manner would yield a boolean true or false
result accordingly after the input string has been parsed.
Thus, move to kstrtobool() over kstrtoul() as it's a better fit for
parsing user input, and it also implicitly offers a range check as only
a finite amount of possible input values will be considered as valid.
Related:
commit d0f1fed29e6e ("Add a strtobool function matching semantics of existing in kernel equivalents")
commit ef951599074b ("lib: move strtobool() to kstrtobool()")
commit a81a5a17d44b ("lib: add "on"/"off" support to kstrtobool")
commit 1404297ebf76 ("lib: update single-char callers of strtobool()")
Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
drivers/pci/pci-sysfs.c | 97 ++++++++++++++++++++---------------------
1 file changed, 47 insertions(+), 50 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index beb8d1f4fafe..0f98c4843764 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -63,13 +63,13 @@ static ssize_t broken_parity_status_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
+ bool enable;
struct pci_dev *pdev = to_pci_dev(dev);
- unsigned long val;
- if (kstrtoul(buf, 0, &val) < 0)
+ if (kstrtobool(buf, &enable) < 0)
return -EINVAL;
- pdev->broken_parity_status = !!val;
+ pdev->broken_parity_status = enable;
return count;
}
@@ -271,12 +271,12 @@ static DEVICE_ATTR_RO(modalias);
static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
+ bool enable;
struct pci_dev *pdev = to_pci_dev(dev);
- unsigned long val;
- ssize_t result = kstrtoul(buf, 0, &val);
+ ssize_t result = 0;
- if (result < 0)
- return result;
+ if (kstrtobool(buf, &enable) < 0)
+ return -EINVAL;
/* this can crash the machine when done on the "wrong" device */
if (!capable(CAP_SYS_ADMIN))
@@ -285,7 +285,7 @@ static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
device_lock(dev);
if (dev->driver)
result = -EBUSY;
- else if (val)
+ else if (enable)
result = pci_enable_device(pdev);
else if (pci_is_enabled(pdev))
pci_disable_device(pdev);
@@ -311,15 +311,14 @@ static ssize_t numa_node_store(struct device *dev,
struct device_attribute *attr, const char *buf,
size_t count)
{
+ int node;
struct pci_dev *pdev = to_pci_dev(dev);
- int node, ret;
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
- ret = kstrtoint(buf, 0, &node);
- if (ret)
- return ret;
+ if (kstrtoint(buf, 0, &node) < 0)
+ return -EINVAL;
if ((node < 0 && node != NUMA_NO_NODE) || node >= MAX_NUMNODES)
return -EINVAL;
@@ -374,11 +373,11 @@ static ssize_t msi_bus_show(struct device *dev, struct device_attribute *attr,
static ssize_t msi_bus_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
+ bool enable;
struct pci_dev *pdev = to_pci_dev(dev);
struct pci_bus *subordinate = pdev->subordinate;
- unsigned long val;
- if (kstrtoul(buf, 0, &val) < 0)
+ if (kstrtobool(buf, &enable) < 0)
return -EINVAL;
if (!capable(CAP_SYS_ADMIN))
@@ -390,32 +389,32 @@ static ssize_t msi_bus_store(struct device *dev, struct device_attribute *attr,
* already requested MSI or MSI-X.
*/
if (!subordinate) {
- pdev->no_msi = !val;
+ pdev->no_msi = !enable;
pci_info(pdev, "MSI/MSI-X %s for future drivers\n",
- val ? "allowed" : "disallowed");
+ enable ? "allowed" : "disallowed");
return count;
}
- if (val)
+ if (enable)
subordinate->bus_flags &= ~PCI_BUS_FLAGS_NO_MSI;
else
subordinate->bus_flags |= PCI_BUS_FLAGS_NO_MSI;
dev_info(&subordinate->dev, "MSI/MSI-X %s for future drivers of devices on this bus\n",
- val ? "allowed" : "disallowed");
+ enable ? "allowed" : "disallowed");
return count;
}
static DEVICE_ATTR_RW(msi_bus);
static ssize_t rescan_store(struct bus_type *bus, const char *buf, size_t count)
{
- unsigned long val;
+ bool rescan;
struct pci_bus *b = NULL;
- if (kstrtoul(buf, 0, &val) < 0)
+ if (kstrtobool(buf, &rescan) < 0)
return -EINVAL;
- if (val) {
+ if (rescan) {
pci_lock_rescan_remove();
while ((b = pci_find_next_bus(b)) != NULL)
pci_rescan_bus(b);
@@ -443,13 +442,13 @@ static ssize_t dev_rescan_store(struct device *dev,
struct device_attribute *attr, const char *buf,
size_t count)
{
- unsigned long val;
+ bool rescan;
struct pci_dev *pdev = to_pci_dev(dev);
- if (kstrtoul(buf, 0, &val) < 0)
+ if (kstrtobool(buf, &rescan) < 0)
return -EINVAL;
- if (val) {
+ if (rescan) {
pci_lock_rescan_remove();
pci_rescan_bus(pdev->bus);
pci_unlock_rescan_remove();
@@ -462,12 +461,12 @@ static struct device_attribute dev_attr_dev_rescan = __ATTR(rescan, 0200, NULL,
static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
- unsigned long val;
+ bool remove;
- if (kstrtoul(buf, 0, &val) < 0)
+ if (kstrtobool(buf, &remove) < 0)
return -EINVAL;
- if (val && device_remove_file_self(dev, attr))
+ if (remove && device_remove_file_self(dev, attr))
pci_stop_and_remove_bus_device_locked(to_pci_dev(dev));
return count;
}
@@ -478,13 +477,13 @@ static ssize_t bus_rescan_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
- unsigned long val;
+ bool rescan;
struct pci_bus *bus = to_pci_bus(dev);
- if (kstrtoul(buf, 0, &val) < 0)
+ if (kstrtobool(buf, &rescan) < 0)
return -EINVAL;
- if (val) {
+ if (rescan) {
pci_lock_rescan_remove();
if (!pci_is_root_bus(bus) && list_empty(&bus->devices))
pci_rescan_bus_bridge_resize(bus->self);
@@ -502,14 +501,14 @@ static ssize_t d3cold_allowed_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
{
+ bool allowed;
struct pci_dev *pdev = to_pci_dev(dev);
- unsigned long val;
- if (kstrtoul(buf, 0, &val) < 0)
+ if (kstrtobool(buf, &allowed) < 0)
return -EINVAL;
- pdev->d3cold_allowed = !!val;
- if (pdev->d3cold_allowed)
+ pdev->d3cold_allowed = allowed;
+ if (allowed)
pci_d3cold_enable(pdev);
else
pci_d3cold_disable(pdev);
@@ -1257,12 +1256,13 @@ static ssize_t pci_write_rom(struct file *filp, struct kobject *kobj,
struct bin_attribute *bin_attr, char *buf,
loff_t off, size_t count)
{
+ bool enable;
struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
- if ((off == 0) && (*buf == '0') && (count == 2))
- pdev->rom_attr_enabled = 0;
- else
- pdev->rom_attr_enabled = 1;
+ if (kstrtobool(buf, &enable) < 0)
+ return -EINVAL;
+
+ pdev->rom_attr_enabled = enable;
return count;
}
@@ -1337,23 +1337,20 @@ static const struct attribute_group pci_dev_rom_attr_group = {
static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
+ bool reset;
struct pci_dev *pdev = to_pci_dev(dev);
- unsigned long val;
- ssize_t result = kstrtoul(buf, 0, &val);
-
- if (result < 0)
- return result;
+ ssize_t result = 0;
- if (val != 1)
+ if (kstrtobool(buf, &reset) < 0)
return -EINVAL;
- pm_runtime_get_sync(dev);
- result = pci_reset_function(pdev);
- pm_runtime_put(dev);
- if (result < 0)
- return result;
+ if (reset) {
+ pm_runtime_get_sync(dev);
+ result = pci_reset_function(pdev);
+ pm_runtime_put(dev);
+ }
- return count;
+ return result < 0 ? result : count;
}
static DEVICE_ATTR_WO(reset);
--
2.32.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/4] PCI/sysfs: Check CAP_SYS_ADMIN before parsing user input
2021-07-05 21:23 [PATCH 1/4] PCI/sysfs: Move to kstrtobool() to handle user input Krzysztof Wilczyński
@ 2021-07-05 21:23 ` Krzysztof Wilczyński
2021-09-14 20:40 ` Bjorn Helgaas
2021-07-05 21:23 ` [PATCH 3/4] PCI/sysfs: Return -EINVAL consistently from "store" functions Krzysztof Wilczyński
2021-07-05 21:23 ` [PATCH 4/4] PCI: Don't use the strtobool() wrapper for kstrtobool() Krzysztof Wilczyński
2 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Wilczyński @ 2021-07-05 21:23 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Lorenzo Pieralisi, Kishon Vijay Abraham I, linux-pci
Check if the "CAP_SYS_ADMIN" capability flag is set before parsing user
input as it makes more sense to first check whether the current user
actually has the right permissions before accepting any input from such
user.
This will also make order in which enable_store() and msi_bus_store()
perform the "CAP_SYS_ADMIN" capability check consistent with other
PCI-related sysfs objects that first verify whether user has this
capability set.
Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
drivers/pci/pci-sysfs.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 0f98c4843764..bc4c141e4c1c 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -275,13 +275,13 @@ static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
struct pci_dev *pdev = to_pci_dev(dev);
ssize_t result = 0;
- if (kstrtobool(buf, &enable) < 0)
- return -EINVAL;
-
/* this can crash the machine when done on the "wrong" device */
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
+ if (kstrtobool(buf, &enable) < 0)
+ return -EINVAL;
+
device_lock(dev);
if (dev->driver)
result = -EBUSY;
@@ -377,12 +377,12 @@ static ssize_t msi_bus_store(struct device *dev, struct device_attribute *attr,
struct pci_dev *pdev = to_pci_dev(dev);
struct pci_bus *subordinate = pdev->subordinate;
- if (kstrtobool(buf, &enable) < 0)
- return -EINVAL;
-
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
+ if (kstrtobool(buf, &enable) < 0)
+ return -EINVAL;
+
/*
* "no_msi" and "bus_flags" only affect what happens when a driver
* requests MSI or MSI-X. They don't affect any drivers that have
--
2.32.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/4] PCI/sysfs: Return -EINVAL consistently from "store" functions
2021-07-05 21:23 [PATCH 1/4] PCI/sysfs: Move to kstrtobool() to handle user input Krzysztof Wilczyński
2021-07-05 21:23 ` [PATCH 2/4] PCI/sysfs: Check CAP_SYS_ADMIN before parsing " Krzysztof Wilczyński
@ 2021-07-05 21:23 ` Krzysztof Wilczyński
2021-07-06 0:24 ` kernel test robot
2021-07-05 21:23 ` [PATCH 4/4] PCI: Don't use the strtobool() wrapper for kstrtobool() Krzysztof Wilczyński
2 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Wilczyński @ 2021-07-05 21:23 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Lorenzo Pieralisi, Kishon Vijay Abraham I, linux-pci
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(-)
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;
--
2.32.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 4/4] PCI: Don't use the strtobool() wrapper for kstrtobool()
2021-07-05 21:23 [PATCH 1/4] PCI/sysfs: Move to kstrtobool() to handle user input Krzysztof Wilczyński
2021-07-05 21:23 ` [PATCH 2/4] PCI/sysfs: Check CAP_SYS_ADMIN before parsing " Krzysztof Wilczyński
2021-07-05 21:23 ` [PATCH 3/4] PCI/sysfs: Return -EINVAL consistently from "store" functions Krzysztof Wilczyński
@ 2021-07-05 21:23 ` Krzysztof Wilczyński
2 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Wilczyński @ 2021-07-05 21:23 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Lorenzo Pieralisi, Kishon Vijay Abraham I, linux-pci
The strtobool() function is a wrapper over the kstrtobool() function
that has been added for backward compatibility.
There is no reason to use the old API, thus rather than using the
wrapper use the kstrtobool() directly.
Related:
commit ef951599074b ("lib: move strtobool() to kstrtobool()")
Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
drivers/pci/p2pdma.c | 6 +++---
drivers/pci/pcie/aspm.c | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 196382630363..1bd299cf3872 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -911,7 +911,7 @@ EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_sg_attrs);
*
* Parses an attribute value to decide whether to enable p2pdma.
* The value can select a PCI device (using its full BDF device
- * name) or a boolean (in any format strtobool() accepts). A false
+ * name) or a boolean (in any format kstrtobool() accepts). A false
* value disables p2pdma, a true value expects the caller
* to automatically find a compatible device and specifying a PCI device
* expects the caller to use the specific provider.
@@ -943,11 +943,11 @@ int pci_p2pdma_enable_store(const char *page, struct pci_dev **p2p_dev,
} else if ((page[0] == '0' || page[0] == '1') && !iscntrl(page[1])) {
/*
* If the user enters a PCI device that doesn't exist
- * like "0000:01:00.1", we don't want strtobool to think
+ * like "0000:01:00.1", we don't want kstrtobool to think
* it's a '0' when it's clearly not what the user wanted.
* So we require 0's and 1's to be exactly one character.
*/
- } else if (!strtobool(page, use_p2pdma)) {
+ } else if (!kstrtobool(page, use_p2pdma)) {
return 0;
}
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index ac0557a305af..ba8b17000d94 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1219,7 +1219,7 @@ static ssize_t aspm_attr_store_common(struct device *dev,
struct pcie_link_state *link = pcie_aspm_get_link(pdev);
bool state_enable;
- if (strtobool(buf, &state_enable) < 0)
+ if (kstrtobool(buf, &state_enable) < 0)
return -EINVAL;
down_read(&pci_bus_sem);
@@ -1276,7 +1276,7 @@ static ssize_t clkpm_store(struct device *dev,
struct pcie_link_state *link = pcie_aspm_get_link(pdev);
bool state_enable;
- if (strtobool(buf, &state_enable) < 0)
+ if (kstrtobool(buf, &state_enable) < 0)
return -EINVAL;
down_read(&pci_bus_sem);
--
2.32.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/4] PCI/sysfs: Return -EINVAL consistently from "store" functions
2021-07-05 21:23 ` [PATCH 3/4] PCI/sysfs: Return -EINVAL consistently from "store" functions Krzysztof Wilczyński
@ 2021-07-06 0:24 ` kernel test robot
2021-07-06 0:35 ` Krzysztof Wilczyński
0 siblings, 1 reply; 8+ messages in thread
From: kernel test robot @ 2021-07-06 0:24 UTC (permalink / raw)
To: Krzysztof Wilczyński, Bjorn Helgaas
Cc: clang-built-linux, kbuild-all, Lorenzo Pieralisi,
Kishon Vijay Abraham I, linux-pci
[-- Attachment #1: Type: text/plain, Size: 7773 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45764 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/4] PCI/sysfs: Return -EINVAL consistently from "store" functions
2021-07-06 0:24 ` kernel test robot
@ 2021-07-06 0:35 ` Krzysztof Wilczyński
0 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Wilczyński @ 2021-07-06 0:35 UTC (permalink / raw)
To: kernel test robot
Cc: Bjorn Helgaas, Lorenzo Pieralisi, Kishon Vijay Abraham I, linux-pci
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/4] PCI/sysfs: Check CAP_SYS_ADMIN before parsing user input
2021-07-05 21:23 ` [PATCH 2/4] PCI/sysfs: Check CAP_SYS_ADMIN before parsing " Krzysztof Wilczyński
@ 2021-09-14 20:40 ` Bjorn Helgaas
2021-09-15 1:15 ` Krzysztof Wilczyński
0 siblings, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2021-09-14 20:40 UTC (permalink / raw)
To: Krzysztof Wilczyński
Cc: Bjorn Helgaas, Lorenzo Pieralisi, Kishon Vijay Abraham I, linux-pci
On Mon, Jul 05, 2021 at 09:23:06PM +0000, Krzysztof Wilczyński wrote:
> Check if the "CAP_SYS_ADMIN" capability flag is set before parsing user
> input as it makes more sense to first check whether the current user
> actually has the right permissions before accepting any input from such
> user.
>
> This will also make order in which enable_store() and msi_bus_store()
> perform the "CAP_SYS_ADMIN" capability check consistent with other
> PCI-related sysfs objects that first verify whether user has this
> capability set.
I like this one. Can you rebase it to skip patch 1/4 (unless you
convince me that 1/4 is safe)?
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> ---
> drivers/pci/pci-sysfs.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 0f98c4843764..bc4c141e4c1c 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -275,13 +275,13 @@ static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
> struct pci_dev *pdev = to_pci_dev(dev);
> ssize_t result = 0;
>
> - if (kstrtobool(buf, &enable) < 0)
> - return -EINVAL;
> -
> /* this can crash the machine when done on the "wrong" device */
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
>
> + if (kstrtobool(buf, &enable) < 0)
> + return -EINVAL;
> +
> device_lock(dev);
> if (dev->driver)
> result = -EBUSY;
> @@ -377,12 +377,12 @@ static ssize_t msi_bus_store(struct device *dev, struct device_attribute *attr,
> struct pci_dev *pdev = to_pci_dev(dev);
> struct pci_bus *subordinate = pdev->subordinate;
>
> - if (kstrtobool(buf, &enable) < 0)
> - return -EINVAL;
> -
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
>
> + if (kstrtobool(buf, &enable) < 0)
> + return -EINVAL;
> +
> /*
> * "no_msi" and "bus_flags" only affect what happens when a driver
> * requests MSI or MSI-X. They don't affect any drivers that have
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/4] PCI/sysfs: Check CAP_SYS_ADMIN before parsing user input
2021-09-14 20:40 ` Bjorn Helgaas
@ 2021-09-15 1:15 ` Krzysztof Wilczyński
0 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Wilczyński @ 2021-09-15 1:15 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, Lorenzo Pieralisi, Kishon Vijay Abraham I, linux-pci
Hi Bjorn,
[...]
> > Check if the "CAP_SYS_ADMIN" capability flag is set before parsing user
> > input as it makes more sense to first check whether the current user
> > actually has the right permissions before accepting any input from such
> > user.
> >
> > This will also make order in which enable_store() and msi_bus_store()
> > perform the "CAP_SYS_ADMIN" capability check consistent with other
> > PCI-related sysfs objects that first verify whether user has this
> > capability set.
>
> I like this one. Can you rebase it to skip patch 1/4 (unless you
> convince me that 1/4 is safe)?
I will remove it, as per:
https://lore.kernel.org/linux-pci/20210915011204.GA1444093@rocinante/T/#t
Thank you!
Krzysztof
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-09-15 1:15 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-05 21:23 [PATCH 1/4] PCI/sysfs: Move to kstrtobool() to handle user input Krzysztof Wilczyński
2021-07-05 21:23 ` [PATCH 2/4] PCI/sysfs: Check CAP_SYS_ADMIN before parsing " Krzysztof Wilczyński
2021-09-14 20:40 ` Bjorn Helgaas
2021-09-15 1:15 ` Krzysztof Wilczyński
2021-07-05 21:23 ` [PATCH 3/4] PCI/sysfs: Return -EINVAL consistently from "store" functions Krzysztof Wilczyński
2021-07-06 0:24 ` kernel test robot
2021-07-06 0:35 ` Krzysztof Wilczyński
2021-07-05 21:23 ` [PATCH 4/4] PCI: Don't use the strtobool() wrapper for kstrtobool() Krzysztof Wilczyński
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).