| Message ID | 20240822134744.44919-1-pstanner@redhat.com |
|---|---|
| Headers | show |
| Series | PCI: Remove pcim_iounmap_regions() | expand |
Le 22/08/2024 à 15:47, Philipp Stanner a écrit : > In psnet_open_pf_bar() and snet_open_vf_bar() a string later passed to > pcim_iomap_regions() is placed on the stack. Neither > pcim_iomap_regions() nor the functions it calls copy that string. > > Should the string later ever be used, this, consequently, causes > undefined behavior since the stack frame will by then have disappeared. > > Fix the bug by allocating the strings on the heap through > devm_kasprintf(). > > Cc: stable@vger.kernel.org # v6.3 > Fixes: 51a8f9d7f587 ("virtio: vdpa: new SolidNET DPU driver.") > Reported-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > Closes: https://lore.kernel.org/all/74e9109a-ac59-49e2-9b1d-d825c9c9f891@wanadoo.fr/ > Suggested-by: Andy Shevchenko <andy@kernel.org> > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > --- > drivers/vdpa/solidrun/snet_main.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/vdpa/solidrun/snet_main.c b/drivers/vdpa/solidrun/snet_main.c > index 99428a04068d..67235f6190ef 100644 > --- a/drivers/vdpa/solidrun/snet_main.c > +++ b/drivers/vdpa/solidrun/snet_main.c > @@ -555,7 +555,7 @@ static const struct vdpa_config_ops snet_config_ops = { > > static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet) > { > - char name[50]; > + char *name; > int ret, i, mask = 0; > /* We don't know which BAR will be used to communicate.. > * We will map every bar with len > 0. > @@ -573,7 +573,10 @@ static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet) > return -ENODEV; > } > > - snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev)); > + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-bars", pci_name(pdev)); > + if (!name) > + return -ENOMEM; > + > ret = pcim_iomap_regions(pdev, mask, name); > if (ret) { > SNET_ERR(pdev, "Failed to request and map PCI BARs\n"); > @@ -590,10 +593,12 @@ static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet) > > static int snet_open_vf_bar(struct pci_dev *pdev, struct snet *snet) > { > - char name[50]; > + char *name; > int ret; > > - snprintf(name, sizeof(name), "snet[%s]-bar", pci_name(pdev)); > + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-bars", pci_name(pdev)); s/psnet/snet/ > + if (!name) > + return -ENOMEM; > /* Request and map BAR */ > ret = pcim_iomap_regions(pdev, BIT(snet->psnet->cfg.vf_bar), name); > if (ret) {
On Thu, Aug 22, 2024 at 03:47:39PM +0200, Philipp Stanner wrote: > In psnet_open_pf_bar() and snet_open_vf_bar() a string later passed to > pcim_iomap_regions() is placed on the stack. Neither > pcim_iomap_regions() nor the functions it calls copy that string. > > Should the string later ever be used, this, consequently, causes > undefined behavior since the stack frame will by then have disappeared. > > Fix the bug by allocating the strings on the heap through > devm_kasprintf(). ... > - snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev)); > + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-bars", pci_name(pdev)); > + if (!name) > + return -ENOMEM; > + > ret = pcim_iomap_regions(pdev, mask, name); ... > - snprintf(name, sizeof(name), "snet[%s]-bar", pci_name(pdev)); > + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-bars", pci_name(pdev)); > + if (!name) > + return -ENOMEM; + Blank line as in the above snippet? > /* Request and map BAR */ > ret = pcim_iomap_regions(pdev, BIT(snet->psnet->cfg.vf_bar), name);
On Thu, Aug 22, 2024 at 03:47:32PM +0200, Philipp Stanner wrote: > Important things first: > This series is based on [1] and [2] which Bjorn Helgaas has currently > queued for v6.12 in the PCI tree. > > This series shall remove pcim_iounmap_regions() in order to make way to > remove its brother, pcim_iomap_regions(). For the non-commented ones (by me or by others) Reviewed-by: Andy Shevchenko <andy@kernel.org>
On Thu, 2024-08-22 at 16:34 +0200, Christophe JAILLET wrote: > Le 22/08/2024 à 15:47, Philipp Stanner a écrit : > > In psnet_open_pf_bar() and snet_open_vf_bar() a string later passed > > to > > pcim_iomap_regions() is placed on the stack. Neither > > pcim_iomap_regions() nor the functions it calls copy that string. > > > > Should the string later ever be used, this, consequently, causes > > undefined behavior since the stack frame will by then have > > disappeared. > > > > Fix the bug by allocating the strings on the heap through > > devm_kasprintf(). > > > > Cc: stable@vger.kernel.org # v6.3 > > Fixes: 51a8f9d7f587 ("virtio: vdpa: new SolidNET DPU driver.") > > Reported-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > Closes: > > https://lore.kernel.org/all/74e9109a-ac59-49e2-9b1d-d825c9c9f891@wanadoo.fr/ > > Suggested-by: Andy Shevchenko <andy@kernel.org> > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > --- > > drivers/vdpa/solidrun/snet_main.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/vdpa/solidrun/snet_main.c > > b/drivers/vdpa/solidrun/snet_main.c > > index 99428a04068d..67235f6190ef 100644 > > --- a/drivers/vdpa/solidrun/snet_main.c > > +++ b/drivers/vdpa/solidrun/snet_main.c > > @@ -555,7 +555,7 @@ static const struct vdpa_config_ops > > snet_config_ops = { > > > > static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet > > *psnet) > > { > > - char name[50]; > > + char *name; > > int ret, i, mask = 0; > > /* We don't know which BAR will be used to communicate.. > > * We will map every bar with len > 0. > > @@ -573,7 +573,10 @@ static int psnet_open_pf_bar(struct pci_dev > > *pdev, struct psnet *psnet) > > return -ENODEV; > > } > > > > - snprintf(name, sizeof(name), "psnet[%s]-bars", > > pci_name(pdev)); > > + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]- > > bars", pci_name(pdev)); > > + if (!name) > > + return -ENOMEM; > > + > > ret = pcim_iomap_regions(pdev, mask, name); > > if (ret) { > > SNET_ERR(pdev, "Failed to request and map PCI > > BARs\n"); > > @@ -590,10 +593,12 @@ static int psnet_open_pf_bar(struct pci_dev > > *pdev, struct psnet *psnet) > > > > static int snet_open_vf_bar(struct pci_dev *pdev, struct snet > > *snet) > > { > > - char name[50]; > > + char *name; > > int ret; > > > > - snprintf(name, sizeof(name), "snet[%s]-bar", > > pci_name(pdev)); > > + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]- > > bars", pci_name(pdev)); > > s/psnet/snet/ sharp eyes ;) Thx, P. > > > + if (!name) > > + return -ENOMEM; > > /* Request and map BAR */ > > ret = pcim_iomap_regions(pdev, BIT(snet->psnet- > > >cfg.vf_bar), name); > > if (ret) { >
On Thu, 2024-08-22 at 17:44 +0300, Andy Shevchenko wrote: > On Thu, Aug 22, 2024 at 03:47:37PM +0200, Philipp Stanner wrote: > > pcim_iomap_regions() and pcim_iomap_table() have been deprecated by > > the PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate > > pcim_iomap_table(), pcim_iomap_regions_request_all()"). > > > > Replace these functions with the function pcim_iomap_region(). > > ... > > > - err = pcim_iomap_regions(pdev, 1 << PCI_PTP_BAR_NO, > > pci_name(pdev)); > > - if (err) > > + clock->reg_base = pcim_iomap_region(pdev, PCI_PTP_BAR_NO, > > pci_name(pdev)); > > + if (IS_ERR(clock->reg_base)) { > > + err = PTR_ERR(clock->reg_base); > > goto error_free; > > - > > - clock->reg_base = pcim_iomap_table(pdev)[PCI_PTP_BAR_NO]; > > + } > > Perhaps > > clock->reg_base = pcim_iomap_region(pdev, PCI_PTP_BAR_NO, > pci_name(pdev)); > err = PTR_ERR_OR_ZERO(clock->reg_base); > if (err) > goto error_free; > > This will make your patch smaller and neater. > > P.S. Do you use --histogram diff algo when preparing patches? So far not. Should one do that? P. >
On Mon, Aug 26, 2024 at 5:51 PM Philipp Stanner <pstanner@redhat.com> wrote: > On Thu, 2024-08-22 at 17:44 +0300, Andy Shevchenko wrote: > > On Thu, Aug 22, 2024 at 03:47:37PM +0200, Philipp Stanner wrote: ... > > > - err = pcim_iomap_regions(pdev, 1 << PCI_PTP_BAR_NO, > > > pci_name(pdev)); > > > - if (err) > > > + clock->reg_base = pcim_iomap_region(pdev, PCI_PTP_BAR_NO, > > > pci_name(pdev)); > > > + if (IS_ERR(clock->reg_base)) { > > > + err = PTR_ERR(clock->reg_base); > > > goto error_free; > > > - > > > - clock->reg_base = pcim_iomap_table(pdev)[PCI_PTP_BAR_NO]; > > > + } > > > > Perhaps > > > > clock->reg_base = pcim_iomap_region(pdev, PCI_PTP_BAR_NO, > > pci_name(pdev)); > > err = PTR_ERR_OR_ZERO(clock->reg_base); > > if (err) > > goto error_free; > > > > This will make your patch smaller and neater. > > > > P.S. Do you use --histogram diff algo when preparing patches? > > So far not. > Should one do that? Id doesn't alter your code, it's in addition to what I suggested, but as Linus shared that there is no reason to avoid using --histogram not only in Linux kernel, but in general as it produces more human-readable diff:s.
On Mon, 2024-08-26 at 18:41 +0300, Andy Shevchenko wrote: > On Mon, Aug 26, 2024 at 5:51 PM Philipp Stanner <pstanner@redhat.com> > wrote: > > On Thu, 2024-08-22 at 17:44 +0300, Andy Shevchenko wrote: > > > On Thu, Aug 22, 2024 at 03:47:37PM +0200, Philipp Stanner wrote: > > ... > > > > > - err = pcim_iomap_regions(pdev, 1 << PCI_PTP_BAR_NO, > > > > pci_name(pdev)); > > > > - if (err) > > > > + clock->reg_base = pcim_iomap_region(pdev, PCI_PTP_BAR_NO, > > > > pci_name(pdev)); > > > > + if (IS_ERR(clock->reg_base)) { > > > > + err = PTR_ERR(clock->reg_base); > > > > goto error_free; > > > > - > > > > - clock->reg_base = pcim_iomap_table(pdev)[PCI_PTP_BAR_NO]; > > > > + } > > > > > > Perhaps > > > > > > clock->reg_base = pcim_iomap_region(pdev, PCI_PTP_BAR_NO, > > > pci_name(pdev)); > > > err = PTR_ERR_OR_ZERO(clock->reg_base); > > > if (err) > > > goto error_free; > > > > > > This will make your patch smaller and neater. > > > > > > P.S. Do you use --histogram diff algo when preparing patches? > > > > So far not. > > Should one do that? > > Id doesn't alter your code, it's in addition to what I suggested, but > as Linus shared that there is no reason to avoid using --histogram > not > only in Linux kernel, but in general as it produces more > human-readable diff:s. If the Boss says so, one can surely do that \o/ Though if it has 0 disadvantages I'd propose proposing to the git-devs to make it the default. P. >
On Mon, Aug 26, 2024 at 6:52 PM Philipp Stanner <pstanner@redhat.com> wrote: > On Mon, 2024-08-26 at 18:41 +0300, Andy Shevchenko wrote: ... > Though if it has 0 disadvantages I'd propose proposing to the git-devs > to make it the default. It's slower. so the people from https://occ.deadnet.se/about/ won't be happy. And more power consuming, so maybe not so environment friendly after all :-P
Changes in v3: - fpga/dfl-pci.c: remove now surplus wrapper around pcim_iomap_region(). (Andy) - block: mtip32xx: remove now surplus label. (Andy) - vdpa: solidrun: Bugfix: Include forgotten place where stack UB occurs. (Andy, Christophe) - Some minor wording improvements in commit messages. (Me) Changes in v2: - Add a fix for the UB stack usage bug in vdap/solidrun. Separate patch, put stable kernel on CC. (Christophe, Andy). - Drop unnecessary pcim_release_region() in mtip32xx (Andy) - Consequently, drop patch "PCI: Make pcim_release_region() a public function", since there's no user anymore. (obsoletes the squash requested by Damien). - vdap/solidrun: • make 'i' an 'unsigned short' (Andy, me) • Use 'continue' to simplify loop (Andy) • Remove leftover blank line - Apply given Reviewed- / acked-bys (Andy, Damien, Bartosz) Important things first: This series is based on [1] and [2] which Bjorn Helgaas has currently queued for v6.12 in the PCI tree. This series shall remove pcim_iounmap_regions() in order to make way to remove its brother, pcim_iomap_regions(). @Bjorn: Feel free to squash the PCI commits. Regards, P. [1] https://lore.kernel.org/all/20240729093625.17561-4-pstanner@redhat.com/ [2] https://lore.kernel.org/all/20240807083018.8734-2-pstanner@redhat.com/ Philipp Stanner (9): PCI: Make pcim_iounmap_region() a public function fpga/dfl-pci.c: Replace deprecated PCI functions block: mtip32xx: Replace deprecated PCI functions gpio: Replace deprecated PCI functions ethernet: cavium: Replace deprecated PCI functions ethernet: stmicro: Simplify PCI devres usage vdpa: solidrun: Fix UB bug with devres vdap: solidrun: Replace deprecated PCI functions PCI: Remove pcim_iounmap_regions() .../driver-api/driver-model/devres.rst | 1 - drivers/block/mtip32xx/mtip32xx.c | 16 +++--- drivers/fpga/dfl-pci.c | 16 ++---- drivers/gpio/gpio-merrifield.c | 14 ++--- .../net/ethernet/cavium/common/cavium_ptp.c | 10 ++-- .../ethernet/stmicro/stmmac/dwmac-loongson.c | 25 ++------ .../net/ethernet/stmicro/stmmac/stmmac_pci.c | 18 ++---- drivers/pci/devres.c | 24 +------- drivers/vdpa/solidrun/snet_main.c | 57 ++++++++----------- include/linux/pci.h | 2 +- 10 files changed, 61 insertions(+), 122 deletions(-)