Skip to contentSkip to conversation

PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions

PATCH Applied Krzysztof Wilczyński 3 participants 18 messages Jun 3, 2021 – Jun 4, 2021

Subsystems (6)
PowerPC PCI PCI enhanced error handling (eeh) for PowerPC PCI peer-to-peer DMA (P2PDMA) +2 more subsystems

v6 of 7 Applied Latest is v7 →

lore
Share & tools
lore

Krzysztof Wilczyński

author
#0 lore raw

v6 cover PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions

Hello,

This series aims to bring support for the sysfs_emit() and sysfs_emit_at() functions to the existing PCI-related sysfs objects. These new functions were introduced to make it less ambiguous which function is preferred when writing to the output buffer in a device attribute's "show" callback [1].

Thus, the existing PCI sysfs objects "show" functions will be converted from the using the sprintf(), snprintf() and scnprintf() functions to sysfs_emit() and sysfs_emit_at() accordingly, as the latter is aware of the PAGE_SIZE buffer limit that the sysfs object has and correctly returns the number of bytes written into the buffer.

This series will also address inconsistency related to the presence (or lack of thereof) of a trailing newline in the show() functions adding it where it's currently missing. This will allow for utilities such as the "cat" command to display the result of the read from a particular sysfs object correctly in a shell.

While working on this series a problem with newline handling related to how the value of the "resource_alignment" sysfs object was parsed and then persisted has been found and corrected. Also, while at it, a change enabling support for the value of "resource_alignment" previously set using either a command-line argument or through the sysfs object to be cleared at run-time was also included, and thus aligning this particular sysfs object with the behaviour of other such objects that allow for the value to be dynamically updated and cleared as required.

Additionally, a fix to a potential buffer overrun that has been found in the dsm_label_utf16s_to_utf8s() function that is responsible for the character conversion from UTF16 to UTF8 of the buffer that holds the device label obtained through the ACPI _DSM mechanism is included as part of this series.

Finally, a minor fix is also included in this series that has been added to ensure that the value of the "driver_override" variable is only exposed through the corresponding sysfs object when a value is set or otherwise if the value has not been set, the object would return a string representation of the NULL value. This will also align this particular sysfs object's behaviour with others, where when there is no value then nothing is returned.

[1] Documentation/filesystems/sysfs.rst

This series is related to:
commit ad025f8 ("PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions")

Signed-off-by Krzysztof Wilczyński <[email protected]>

---
Changes in v2:
None.

Changes in v3:
Added Logan Gunthorpe's "Reviewed-by".

Changes in v4:
Separated and squashed all the trivial sysfs_emit()/sysfs_emit_at() changes into a single patch as per Bjorn Helgaas' request. Carried Logan Gunthorpe's "Reviewed-by" over.

Changes in v5:
Added check to the resource_alignment_show() function to ensure that there is an extra space left in the buffer for the newline character, assuming that it might be provided.

Changes in v6:
Added a cover letter as per Bjorn Helgaas' request. New patch addressing a potential buffer overrun in the dsm_label_utf16s_to_utf8s() function has been added.

Krzysztof Wilczyński (6):
PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions PCI/sysfs: Use return value from dsm_label_utf16s_to_utf8s() directly PCI/sysfs: Fix trailing newline handling of resource_alignment_param PCI/sysfs: Add missing trailing newline to devspec_show() PCI/sysfs: Only show value when driver_override is not NULL PCI/sysfs: Fix a buffer overrun problem with

    dsm_label_utf16s_to_utf8s()
 drivers/pci/hotplug/pci_hotplug_core.c |  8 +++---
 drivers/pci/hotplug/rpadlpar_sysfs.c   |  4 +--
 drivers/pci/hotplug/shpchp_sysfs.c     | 38 ++++++++++++++------------
 drivers/pci/iov.c                      | 12 ++++----
 drivers/pci/msi.c                      |  8 +++---
 drivers/pci/p2pdma.c                   |  7 ++---
 drivers/pci/pci-label.c                | 22 ++++++++-------
 drivers/pci/pci-sysfs.c                |  7 +++--
 drivers/pci/pci.c                      | 34 +++++++++++++----------
 drivers/pci/pcie/aer.c                 | 20 ++++++++------
 drivers/pci/pcie/aspm.c                |  4 +--
 drivers/pci/slot.c                     | 18 ++++++------
 drivers/pci/switch/switchtec.c         | 18 ++++++------
 13 files changed, 107 insertions(+), 93 deletions(-)
Signature · 1 line
2.31.1
author
#1 lore raw

PATCH v6 1/6 PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions

The sysfs_emit() and sysfs_emit_at() functions were introduced to make it less ambiguous which function is preferred when writing to the output buffer in a device attribute's "show" callback [1].

Convert the PCI sysfs object "show" functions from sprintf(), snprintf() and scnprintf() to sysfs_emit() and sysfs_emit_at() accordingly, as the latter is aware of the PAGE_SIZE buffer and correctly returns the number of bytes written into the buffer.

No functional change intended.

[1] Documentation/filesystems/sysfs.rst

Related to:
commit ad025f8e46f3 ("PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions")

Signed-off-by Krzysztof Wilczyński <[email protected]>
Reviewed-by Logan Gunthorpe <[email protected]>
---
 drivers/pci/hotplug/pci_hotplug_core.c |  8 +++---
 drivers/pci/hotplug/rpadlpar_sysfs.c   |  4 +--
 drivers/pci/hotplug/shpchp_sysfs.c     | 38 ++++++++++++++------------
 drivers/pci/iov.c                      | 12 ++++----
 drivers/pci/msi.c                      |  8 +++---
 drivers/pci/p2pdma.c                   |  7 ++---
 drivers/pci/pci-label.c                |  6 ++--
 drivers/pci/pci.c                      |  2 +-
 drivers/pci/pcie/aer.c                 | 20 ++++++++------
 drivers/pci/pcie/aspm.c                |  4 +--
 drivers/pci/slot.c                     | 18 ++++++------
 drivers/pci/switch/switchtec.c         | 18 ++++++------
 12 files changed, 75 insertions(+), 70 deletions(-)
drivers/pci/hotplug/pci_hotplug_core.c +4 -4
index 5ac31f683b85..058d5937d8a9 100644
@@ -73,7 +73,7 @@ static ssize_t power_read_file(struct pci_slot *pci_slot, char *buf)
 	if (retval)
 		return retval;
 
