| Message ID | 1626662666-15798-1-git-send-email-hayashi.kunihiko@socionext.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: endpoint: Use sysfs_emit() in "show" functions | expand |
Hello Hayashi-san, Thank you for sending the patch over! > Convert sprintf() in sysfs "show" functions to sysfs_emit() in order to > check for buffer overruns in sysfs outputs. Nice catch! A small nitpick: what you are changing here are technically not sysfs objects since all of these are related to configfs. Having said that, configfs shares the same semantics for normal attributes with sysfs, so a maximum size of PAGE_SIZE applies here too, and thus sysfs_emit() would work fine. Thank you for taking care of this! Reviewed-by: Krzysztof Wilczyński <kw@linux.com> Krzysztof
Hi Krzysztof, Thank you for reviewing. On 2021/07/19 12:43, Krzysztof Wilczyński wrote: > Hello Hayashi-san, > > Thank you for sending the patch over! > >> Convert sprintf() in sysfs "show" functions to sysfs_emit() in order to >> check for buffer overruns in sysfs outputs. > > Nice catch! I actually executed "cat" against configfs to meet the issue and found your solution in pci-sysfs. > > A small nitpick: what you are changing here are technically not sysfs > objects since all of these are related to configfs. Having said that, > configfs shares the same semantics for normal attributes with sysfs, so > a maximum size of PAGE_SIZE applies here too, and thus sysfs_emit() > would work fine. Thank you for helpful information. I understand that applying sysfs_emit() to configfs is no problem. > > Thank you for taking care of this! > > Reviewed-by: Krzysztof Wilczyński <kw@linux.com> > > Krzysztof > Thank you, --- Best Regards Kunihiko Hayashi
[+cc Sasha for visibility] Hi! [...] > > Nice catch! > > I actually executed "cat" against configfs to meet the issue and found > your solution in pci-sysfs. Oh! That's not good... I am curious, which attribute caused this? Also, if this is fixing a bug, then it might warrant letting the folks who look after the long-term and stable kernels know. I also wonder if there would be something to add for the "Fixes:" tag, if there is a previous commit this change fixes. Krzysztof
Hi Krzysztof, On 2021/07/20 0:18, Krzysztof Wilczy?ski wrote: > [+cc Sasha for visibility] > > Hi! > > [...] >>> Nice catch! >> >> I actually executed "cat" against configfs to meet the issue and found >> your solution in pci-sysfs. > > Oh! That's not good... I am curious, which attribute caused this? Sorry I misunderstood. I found this "cat" issue on next-20210713 and all configfs attribues had the same issue. However, my patch wasn't the solution for the issue. This has been fixed by 7fe1e79b59ba ("configfs: implement the .read_iter and .write_iter methods"). The function replacement was found in the process of finding the issue. > Also, if this is fixing a bug, then it might warrant letting the folks who look > after the long-term and stable kernels know. I also wonder if there would be > something to add for the "Fixes:" tag, if there is a previous commit this > change fixes. So my patch doesn't fix any issues. Thank you, --- Best Regards Kunihiko Hayashi
diff --git a/drivers/pci/endpoint/functions/pci-epf-ntb.c b/drivers/pci/endpoint/functions/pci-epf-ntb.c index bce274d..f5dfc63 100644 --- a/drivers/pci/endpoint/functions/pci-epf-ntb.c +++ b/drivers/pci/endpoint/functions/pci-epf-ntb.c @@ -1918,7 +1918,7 @@ static ssize_t epf_ntb_##_name##_show(struct config_item *item, \ struct config_group *group = to_config_group(item); \ struct epf_ntb *ntb = to_epf_ntb(group); \ \ - return sprintf(page, "%d\n", ntb->_name); \ + return sysfs_emit(page, "%d\n", ntb->_name); \ } #define EPF_NTB_W(_name) \ @@ -1949,7 +1949,7 @@ static ssize_t epf_ntb_##_name##_show(struct config_item *item, \ \ sscanf(#_name, "mw%d", &win_no); \ \ - return sprintf(page, "%lld\n", ntb->mws_size[win_no - 1]); \ + return sysfs_emit(page, "%lld\n", ntb->mws_size[win_no - 1]); \ } #define EPF_NTB_MW_W(_name) \ diff --git a/drivers/pci/endpoint/pci-ep-cfs.c b/drivers/pci/endpoint/pci-ep-cfs.c index f3a8b83..b40a42f 100644 --- a/drivers/pci/endpoint/pci-ep-cfs.c +++ b/drivers/pci/endpoint/pci-ep-cfs.c @@ -198,8 +198,7 @@ static ssize_t pci_epc_start_store(struct config_item *item, const char *page, static ssize_t pci_epc_start_show(struct config_item *item, char *page) { - return sprintf(page, "%d\n", - to_pci_epc_group(item)->start); + return sysfs_emit(page, "%d\n", to_pci_epc_group(item)->start); } CONFIGFS_ATTR(pci_epc_, start); @@ -321,7 +320,7 @@ static ssize_t pci_epf_##_name##_show(struct config_item *item, char *page) \ struct pci_epf *epf = to_pci_epf_group(item)->epf; \ if (WARN_ON_ONCE(!epf->header)) \ return -EINVAL; \ - return sprintf(page, "0x%04x\n", epf->header->_name); \ + return sysfs_emit(page, "0x%04x\n", epf->header->_name); \ } #define PCI_EPF_HEADER_W_u32(_name) \ @@ -390,8 +389,8 @@ static ssize_t pci_epf_msi_interrupts_store(struct config_item *item, static ssize_t pci_epf_msi_interrupts_show(struct config_item *item, char *page) { - return sprintf(page, "%d\n", - to_pci_epf_group(item)->epf->msi_interrupts); + return sysfs_emit(page, "%d\n", + to_pci_epf_group(item)->epf->msi_interrupts); } static ssize_t pci_epf_msix_interrupts_store(struct config_item *item, @@ -412,8 +411,8 @@ static ssize_t pci_epf_msix_interrupts_store(struct config_item *item, static ssize_t pci_epf_msix_interrupts_show(struct config_item *item, char *page) { - return sprintf(page, "%d\n", - to_pci_epf_group(item)->epf->msix_interrupts); + return sysfs_emit(page, "%d\n", + to_pci_epf_group(item)->epf->msix_interrupts); } PCI_EPF_HEADER_R(vendorid)
Convert sprintf() in sysfs "show" functions to sysfs_emit() in order to check for buffer overruns in sysfs outputs. Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> --- drivers/pci/endpoint/functions/pci-epf-ntb.c | 4 ++-- drivers/pci/endpoint/pci-ep-cfs.c | 13 ++++++------- 2 files changed, 8 insertions(+), 9 deletions(-)