-	return sprintf(buf, "%d\n", value);
+	return sysfs_emit(buf, "%d\n", value);
 }
 
 static ssize_t power_write_file(struct pci_slot *pci_slot, const char *buf,

@@ -130,7 +130,7 @@ static ssize_t attention_read_file(struct pci_slot *pci_slot, char *buf)
 	if (retval)
 		return retval;
 
-	return sprintf(buf, "%d\n", value);
+	return sysfs_emit(buf, "%d\n", value);
 }
 
 static ssize_t attention_write_file(struct pci_slot *pci_slot, const char *buf,

@@ -175,7 +175,7 @@ static ssize_t latch_read_file(struct pci_slot *pci_slot, char *buf)
 	if (retval)
 		return retval;
 
-	return sprintf(buf, "%d\n", value);
+	return sysfs_emit(buf, "%d\n", value);
 }
 
 static struct pci_slot_attribute hotplug_slot_attr_latch = {

@@ -192,7 +192,7 @@ static ssize_t presence_read_file(struct pci_slot *pci_slot, char *buf)
 	if (retval)
 		return retval;
 
-	return sprintf(buf, "%d\n", value);
+	return sysfs_emit(buf, "%d\n", value);
 }
 
 static struct pci_slot_attribute hotplug_slot_attr_presence = {
drivers/pci/hotplug/rpadlpar_sysfs.c +2 -2
index dbfa0b55d31a..068b7810a574 100644
@@ -50,7 +50,7 @@ static ssize_t add_slot_store(struct kobject *kobj, struct kobj_attribute *attr,
 static ssize_t add_slot_show(struct kobject *kobj,
 			     struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "0\n");
+	return sysfs_emit(buf, "0\n");
 }
 
 static ssize_t remove_slot_store(struct kobject *kobj,

@@ -80,7 +80,7 @@ static ssize_t remove_slot_store(struct kobject *kobj,
 static ssize_t remove_slot_show(struct kobject *kobj,
 				struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "0\n");
+	return sysfs_emit(buf, "0\n");
 }
 
 static struct kobj_attribute add_slot_attr =
drivers/pci/hotplug/shpchp_sysfs.c +21 -17
index 45658bb5c554..64beed7a26be 100644
@@ -24,50 +24,54 @@
 static ssize_t show_ctrl(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct pci_dev *pdev;
-	char *out = buf;
 	int index, busnr;
 	struct resource *res;
 	struct pci_bus *bus;
+	size_t len = 0;
 
 	pdev = to_pci_dev(dev);
 	bus = pdev->subordinate;
 
-	out += sprintf(buf, "Free resources: memory\n");
+	len += sysfs_emit_at(buf, len, "Free resources: memory\n");
 	pci_bus_for_each_resource(bus, res, index) {
 		if (res && (res->flags & IORESOURCE_MEM) &&
 				!(res->flags & IORESOURCE_PREFETCH)) {
-			out += sprintf(out, "start = %8.8llx, length = %8.8llx\n",
-				       (unsigned long long)res->start,
-				       (unsigned long long)resource_size(res));
+			len += sysfs_emit_at(buf, len,
+					     "start = %8.8llx, length = %8.8llx\n",
+					     (unsigned long long)res->start,
+					     (unsigned long long)resource_size(res));
 		}
 	}
-	out += sprintf(out, "Free resources: prefetchable memory\n");
+	len += sysfs_emit_at(buf, len, "Free resources: prefetchable memory\n");
 	pci_bus_for_each_resource(bus, res, index) {
 		if (res && (res->flags & IORESOURCE_MEM) &&
 			       (res->flags & IORESOURCE_PREFETCH)) {
-			out += sprintf(out, "start = %8.8llx, length = %8.8llx\n",
-				       (unsigned long long)res->start,
-				       (unsigned long long)resource_size(res));
+			len += sysfs_emit_at(buf, len,
+					     "start = %8.8llx, length = %8.8llx\n",
+					     (unsigned long long)res->start,
+					     (unsigned long long)resource_size(res));
 		}
 	}
-	out += sprintf(out, "Free resources: IO\n");
+	len += sysfs_emit_at(buf, len, "Free resources: IO\n");
 	pci_bus_for_each_resource(bus, res, index) {
 		if (res && (res->flags & IORESOURCE_IO)) {
-			out += sprintf(out, "start = %8.8llx, length = %8.8llx\n",
-				       (unsigned long long)res->start,
-				       (unsigned long long)resource_size(res));
+			len += sysfs_emit_at(buf, len,
+					     "start = %8.8llx, length = %8.8llx\n",
+					     (unsigned long long)res->start,
+					     (unsigned long long)resource_size(res));
 		}
 	}
-	out += sprintf(out, "Free resources: bus numbers\n");
+	len += sysfs_emit_at(buf, len, "Free resources: bus numbers\n");
 	for (busnr = bus->busn_res.start; busnr <= bus->busn_res.end; busnr++) {
 		if (!pci_find_bus(pci_domain_nr(bus), busnr))
 			break;
 	}
 	if (busnr < bus->busn_res.end)
-		out += sprintf(out, "start = %8.8x, length = %8.8x\n",
-				busnr, (int)(bus->busn_res.end - busnr));
+		len += sysfs_emit_at(buf, len,
+				     "start = %8.8x, length = %8.8x\n",
+				     busnr, (int)(bus->busn_res.end - busnr));
 
-	return out - buf;
+	return len;
 }
 static DEVICE_ATTR(ctrl, S_IRUGO, show_ctrl, NULL);
drivers/pci/iov.c +6 -6
index afc06e6ce115..a71258347323 100644
@@ -346,7 +346,7 @@ static ssize_t sriov_totalvfs_show(struct device *dev,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	return sprintf(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
+	return sysfs_emit(buf, "%u\n", pci_sriov_get_totalvfs(pdev));
 }
 
 static ssize_t sriov_numvfs_show(struct device *dev,

@@ -361,7 +361,7 @@ static ssize_t sriov_numvfs_show(struct device *dev,
 	num_vfs = pdev->sriov->num_VFs;
 	device_unlock(&pdev->dev);
 
-	return sprintf(buf, "%u\n", num_vfs);
+	return sysfs_emit(buf, "%u\n", num_vfs);
 }
 
 /*

@@ -435,7 +435,7 @@ static ssize_t sriov_offset_show(struct device *dev,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	return sprintf(buf, "%u\n", pdev->sriov->offset);
+	return sysfs_emit(buf, "%u\n", pdev->sriov->offset);
 }
 
 static ssize_t sriov_stride_show(struct device *dev,

@@ -444,7 +444,7 @@ static ssize_t sriov_stride_show(struct device *dev,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	return sprintf(buf, "%u\n", pdev->sriov->stride);
+	return sysfs_emit(buf, "%u\n", pdev->sriov->stride);
 }
 
 static ssize_t sriov_vf_device_show(struct device *dev,

@@ -453,7 +453,7 @@ static ssize_t sriov_vf_device_show(struct device *dev,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	return sprintf(buf, "%x\n", pdev->sriov->vf_device);
+	return sysfs_emit(buf, "%x\n", pdev->sriov->vf_device);
 }
 
 static ssize_t sriov_drivers_autoprobe_show(struct device *dev,

@@ -462,7 +462,7 @@ static ssize_t sriov_drivers_autoprobe_show(struct device *dev,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	return sprintf(buf, "%u\n", pdev->sriov->drivers_autoprobe);
+	return sysfs_emit(buf, "%u\n", pdev->sriov->drivers_autoprobe);
 }
 
 static ssize_t sriov_drivers_autoprobe_store(struct device *dev,
drivers/pci/msi.c +4 -4
index 217dc9f0231f..9232255c8515 100644
@@ -464,11 +464,11 @@ static ssize_t msi_mode_show(struct device *dev, struct device_attribute *attr,
 		return retval;
 
 	entry = irq_get_msi_desc(irq);
-	if (entry)
-		return sprintf(buf, "%s\n",
-				entry->msi_attrib.is_msix ? "msix" : "msi");
+	if (!entry)
+		return -ENODEV;
 
-	return -ENODEV;
+	return sysfs_emit(buf, "%s\n",
+			  entry->msi_attrib.is_msix ? "msix" : "msi");
 }
 
 static int populate_msi_sysfs(struct pci_dev *pdev)
drivers/pci/p2pdma.c +3 -4
index 196382630363..a1351b3e2c4c 100644
@@ -53,7 +53,7 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr,
 	if (pdev->p2pdma->pool)
 		size = gen_pool_size(pdev->p2pdma->pool);
 
-	return scnprintf(buf, PAGE_SIZE, "%zd\n", size);
+	return sysfs_emit(buf, "%zd\n", size);
 }
 static DEVICE_ATTR_RO(size);
 

@@ -66,7 +66,7 @@ static ssize_t available_show(struct device *dev, struct device_attribute *attr,
 	if (pdev->p2pdma->pool)
 		avail = gen_pool_avail(pdev->p2pdma->pool);
 
-	return scnprintf(buf, PAGE_SIZE, "%zd\n", avail);
+	return sysfs_emit(buf, "%zd\n", avail);
 }
 static DEVICE_ATTR_RO(available);
 

@@ -75,8 +75,7 @@ static ssize_t published_show(struct device *dev, struct device_attribute *attr,
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 
-	return scnprintf(buf, PAGE_SIZE, "%d\n",
-			 pdev->p2pdma->p2pmem_published);
+	return sysfs_emit(buf, "%d\n", pdev->p2pdma->p2pmem_published);
 }
 static DEVICE_ATTR_RO(published);
drivers/pci/pci-label.c +3 -3
index c32f3b7540e8..311dd48e2881 100644
@@ -175,11 +175,11 @@ static int dsm_get_label(struct device *dev, char *buf,
 		 * this entry must return a null string.
 		 */
 		if (attr == ACPI_ATTR_INDEX_SHOW) {
-			scnprintf(buf, PAGE_SIZE, "%llu\n", tmp->integer.value);
+			sysfs_emit(buf, "%llu\n", tmp->integer.value);
 		} else if (attr == ACPI_ATTR_LABEL_SHOW) {
 			if (tmp[1].type == ACPI_TYPE_STRING)
-				scnprintf(buf, PAGE_SIZE, "%s\n",
-					  tmp[1].string.pointer);
+				sysfs_emit(buf, "%s\n",
+					   tmp[1].string.pointer);
 			else if (tmp[1].type == ACPI_TYPE_BUFFER)
 				dsm_label_utf16s_to_utf8s(tmp + 1, buf);
 		}
drivers/pci/pci.c +1 -1
index b717680377a9..5ed316ea5831 100644
@@ -6439,7 +6439,7 @@ static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
 
 	spin_lock(&resource_alignment_lock);
 	if (resource_alignment_param)
-		count = scnprintf(buf, PAGE_SIZE, "%s", resource_alignment_param);
+		count = sysfs_emit(buf, "%s", resource_alignment_param);
 	spin_unlock(&resource_alignment_lock);
 
 	/*
drivers/pci/pcie/aer.c +11 -9
index ec943cee5ecc..40ef7bed7a77 100644
@@ -529,21 +529,23 @@ static const char *aer_agent_string[] = {
 		     char *buf)						\
 {									\
 	unsigned int i;							\
-	char *str = buf;						\
 	struct pci_dev *pdev = to_pci_dev(dev);				\
 	u64 *stats = pdev->aer_stats->stats_array;			\
+	size_t len = 0;							\
 									\
 	for (i = 0; i < ARRAY_SIZE(strings_array); i++) {		\
 		if (strings_array[i])					\
-			str += sprintf(str, "%s %llu\n",		\
-				       strings_array[i], stats[i]);	\
+			len += sysfs_emit_at(buf, len, "%s %llu\n",	\
+					     strings_array[i],		\
+					     stats[i]);			\
 		else if (stats[i])					\
-			str += sprintf(str, #stats_array "_bit[%d] %llu\n",\
-				       i, stats[i]);			\
+			len += sysfs_emit_at(buf, len,			\
+					     #stats_array "_bit[%d] %llu\n",\
+					     i, stats[i]);		\
 	}								\
-	str += sprintf(str, "TOTAL_%s %llu\n", total_string,		\
-		       pdev->aer_stats->total_field);			\
-	return str-buf;							\
+	len += sysfs_emit_at(buf, len, "TOTAL_%s %llu\n", total_string,	\
+			     pdev->aer_stats->total_field);		\
+	return len;							\
 }									\
 static DEVICE_ATTR_RO(name)
 

@@ -563,7 +565,7 @@ aer_stats_dev_attr(aer_dev_nonfatal, dev_nonfatal_errs,
 		     char *buf)						\
 {									\
 	struct pci_dev *pdev = to_pci_dev(dev);				\
-	return sprintf(buf, "%llu\n", pdev->aer_stats->field);		\
+	return sysfs_emit(buf, "%llu\n", pdev->aer_stats->field);	\
 }									\
 static DEVICE_ATTR_RO(name)
drivers/pci/pcie/aspm.c +2 -2
index ac0557a305af..013a47f587ce 100644
@@ -1208,7 +1208,7 @@ static ssize_t aspm_attr_show_common(struct device *dev,
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
 
-	return sprintf(buf, "%d\n", (link->aspm_enabled & state) ? 1 : 0);
+	return sysfs_emit(buf, "%d\n", (link->aspm_enabled & state) ? 1 : 0);
 }
 
 static ssize_t aspm_attr_store_common(struct device *dev,

@@ -1265,7 +1265,7 @@ static ssize_t clkpm_show(struct device *dev,
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct pcie_link_state *link = pcie_aspm_get_link(pdev);
 
-	return sprintf(buf, "%d\n", link->clkpm_enabled);
+	return sysfs_emit(buf, "%d\n", link->clkpm_enabled);
 }
 
 static ssize_t clkpm_store(struct device *dev,
drivers/pci/slot.c +9 -9
index d627dd9179b4..751a26668e3a 100644
@@ -39,19 +39,19 @@ static const struct sysfs_ops pci_slot_sysfs_ops = {
 static ssize_t address_read_file(struct pci_slot *slot, char *buf)
 {
 	if (slot->number == 0xff)
-		return sprintf(buf, "%04x:%02x\n",
-				pci_domain_nr(slot->bus),
-				slot->bus->number);
-	else
-		return sprintf(buf, "%04x:%02x:%02x\n",
-				pci_domain_nr(slot->bus),
-				slot->bus->number,
-				slot->number);
+		return sysfs_emit(buf, "%04x:%02x\n",
+				  pci_domain_nr(slot->bus),
+				  slot->bus->number);
+
+	return sysfs_emit(buf, "%04x:%02x:%02x\n",
+			  pci_domain_nr(slot->bus),
+			  slot->bus->number,
+			  slot->number);
 }
 
 static ssize_t bus_speed_read(enum pci_bus_speed speed, char *buf)
 {
-	return sprintf(buf, "%s\n", pci_speed_string(speed));
+	return sysfs_emit(buf, "%s\n", pci_speed_string(speed));
 }
 
 static ssize_t max_speed_read_file(struct pci_slot *slot, char *buf)
drivers/pci/switch/switchtec.c +9 -9
index ba52459928f7..0b301f8be9ed 100644
@@ -280,7 +280,7 @@ static ssize_t device_version_show(struct device *dev,
 
 	ver = ioread32(&stdev->mmio_sys_info->device_version);
 
-	return sprintf(buf, "%x\n", ver);
+	return sysfs_emit(buf, "%x\n", ver);
 }
 static DEVICE_ATTR_RO(device_version);
 

@@ -292,7 +292,7 @@ static ssize_t fw_version_show(struct device *dev,
 
 	ver = ioread32(&stdev->mmio_sys_info->firmware_version);
 
-	return sprintf(buf, "%08x\n", ver);
+	return sysfs_emit(buf, "%08x\n", ver);
 }
 static DEVICE_ATTR_RO(fw_version);
 

@@ -344,7 +344,7 @@ static ssize_t component_vendor_show(struct device *dev,
 
 	/* component_vendor field not supported after gen3 */
 	if (stdev->gen != SWITCHTEC_GEN3)
-		return sprintf(buf, "none\n");
+		return sysfs_emit(buf, "none\n");
 
 	return io_string_show(buf, &si->gen3.component_vendor,
 			      sizeof(si->gen3.component_vendor));

@@ -359,9 +359,9 @@ static ssize_t component_id_show(struct device *dev,
 
 	/* component_id field not supported after gen3 */
 	if (stdev->gen != SWITCHTEC_GEN3)
-		return sprintf(buf, "none\n");
+		return sysfs_emit(buf, "none\n");
 
-	return sprintf(buf, "PM%04X\n", id);
+	return sysfs_emit(buf, "PM%04X\n", id);
 }
 static DEVICE_ATTR_RO(component_id);
 

@@ -373,9 +373,9 @@ static ssize_t component_revision_show(struct device *dev,
 
 	/* component_revision field not supported after gen3 */
 	if (stdev->gen != SWITCHTEC_GEN3)
-		return sprintf(buf, "255\n");
+		return sysfs_emit(buf, "255\n");
 
-	return sprintf(buf, "%d\n", rev);
+	return sysfs_emit(buf, "%d\n", rev);
 }
 static DEVICE_ATTR_RO(component_revision);
 

@@ -384,7 +384,7 @@ static ssize_t partition_show(struct device *dev,
 {
 	struct switchtec_dev *stdev = to_stdev(dev);
 
-	return sprintf(buf, "%d\n", stdev->partition);
+	return sysfs_emit(buf, "%d\n", stdev->partition);
 }
 static DEVICE_ATTR_RO(partition);
 

@@ -393,7 +393,7 @@ static ssize_t partition_count_show(struct device *dev,
 {
 	struct switchtec_dev *stdev = to_stdev(dev);
 
-	return sprintf(buf, "%d\n", stdev->partition_count);
+	return sysfs_emit(buf, "%d\n", stdev->partition_count);
 }
 static DEVICE_ATTR_RO(partition_count);
Signature · 1 line
2.31.1
author
#2 lore raw

PATCH v6 2/6 PCI/sysfs: Use return value from dsm_label_utf16s_to_utf8s() directly

In reply to Krzysztof Wilczyński

Modify the function dsm_label_utf16s_to_utf8s() to directly return the number of bytes written into the buffer so that the strlen() used later to calculate the length of the buffer can be removed as it would no longer be needed.

No functional change intended.

Signed-off-by Krzysztof Wilczyński <[email protected]>
Reviewed-by Logan Gunthorpe <[email protected]>
---
 drivers/pci/pci-label.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
drivers/pci/pci-label.c +10 -8
index 311dd48e2881..000e169c7197 100644
@@ -139,14 +139,17 @@ enum acpi_attr_enum {
 	ACPI_ATTR_INDEX_SHOW,
 };
 
-static void dsm_label_utf16s_to_utf8s(union acpi_object *obj, char *buf)
+static int dsm_label_utf16s_to_utf8s(union acpi_object *obj, char *buf)
 {
 	int len;
+
 	len = utf16s_to_utf8s((const wchar_t *)obj->buffer.pointer,
 			      obj->buffer.length,
 			      UTF16_LITTLE_ENDIAN,
 			      buf, PAGE_SIZE);
 	buf[len] = '\n';
+
+	return len;
 }
 
 static int dsm_get_label(struct device *dev, char *buf,

@@ -154,7 +157,7 @@ static int dsm_get_label(struct device *dev, char *buf,
 {
 	acpi_handle handle = ACPI_HANDLE(dev);
 	union acpi_object *obj, *tmp;
-	int len = -1;
+	int len = 0;
 
 	if (!handle)
 		return -1;

@@ -175,20 +178,19 @@ static int dsm_get_label(struct device *dev, char *buf,
 		 * this entry must return a null string.
 		 */
 		if (attr == ACPI_ATTR_INDEX_SHOW) {
-			sysfs_emit(buf, "%llu\n", tmp->integer.value);
+			len = sysfs_emit(buf, "%llu\n", tmp->integer.value);
 		} else if (attr == ACPI_ATTR_LABEL_SHOW) {
 			if (tmp[1].type == ACPI_TYPE_STRING)
-				sysfs_emit(buf, "%s\n",
-					   tmp[1].string.pointer);
+				len = sysfs_emit(buf, "%s\n",
+						 tmp[1].string.pointer);
 			else if (tmp[1].type == ACPI_TYPE_BUFFER)
-				dsm_label_utf16s_to_utf8s(tmp + 1, buf);
+				len = dsm_label_utf16s_to_utf8s(tmp + 1, buf);
 		}
-		len = strlen(buf) > 0 ? strlen(buf) : -1;
 	}
 
 	ACPI_FREE(obj);
 
-	return len;
+	return len > 0 ? len : -1;
 }
 
 static ssize_t label_show(struct device *dev, struct device_attribute *attr,
Signature · 1 line
2.31.1
author
#3 lore raw

PATCH v6 3/6 PCI/sysfs: Fix trailing newline handling of resource_alignment_param

In reply to Krzysztof Wilczyński

The value of the "resource_alignment" can be specified using a kernel command-line argument (using the "pci=resource_alignment=") or through the corresponding sysfs object under the /sys/bus/pci path.

Currently, when the value is set via the kernel command-line argument, and then subsequently accessed through sysfs object, the value read back will not be correct, as per:

  # grep -oE 'pci=resource_alignment.+' /proc/cmdline
  pci=resource_alignment=20@00:1f.2
  # cat /sys/bus/pci/resource_alignment
  20@00:1f.

This is also true when the value is set through the sysfs object, but the trailing newline has not been included, as per:

  # echo -n 20@00:1f.2 > /sys/bus/pci/resource_alignment
  # cat /sys/bus/pci/resource_alignment
  20@00:1f.

When the value set through the sysfs object includes the trailing newline, then reading it back will work as intended, as per:

  # echo 20@00:1f.2 > /sys/bus/pci/resource_alignment
  # cat /sys/bus/pci/resource_alignment
  20@00:1f.2

To fix this inconsistency, append a trailing newline in the show() function and strip the trailing line in the store() function if one is present.

Also, allow for the value previously set using either a command-line argument or through the sysfs object to be cleared at run-time.

Fixes e499081da1a2 ("PCI: Force trailing new line to resource_alignment_param in sysfs")
Signed-off-by Krzysztof Wilczyński <[email protected]>
Reviewed-by Logan Gunthorpe <[email protected]>
---
 drivers/pci/pci.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)
drivers/pci/pci.c +20 -14
index 5ed316ea5831..b46445a49543 100644
@@ -6439,34 +6439,40 @@ static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
 
 	spin_lock(&resource_alignment_lock);
 	if (resource_alignment_param)
-		count = sysfs_emit(buf, "%s", resource_alignment_param);
+		count = sysfs_emit(buf, "%s\n", resource_alignment_param);
 	spin_unlock(&resource_alignment_lock);
 
-	/*
-	 * When set by the command line, resource_alignment_param will not
-	 * have a trailing line feed, which is ugly. So conditionally add
-	 * it here.
-	 */
-	if (count >= 2 && buf[count - 2] != '\n' && count < PAGE_SIZE - 1) {
-		buf[count - 1] = '\n';
-		buf[count++] = 0;
-	}
-
 	return count;
 }
 
 static ssize_t resource_alignment_store(struct bus_type *bus,
 					const char *buf, size_t count)
 {
-	char *param = kstrndup(buf, count, GFP_KERNEL);
+	char *param, *old, *end;
+
+	if (count >= (PAGE_SIZE - 1))
+		return -EINVAL;
 
+	param = kstrndup(buf, count, GFP_KERNEL);
 	if (!param)
 		return -ENOMEM;
 
+	end = strchr(param, '\n');
+	if (end)
+		*end = '\0';
+
 	spin_lock(&resource_alignment_lock);
-	kfree(resource_alignment_param);
-	resource_alignment_param = param;
+	old = resource_alignment_param;
+	if (strlen(param)) {
+		resource_alignment_param = param;
+	} else {
+		kfree(resource_alignment_param);
+		resource_alignment_param = NULL;
+	}
 	spin_unlock(&resource_alignment_lock);
+
+	kfree(old);
+
 	return count;
 }
Signature · 1 line
2.31.1
#4 lore raw

Re: [PATCH v6 3/6] PCI/sysfs: Fix trailing newline handling of resource_alignment_param

Quoted 81 lines · Krzysztof Wilczyński

The value of the "resource_alignment" can be specified using a kernel command-line argument (using the "pci=resource_alignment=") or through the corresponding sysfs object under the /sys/bus/pci path.

Currently, when the value is set via the kernel command-line argument, and then subsequently accessed through sysfs object, the value read back will not be correct, as per:

  # grep -oE 'pci=resource_alignment.+' /proc/cmdline
  pci=resource_alignment=20@00:1f.2
  # cat /sys/bus/pci/resource_alignment
  20@00:1f.

This is also true when the value is set through the sysfs object, but the trailing newline has not been included, as per:

  # echo -n 20@00:1f.2 > /sys/bus/pci/resource_alignment
  # cat /sys/bus/pci/resource_alignment
  20@00:1f.

When the value set through the sysfs object includes the trailing newline, then reading it back will work as intended, as per:

  # echo 20@00:1f.2 > /sys/bus/pci/resource_alignment
  # cat /sys/bus/pci/resource_alignment
  20@00:1f.2

To fix this inconsistency, append a trailing newline in the show() function and strip the trailing line in the store() function if one is present.

Also, allow for the value previously set using either a command-line argument or through the sysfs object to be cleared at run-time.

Fixes e499081da1a2 ("PCI: Force trailing new line to resource_alignment_param in sysfs")
Signed-off-by Krzysztof Wilczyński <[email protected]>
Reviewed-by Logan Gunthorpe <[email protected]>
---
 drivers/pci/pci.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)
drivers/pci/pci.c +8 -12
index 5ed316ea5831..b46445a49543 100644
@@ -6439,34 +6439,40 @@ static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
 
 	spin_lock(&resource_alignment_lock);
 	if (resource_alignment_param)
-		count = sysfs_emit(buf, "%s", resource_alignment_param);
+		count = sysfs_emit(buf, "%s\n", resource_alignment_param);
 	spin_unlock(&resource_alignment_lock);
 
-	/*
-	 * When set by the command line, resource_alignment_param will not
-	 * have a trailing line feed, which is ugly. So conditionally add
-	 * it here.
-	 */
-	if (count >= 2 && buf[count - 2] != '\n' && count < PAGE_SIZE - 1) {
-		buf[count - 1] = '\n';
-		buf[count++] = 0;
-	}
-
 	return count;
 }
 
 static ssize_t resource_alignment_store(struct bus_type *bus,
 					const char *buf, size_t count)
 {
-	char *param = kstrndup(buf, count, GFP_KERNEL);
+	char *param, *old, *end;
+
+	if (count >= (PAGE_SIZE - 1))
+		return -EINVAL;
 
+	param = kstrndup(buf, count, GFP_KERNEL);
 	if (!param)
 		return -ENOMEM;
 
+	end = strchr(param, '\n');
+	if (end)

drivers/pci/pci.c static ssize_t resource_alignment_show(struct bus_type *bus, char *buf) · line 6469

Quoted 10 lines
drivers/pci/pci.c +7 -2
+		*end = '\0';
+
 	spin_lock(&resource_alignment_lock);
-	kfree(resource_alignment_param);
-	resource_alignment_param = param;
+	old = resource_alignment_param;
+	if (strlen(param)) {
+		resource_alignment_param = param;
+	} else {
+		kfree(resource_alignment_param);

When "strlen(param) == 0", don't we kfree resource_alignment_param twice? Once here,

drivers/pci/pci.c static ssize_t resource_alignment_show(struct bus_type *bus, char *buf) · line 6474

Quoted 5 lines
drivers/pci/pci.c +4 -0
+		resource_alignment_param = NULL;
+	}
 	spin_unlock(&resource_alignment_lock);
+
+	kfree(old);

and again here?

Quoted 6 lines
drivers/pci/pci.c +0 -0
 	return count;
 }
Signature · 1 line
2.31.1
author
#5 lore raw

Re: [PATCH v6 3/6] PCI/sysfs: Fix trailing newline handling of resource_alignment_param

Hi Bjorn,

[...]

Quoted 8 lines

drivers/pci/pci.c static ssize_t resource_alignment_show(struct bus_type *bus, char *buf) · line 6469

Quoted 5 lines
drivers/pci/pci.c +5 -0
+	old = resource_alignment_param;
+	if (strlen(param)) {
+		resource_alignment_param = param;
+	} else {
+		kfree(resource_alignment_param);

When "strlen(param) == 0", don't we kfree resource_alignment_param twice? Once here,

Yes, we should be freeing the allocation made using kstrndup() here, this is my bad, I completely missed that the wrong variable name was used.

Quoted 9 lines

drivers/pci/pci.c static ssize_t resource_alignment_show(struct bus_type *bus, char *buf) · line 6474

Quoted 5 lines
drivers/pci/pci.c +4 -0
+		resource_alignment_param = NULL;
+	}
 	spin_unlock(&resource_alignment_lock);
+
+	kfree(old);

and again here?

Quoted 1 line

return count;

[...]

Yes, good catch! I am going to send v7 shortly. Apologies!

Krzysztof

author
#6 lore raw

PATCH v6 4/6 PCI/sysfs: Add missing trailing newline to devspec_show()

In reply to Krzysztof Wilczyński

At the moment, when the value of the "devspec" sysfs object is read from the user space there will be no newline present, and the utilities such as the "cat" command won't display the result of the read correctly in a shell, as the trailing newline is currently missing.

To fix this, append a newline character in the show() function.

Signed-off-by Krzysztof Wilczyński <[email protected]>
Reviewed-by Logan Gunthorpe <[email protected]>
---
 drivers/pci/pci-sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/pci-sysfs.c +1 -1
index beb8d1f4fafe..5d63df7c1820 100644
@@ -537,7 +537,7 @@ static ssize_t devspec_show(struct device *dev,
 
 	if (np == NULL)
 		return 0;
-	return sysfs_emit(buf, "%pOF", np);
+	return sysfs_emit(buf, "%pOF\n", np);
 }
 static DEVICE_ATTR_RO(devspec);
 #endif
Signature · 1 line
2.31.1
#7 lore raw

Re: [PATCH v6 4/6] PCI/sysfs: Add missing trailing newline to devspec_show()

[+cc Sebastian]

Quoted 6 lines · Krzysztof Wilczyński

At the moment, when the value of the "devspec" sysfs object is read from the user space there will be no newline present, and the utilities such as the "cat" command won't display the result of the read correctly in a shell, as the trailing newline is currently missing.

To fix this, append a newline character in the show() function.

There are a few other devspec_show() functions:

arch/powerpc/platforms/pseries/ibmebus.c
arch/powerpc/platforms/pseries/vio.c
arch/sparc/kernel/vio.c
drivers/usb/core/sysfs.c

and they all include the newline. So I assume it's not likely that this minor user-visible change will break something.

I did cc: Sebastian, since his dfc73e7acd99 ("PCI: Move Open Firmware devspec attribute to PCI common code") moved this code to pci-sysfs.c in the first place (it came from microblaze and powerpc code that *also* did not include the newline).

I notice that pci-sysfs.c is the only one that returns 0 if of_node is NULL. Probably makes more sense than what the others do. I'm guessing the others would print "(null)" which doesn't seem quite right for a sysfs attribute.

But pci-sysfs.c also goes to a little more work than necessary to look up of_node:

  struct pci_dev *pdev = to_pci_dev(dev);
  struct device_node *np = pci_device_to_OF_node(pdev);

when I think this would be equivalent:

  struct device_node *np = dev->of_node;

Some of the others do a similar dance with struct platform_device.

Why'd I write all the above? I dunno; this looks good to me, no action required for you :)

Quoted 22 lines
Signed-off-by Krzysztof Wilczyński <[email protected]>
Reviewed-by Logan Gunthorpe <[email protected]>
---
 drivers/pci/pci-sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/pci-sysfs.c +1 -1
index beb8d1f4fafe..5d63df7c1820 100644
@@ -537,7 +537,7 @@ static ssize_t devspec_show(struct device *dev,
 
 	if (np == NULL)
 		return 0;
-	return sysfs_emit(buf, "%pOF", np);
+	return sysfs_emit(buf, "%pOF\n", np);
 }
 static DEVICE_ATTR_RO(devspec);
 #endif
Signature · 1 line
2.31.1
author
#8 lore raw

PATCH v6 5/6 PCI/sysfs: Only show value when driver_override is not NULL

In reply to Krzysztof Wilczyński

Only expose the value of the "driver_override" variable through the corresponding sysfs object when a value is actually set.

Signed-off-by Krzysztof Wilczyński <[email protected]>
Reviewed-by Logan Gunthorpe <[email protected]>
---
 drivers/pci/pci-sysfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/pci/pci-sysfs.c +3 -2
index 5d63df7c1820..4e9f582ca10f 100644
@@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	ssize_t len;
+	ssize_t len = 0;
 
 	device_lock(dev);
-	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
+	if (pdev->driver_override)
+		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
 	device_unlock(dev);
 	return len;
 }
Signature · 1 line
2.31.1
#9 lore raw

Re: [PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

Quoted 2 lines · Krzysztof Wilczyński

Only expose the value of the "driver_override" variable through the corresponding sysfs object when a value is actually set.

What's the reason for this change? The above tells me what it *does*, but not *why* or whether it affects users.

Is this to avoid trying to print a NULL pointer as %s? Do we print "(null)" or something in that case now? I assume sprintf() doesn't actually oops. If we change what appears in sysfs, we should mention that here. And maybe consider whether there's any chance of breaking user code that might know what to do with "(null)" but not with an empty string.

There are six other driver_override_show() methods. Five don't check the ->driver_override pointer at all; one (spi.c) checks like this:

  len = snprintf(buf, PAGE_SIZE, "%s\n", spi->driver_override ? : "");

Do the others need similar fixes? Most of them still use sprintf() also.

Quoted 27 lines
Signed-off-by Krzysztof Wilczyński <[email protected]>
Reviewed-by Logan Gunthorpe <[email protected]>
---
 drivers/pci/pci-sysfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/pci/pci-sysfs.c +3 -2
index 5d63df7c1820..4e9f582ca10f 100644
@@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	ssize_t len;
+	ssize_t len = 0;
 
 	device_lock(dev);
-	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
+	if (pdev->driver_override)
+		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
 	device_unlock(dev);
 	return len;
 }
Signature · 1 line
2.31.1
#10 lore raw

Re: [PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

Quoted 14 lines · Bjorn Helgaas
Quoted 2 lines · Krzysztof Wilczyński

Only expose the value of the "driver_override" variable through the corresponding sysfs object when a value is actually set.

What's the reason for this change? The above tells me what it *does*, but not *why* or whether it affects users.

Is this to avoid trying to print a NULL pointer as %s? Do we print "(null)" or something in that case now? I assume sprintf() doesn't actually oops. If we change what appears in sysfs, we should mention that here. And maybe consider whether there's any chance of breaking user code that might know what to do with "(null)" but not with an empty string.

Quoted 7 lines

There are six other driver_override_show() methods. Five don't check the ->driver_override pointer at all; one (spi.c) checks like this:

  len = snprintf(buf, PAGE_SIZE, "%s\n", spi->driver_override ? : "");

Do the others need similar fixes? Most of them still use sprintf() also.

I can't remember if there's a reason for holding device_lock() around this. Of the seven: amba, platform, vmbus, pci, s390, and spi hold it, while fsl-mc does not.

Since we're only reading a single scalar, I don't see the reason for device_lock(). If we do need it, it would be nice to have a brief comment explaining why. Obviously not an issue with this patch :)

Quoted 27 lines
Quoted 27 lines
Signed-off-by Krzysztof Wilczyński <[email protected]>
Reviewed-by Logan Gunthorpe <[email protected]>
---
 drivers/pci/pci-sysfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/pci/pci-sysfs.c +3 -2
index 5d63df7c1820..4e9f582ca10f 100644
@@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	ssize_t len;
+	ssize_t len = 0;
 
 	device_lock(dev);
-	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
+	if (pdev->driver_override)
+		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
 	device_unlock(dev);
 	return len;
 }
Signature · 1 line
2.31.1
author
#11 lore raw

Re: [PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

In reply to Bjorn Helgaas

Hi Bjorn,

Thank you for the review!

[...]

Quoted 5 lines
Quoted 2 lines

Only expose the value of the "driver_override" variable through the corresponding sysfs object when a value is actually set.

What's the reason for this change? The above tells me what it *does*, but not *why* or whether it affects users.

Good point! I am going to update the commit message with more details about why this change in the next version. Sorry about this!

Quoted 6 lines

Is this to avoid trying to print a NULL pointer as %s? Do we print "(null)" or something in that case now? I assume sprintf() doesn't actually oops. If we change what appears in sysfs, we should mention that here. And maybe consider whether there's any chance of breaking user code that might know what to do with "(null)" but not with an empty string.

Yes, sprintf() deals with a NULL pointer and prints "(null)", and I personally think that this is not very useful, especially in the case of this particular sysfs object - I would also prefer not to expose the notion of a NULL-value to the userspace this way, something we ought to handle properly, see for example how the "resource_alignment" sysfs object behaves when there is no value set, as per:

  # wc /sys/bus/pci/resource_alignment
     0    0    0 /sys/bus/pci/resource_alignment
Quoted 4 lines

There are six other driver_override_show() methods. Five don't check the ->driver_override pointer at all; one (spi.c) checks like this:

  len = snprintf(buf, PAGE_SIZE, "%s\n", spi->driver_override ? : "");

For an empty sysfs object, as per the sysfs recommendations, I believe that having 0 bytes might be more appropriate to indicate lack of any sensible value. Using "" (empty string) like in the example above would still warrant adding a trailing newline character, which would make it empty (technically), but certainly not 0 bytes. Having said that, some drivers opt not to add a particular sysfs object at all should there be no value for it.

Quoted 2 lines

Do the others need similar fixes? Most of them still use sprintf() also.

[...]

I would like to think that we should handle NULL-value in sysfs objects sensibly everywhere, so the other cases you were able to find could be updated.

Krzysztof

#12 lore raw

Re: [PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

In reply to Krzysztof Wilczyński

[+cc Alex, FYI]

Quoted 2 lines · Krzysztof Wilczyński

Only expose the value of the "driver_override" variable through the corresponding sysfs object when a value is actually set.

This changes the attribute contents from "(null)" to an empty (zero-length) file when no driver override has been set.

There are a few other driver_override_show() functions. Most don't check the pointer so they'll show "(null)". One (spi.c) checks and shows an empty string ("", file containing a single NULL character) instead of an empty (zero-length) file.

Quoted 27 lines
Signed-off-by Krzysztof Wilczyński <[email protected]>
Reviewed-by Logan Gunthorpe <[email protected]>
---
 drivers/pci/pci-sysfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/pci/pci-sysfs.c +3 -2
index 5d63df7c1820..4e9f582ca10f 100644
@@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	ssize_t len;
+	ssize_t len = 0;
 
 	device_lock(dev);
-	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
+	if (pdev->driver_override)
+		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
 	device_unlock(dev);
 	return len;
 }
Signature · 1 line
2.31.1
#13 lore raw

Re: [PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

Quoted 6 lines · On Thu, 3 Jun 2021 18:23:34 -0500 Bjorn Helgaas <[email protected]> wrote:

[+cc Alex, FYI]

Quoted 2 lines · Krzysztof Wilczyński

Only expose the value of the "driver_override" variable through the corresponding sysfs object when a value is actually set.

Quoted 7 lines

This changes the attribute contents from "(null)" to an empty (zero-length) file when no driver override has been set.

There are a few other driver_override_show() functions. Most don't check the pointer so they'll show "(null)". One (spi.c) checks and shows an empty string ("", file containing a single NULL character) instead of an empty (zero-length) file.

Yeah, "(null)" was the expected output in this case. It looks like this might break driverctl. Thanks,

Alex

Quoted 28 lines
Quoted 27 lines
Signed-off-by Krzysztof Wilczyński <[email protected]>
Reviewed-by Logan Gunthorpe <[email protected]>
---
 drivers/pci/pci-sysfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/pci/pci-sysfs.c +3 -2
index 5d63df7c1820..4e9f582ca10f 100644
@@ -580,10 +580,11 @@ static ssize_t driver_override_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	ssize_t len;
+	ssize_t len = 0;
 
 	device_lock(dev);
-	len = sysfs_emit(buf, "%s\n", pdev->driver_override);
+	if (pdev->driver_override)
+		len = sysfs_emit(buf, "%s\n", pdev->driver_override);
 	device_unlock(dev);
 	return len;
 }
Signature · 1 line
2.31.1
author
#14 lore raw

Re: [PATCH v6 5/6] PCI/sysfs: Only show value when driver_override is not NULL

Hi Alex and Bjorn,

[...]

Quoted 10 lines
Quoted 7 lines

This changes the attribute contents from "(null)" to an empty (zero-length) file when no driver override has been set.

There are a few other driver_override_show() functions. Most don't check the pointer so they'll show "(null)". One (spi.c) checks and shows an empty string ("", file containing a single NULL character) instead of an empty (zero-length) file.

Yeah, "(null)" was the expected output in this case. It looks like this might break driverctl. Thanks,

[...]

I had a look and it does, indeed, rely on explicitly checking whether the value is "(null)", as per:

https://gitlab.com/driverctl/driverctl/-/blob/master/driverctl#L96-131

I think, to avoid breaking this and other userspace tools that might rely on this, we should drop this patch.

Having said that, I would love to be able to correct the behaviour we have at the moment, but it seems that this ship has sail, so to speak.

Krzysztof

author
#15 lore raw

PATCH v6 6/6 PCI/sysfs: Fix a buffer overrun problem with dsm_label_utf16s_to_utf8s()

In reply to Krzysztof Wilczyński

When using ACPI to retrieve a device label from _DSM using the dsm_get_label() function the result can be retrieved either as a string value or a pointer to a buffer.

Should it be the latter, then an additional function called dsm_label_utf16s_to_utf8s() will be invoked to convert any UTF16 characters to UTF8 so that the value could be safely stored and later displayed through a relevant sysfs object.

Internally, dsm_label_utf16s_to_utf8s() calls the utf16s_to_utf8s() function to handle the conversion setting the limit of the data that can be saved into a provided buffer as a result of the character conversion to be a total of PAGE_SIZE. The utf16s_to_utf8s() function returns an integer value denoting the number of bytes that have been written into the provided buffer.

Following the execution of the utf16s_to_utf8s() a newline character will be added at the end of the resulting buffer so that when the value is read in the userspace through the sysfs object then it would include newline making it more accessible when working with the sysfs file system in the shell, etc. Normally, this wouldn't be a problem, but if the function utf16s_to_utf8s() happens to return the number of bytes written to be precisely PAGE_SIZE, then we would overrun the buffer and write the newline character outside the allotted space which can have undefined consequences or result in a failure.

To fix this buffer overrun, ensure that there always is enough space left for the newline character to be safely appended.

Fixes 6058989bad05 ("PCI: Export ACPI _DSM provided firmware instance number and string name to sysfs")
Reported-by Joe Perches <[email protected]>
Signed-off-by Krzysztof Wilczyński <[email protected]>
---
 drivers/pci/pci-label.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pci/pci-label.c +2 -2
index 000e169c7197..0c6446519640 100644
@@ -146,8 +146,8 @@ static int dsm_label_utf16s_to_utf8s(union acpi_object *obj, char *buf)
 	len = utf16s_to_utf8s((const wchar_t *)obj->buffer.pointer,
 			      obj->buffer.length,
 			      UTF16_LITTLE_ENDIAN,
-			      buf, PAGE_SIZE);
-	buf[len] = '\n';
+			      buf, PAGE_SIZE - 1);
+	buf[len++] = '\n';
 
 	return len;
 }
Signature · 1 line
2.31.1
Applied
#16 lore raw
In reply to Krzysztof Wilczyński
Quoted 82 lines · Krzysztof Wilczyński

Hello,

This series aims to bring support for the sysfs_emit() and sysfs_emit_at() functions to the existing PCI-related sysfs objects. These new functions were introduced to make it less ambiguous which function is preferred when writing to the output buffer in a device attribute's "show" callback [1].

Thus, the existing PCI sysfs objects "show" functions will be converted from the using the sprintf(), snprintf() and scnprintf() functions to sysfs_emit() and sysfs_emit_at() accordingly, as the latter is aware of the PAGE_SIZE buffer limit that the sysfs object has and correctly returns the number of bytes written into the buffer.

This series will also address inconsistency related to the presence (or lack of thereof) of a trailing newline in the show() functions adding it where it's currently missing. This will allow for utilities such as the "cat" command to display the result of the read from a particular sysfs object correctly in a shell.

While working on this series a problem with newline handling related to how the value of the "resource_alignment" sysfs object was parsed and then persisted has been found and corrected. Also, while at it, a change enabling support for the value of "resource_alignment" previously set using either a command-line argument or through the sysfs object to be cleared at run-time was also included, and thus aligning this particular sysfs object with the behaviour of other such objects that allow for the value to be dynamically updated and cleared as required.

Additionally, a fix to a potential buffer overrun that has been found in the dsm_label_utf16s_to_utf8s() function that is responsible for the character conversion from UTF16 to UTF8 of the buffer that holds the device label obtained through the ACPI _DSM mechanism is included as part of this series.

Finally, a minor fix is also included in this series that has been added to ensure that the value of the "driver_override" variable is only exposed through the corresponding sysfs object when a value is set or otherwise if the value has not been set, the object would return a string representation of the NULL value. This will also align this particular sysfs object's behaviour with others, where when there is no value then nothing is returned.

[1] Documentation/filesystems/sysfs.rst

This series is related to:
commit ad025f8 ("PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions")

Signed-off-by Krzysztof Wilczyński <[email protected]>

---
Changes in v2:
None.

Changes in v3:
Added Logan Gunthorpe's "Reviewed-by".

Changes in v4:
Separated and squashed all the trivial sysfs_emit()/sysfs_emit_at() changes into a single patch as per Bjorn Helgaas' request. Carried Logan Gunthorpe's "Reviewed-by" over.

Changes in v5:
Added check to the resource_alignment_show() function to ensure that there is an extra space left in the buffer for the newline character, assuming that it might be provided.

Changes in v6:
Added a cover letter as per Bjorn Helgaas' request. New patch addressing a potential buffer overrun in the dsm_label_utf16s_to_utf8s() function has been added.

Krzysztof Wilczyński (6):
PCI/sysfs: Use sysfs_emit() and sysfs_emit_at() in "show" functions PCI/sysfs: Use return value from dsm_label_utf16s_to_utf8s() directly PCI/sysfs: Fix trailing newline handling of resource_alignment_param PCI/sysfs: Add missing trailing newline to devspec_show() PCI/sysfs: Only show value when driver_override is not NULL PCI/sysfs: Fix a buffer overrun problem with

    dsm_label_utf16s_to_utf8s()
Quoted 14 lines
 drivers/pci/hotplug/pci_hotplug_core.c |  8 +++---
 drivers/pci/hotplug/rpadlpar_sysfs.c   |  4 +--
 drivers/pci/hotplug/shpchp_sysfs.c     | 38 ++++++++++++++------------
 drivers/pci/iov.c                      | 12 ++++----
 drivers/pci/msi.c                      |  8 +++---
 drivers/pci/p2pdma.c                   |  7 ++---
 drivers/pci/pci-label.c                | 22 ++++++++-------
 drivers/pci/pci-sysfs.c                |  7 +++--
 drivers/pci/pci.c                      | 34 +++++++++++++----------
 drivers/pci/pcie/aer.c                 | 20 ++++++++------
 drivers/pci/pcie/aspm.c                |  4 +--
 drivers/pci/slot.c                     | 18 ++++++------
 drivers/pci/switch/switchtec.c         | 18 ++++++------
 13 files changed, 107 insertions(+), 93 deletions(-)

I applied these to pci/resource for v5.14, thanks!

I dropped the driver_override change to avoid breaking driverctl.

I reordered the dsm_label_utf16s_to_utf8s()-related patches to make the buffer overflow fix slightly smaller.

If we need to tweak the resource_alignment_param patch, I can just squash in incremental changes.