| Message ID | 20240609-pcie-kirin-memleak-v1-1-62b45b879576@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: kirin: fix memory leak in kirin_pcie_parse_port() | expand |
On Sun, Jun 09, 2024 at 12:56:14PM +0200, Javier Carrasco wrote: > The conversion of this file to use the agnostic GPIO API has introduced > a new early return where the refcounts of two device nodes (parent and > child) are not decremented. > > Given that the device nodes are not required outside the loops where > they are used, and to avoid potential bugs every time a new error path > is introduced to the loop, the _scoped() versions of the macros have > been applied. The bug was introduced recently, and the fix is not > relevant for old stable kernels that might not support the scoped() > variant. Looks reasonable to me, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Thanks!
On Sun, Jun 09, 2024 at 12:56:14PM +0200, Javier Carrasco wrote: > The conversion of this file to use the agnostic GPIO API has introduced > a new early return where the refcounts of two device nodes (parent and > child) are not decremented. > > Given that the device nodes are not required outside the loops where > they are used, and to avoid potential bugs every time a new error path > is introduced to the loop, the _scoped() versions of the macros have > been applied. The bug was introduced recently, and the fix is not > relevant for old stable kernels that might not support the scoped() > variant. > > Fixes: 1d38f9d89f85 ("PCI: kirin: Convert to use agnostic GPIO API") > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > This bug was found while analyzing the code and I don't have hardware to > validate it beyond compilation and static analysis. Any test with real > hardware to make sure there are no regressions is always welcome. > > The dev_err() messages have not been converted into dev_err_probe() to > keep the current format, but I am open to convert them if preferred. Sure, please do it in a separate patch. - Mani > --- > drivers/pci/controller/dwc/pcie-kirin.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c > index d1f54f188e71..0a29136491b8 100644 > --- a/drivers/pci/controller/dwc/pcie-kirin.c > +++ b/drivers/pci/controller/dwc/pcie-kirin.c > @@ -403,11 +403,10 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > struct device_node *node) > { > struct device *dev = &pdev->dev; > - struct device_node *parent, *child; > int ret, slot, i; > > - for_each_available_child_of_node(node, parent) { > - for_each_available_child_of_node(parent, child) { > + for_each_available_child_of_node_scoped(node, parent) { > + for_each_available_child_of_node_scoped(parent, child) { > i = pcie->num_slots; > > pcie->id_reset_gpio[i] = devm_fwnode_gpiod_get_index(dev, > @@ -424,14 +423,13 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > pcie->num_slots++; > if (pcie->num_slots > MAX_PCI_SLOTS) { > dev_err(dev, "Too many PCI slots!\n"); > - ret = -EINVAL; > - goto put_node; > + return -EINVAL; > } > > ret = of_pci_get_devfn(child); > if (ret < 0) { > dev_err(dev, "failed to parse devfn: %d\n", ret); > - goto put_node; > + return ret; > } > > slot = PCI_SLOT(ret); > @@ -439,10 +437,8 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > pcie->reset_names[i] = devm_kasprintf(dev, GFP_KERNEL, > "pcie_perst_%d", > slot); > - if (!pcie->reset_names[i]) { > - ret = -ENOMEM; > - goto put_node; > - } > + if (!pcie->reset_names[i]) > + return -ENOMEM; > > gpiod_set_consumer_name(pcie->id_reset_gpio[i], > pcie->reset_names[i]); > @@ -450,11 +446,6 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > } > > return 0; > - > -put_node: > - of_node_put(child); > - of_node_put(parent); > - return ret; > } > > static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, > > --- > base-commit: d35b2284e966c0bef3e2182a5c5ea02177dd32e4 > change-id: 20240609-pcie-kirin-memleak-18c83a31d111 > > Best regards, > -- > Javier Carrasco <javier.carrasco.cruz@gmail.com> >
On 09/06/2024 12:56, Javier Carrasco wrote: > The conversion of this file to use the agnostic GPIO API has introduced > a new early return where the refcounts of two device nodes (parent and > child) are not decremented. > > Given that the device nodes are not required outside the loops where > they are used, and to avoid potential bugs every time a new error path > is introduced to the loop, the _scoped() versions of the macros have > been applied. The bug was introduced recently, and the fix is not > relevant for old stable kernels that might not support the scoped() > variant. > > Fixes: 1d38f9d89f85 ("PCI: kirin: Convert to use agnostic GPIO API") > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > --- > This bug was found while analyzing the code and I don't have hardware to > validate it beyond compilation and static analysis. Any test with real > hardware to make sure there are no regressions is always welcome. > > The dev_err() messages have not been converted into dev_err_probe() to > keep the current format, but I am open to convert them if preferred. > --- > drivers/pci/controller/dwc/pcie-kirin.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c > index d1f54f188e71..0a29136491b8 100644 > --- a/drivers/pci/controller/dwc/pcie-kirin.c > +++ b/drivers/pci/controller/dwc/pcie-kirin.c > @@ -403,11 +403,10 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > struct device_node *node) > { > struct device *dev = &pdev->dev; > - struct device_node *parent, *child; > int ret, slot, i; > > - for_each_available_child_of_node(node, parent) { > - for_each_available_child_of_node(parent, child) { > + for_each_available_child_of_node_scoped(node, parent) { > + for_each_available_child_of_node_scoped(parent, child) { > i = pcie->num_slots; > > pcie->id_reset_gpio[i] = devm_fwnode_gpiod_get_index(dev, > @@ -424,14 +423,13 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > pcie->num_slots++; > if (pcie->num_slots > MAX_PCI_SLOTS) { > dev_err(dev, "Too many PCI slots!\n"); > - ret = -EINVAL; > - goto put_node; > + return -EINVAL; > } > > ret = of_pci_get_devfn(child); > if (ret < 0) { > dev_err(dev, "failed to parse devfn: %d\n", ret); > - goto put_node; > + return ret; > } > > slot = PCI_SLOT(ret); > @@ -439,10 +437,8 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > pcie->reset_names[i] = devm_kasprintf(dev, GFP_KERNEL, > "pcie_perst_%d", > slot); > - if (!pcie->reset_names[i]) { > - ret = -ENOMEM; > - goto put_node; > - } > + if (!pcie->reset_names[i]) > + return -ENOMEM; > > gpiod_set_consumer_name(pcie->id_reset_gpio[i], > pcie->reset_names[i]); > @@ -450,11 +446,6 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > } > > return 0; > - > -put_node: > - of_node_put(child); > - of_node_put(parent); > - return ret; > } > > static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, > > --- > base-commit: d35b2284e966c0bef3e2182a5c5ea02177dd32e4 > change-id: 20240609-pcie-kirin-memleak-18c83a31d111 > > Best regards, Hi, has this patch been applied anywhere? I couldn't find it in linux-next and pci/next. Best regards, Javier Carrasco
Hello, [...] > Hi, has this patch been applied anywhere? I couldn't find it in > linux-next and pci/next. No, not yet. It's on my TODO list. You can always have a peek at our Patchwork to see how are things progressing. Meanwhile, there are always patches to be reviewed, if you feel particularly restless, so feel free. Thank you! Krzysztof
On Sun, 09 Jun 2024 12:56:14 +0200 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > The conversion of this file to use the agnostic GPIO API has introduced > a new early return where the refcounts of two device nodes (parent and > child) are not decremented. > > Given that the device nodes are not required outside the loops where > they are used, and to avoid potential bugs every time a new error path > is introduced to the loop, the _scoped() versions of the macros have > been applied. The bug was introduced recently, and the fix is not > relevant for old stable kernels that might not support the scoped() > variant. > > Fixes: 1d38f9d89f85 ("PCI: kirin: Convert to use agnostic GPIO API") > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> Diff on this on is irritating as it doesn't actually show the buggy code... Ah well. Change is valid, but one suggestion inline. Looks like it's queued now already, but if not. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > This bug was found while analyzing the code and I don't have hardware to > validate it beyond compilation and static analysis. Any test with real > hardware to make sure there are no regressions is always welcome. > > The dev_err() messages have not been converted into dev_err_probe() to > keep the current format, but I am open to convert them if preferred. > --- > drivers/pci/controller/dwc/pcie-kirin.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c > index d1f54f188e71..0a29136491b8 100644 > --- a/drivers/pci/controller/dwc/pcie-kirin.c > +++ b/drivers/pci/controller/dwc/pcie-kirin.c > @@ -403,11 +403,10 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > struct device_node *node) > { > struct device *dev = &pdev->dev; > - struct device_node *parent, *child; > int ret, slot, i; > > - for_each_available_child_of_node(node, parent) { > - for_each_available_child_of_node(parent, child) { > + for_each_available_child_of_node_scoped(node, parent) { > + for_each_available_child_of_node_scoped(parent, child) { > i = pcie->num_slots; > > pcie->id_reset_gpio[i] = devm_fwnode_gpiod_get_index(dev, > @@ -424,14 +423,13 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > pcie->num_slots++; > if (pcie->num_slots > MAX_PCI_SLOTS) { > dev_err(dev, "Too many PCI slots!\n"); > - ret = -EINVAL; > - goto put_node; > + return -EINVAL; Perhaps a future change, but this would be nicer as return dev_err_probe(dev, -EINVAL, "Too many PCI slots!\n"); Maybe as part of a general change to this driver to use dev_err_probe() for all the error prints in paths only called from probe(). > } > > ret = of_pci_get_devfn(child); > if (ret < 0) { > dev_err(dev, "failed to parse devfn: %d\n", ret); > - goto put_node; > + return ret; > } > > slot = PCI_SLOT(ret); > @@ -439,10 +437,8 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > pcie->reset_names[i] = devm_kasprintf(dev, GFP_KERNEL, > "pcie_perst_%d", > slot); > - if (!pcie->reset_names[i]) { > - ret = -ENOMEM; > - goto put_node; > - } > + if (!pcie->reset_names[i]) > + return -ENOMEM; > > gpiod_set_consumer_name(pcie->id_reset_gpio[i], > pcie->reset_names[i]); > @@ -450,11 +446,6 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > } > > return 0; > - > -put_node: > - of_node_put(child); > - of_node_put(parent); > - return ret; > } > > static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, > > --- > base-commit: d35b2284e966c0bef3e2182a5c5ea02177dd32e4 > change-id: 20240609-pcie-kirin-memleak-18c83a31d111 > > Best regards,
On 05/07/2024 12:18, Jonathan Cameron wrote: > On Sun, 09 Jun 2024 12:56:14 +0200 > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > >> The conversion of this file to use the agnostic GPIO API has introduced >> a new early return where the refcounts of two device nodes (parent and >> child) are not decremented. >> >> Given that the device nodes are not required outside the loops where >> they are used, and to avoid potential bugs every time a new error path >> is introduced to the loop, the _scoped() versions of the macros have >> been applied. The bug was introduced recently, and the fix is not >> relevant for old stable kernels that might not support the scoped() >> variant. >> >> Fixes: 1d38f9d89f85 ("PCI: kirin: Convert to use agnostic GPIO API") >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > Diff on this on is irritating as it doesn't actually show the > buggy code... Ah well. > > Change is valid, but one suggestion inline. > > Looks like it's queued now already, but if not. > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > >> --- >> This bug was found while analyzing the code and I don't have hardware to >> validate it beyond compilation and static analysis. Any test with real >> hardware to make sure there are no regressions is always welcome. >> >> The dev_err() messages have not been converted into dev_err_probe() to >> keep the current format, but I am open to convert them if preferred. >> --- >> drivers/pci/controller/dwc/pcie-kirin.c | 21 ++++++--------------- >> 1 file changed, 6 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c >> index d1f54f188e71..0a29136491b8 100644 >> --- a/drivers/pci/controller/dwc/pcie-kirin.c >> +++ b/drivers/pci/controller/dwc/pcie-kirin.c >> @@ -403,11 +403,10 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, >> struct device_node *node) >> { >> struct device *dev = &pdev->dev; >> - struct device_node *parent, *child; >> int ret, slot, i; >> >> - for_each_available_child_of_node(node, parent) { >> - for_each_available_child_of_node(parent, child) { >> + for_each_available_child_of_node_scoped(node, parent) { >> + for_each_available_child_of_node_scoped(parent, child) { >> i = pcie->num_slots; >> >> pcie->id_reset_gpio[i] = devm_fwnode_gpiod_get_index(dev, >> @@ -424,14 +423,13 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, >> pcie->num_slots++; >> if (pcie->num_slots > MAX_PCI_SLOTS) { >> dev_err(dev, "Too many PCI slots!\n"); >> - ret = -EINVAL; >> - goto put_node; >> + return -EINVAL; > Perhaps a future change, but this would be nicer as > return dev_err_probe(dev, -EINVAL, > "Too many PCI slots!\n"); > Maybe as part of a general change to this driver to use > dev_err_probe() for all the error prints in paths only called > from probe(). > Yeah, it seems that other paths that have nothing to do with this fix would require the same modification. Best regards, Javier Carrasco
> The conversion of this file to use the agnostic GPIO API has introduced > a new early return where the refcounts of two device nodes (parent and > child) are not decremented. > > Given that the device nodes are not required outside the loops where > they are used, and to avoid potential bugs every time a new error path > is introduced to the loop, the _scoped() versions of the macros have > been applied. The bug was introduced recently, and the fix is not > relevant for old stable kernels that might not support the scoped() > variant. Applied to controller/kirin, thank you! [1/1] PCI: kirin: Fix memory leak in kirin_pcie_parse_port() https://git.kernel.org/pci/pci/c/1a164371b362 Krzysztof
Hello, [...] > Looks like it's queued now already, but if not. > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> No worries. Updated the commit to add your review. Thank you! Krzysztof
On Sun, Jun 09, 2024 at 12:56:14PM +0200, Javier Carrasco wrote: > The conversion of this file to use the agnostic GPIO API has introduced > a new early return where the refcounts of two device nodes (parent and > child) are not decremented. > > Given that the device nodes are not required outside the loops where > they are used, and to avoid potential bugs every time a new error path > is introduced to the loop, the _scoped() versions of the macros have > been applied. The bug was introduced recently, and the fix is not > relevant for old stable kernels that might not support the scoped() > variant. > > Fixes: 1d38f9d89f85 ("PCI: kirin: Convert to use agnostic GPIO API") > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> FYI, I moved this to the pci/controller/gpio patch just before 1d38f9d89f85. That way there's no bisection hole and we don't need to complicate this commit log with details about the bug because the bug never exists. > --- > This bug was found while analyzing the code and I don't have hardware to > validate it beyond compilation and static analysis. Any test with real > hardware to make sure there are no regressions is always welcome. > > The dev_err() messages have not been converted into dev_err_probe() to > keep the current format, but I am open to convert them if preferred. > --- > drivers/pci/controller/dwc/pcie-kirin.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c > index d1f54f188e71..0a29136491b8 100644 > --- a/drivers/pci/controller/dwc/pcie-kirin.c > +++ b/drivers/pci/controller/dwc/pcie-kirin.c > @@ -403,11 +403,10 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > struct device_node *node) > { > struct device *dev = &pdev->dev; > - struct device_node *parent, *child; > int ret, slot, i; > > - for_each_available_child_of_node(node, parent) { > - for_each_available_child_of_node(parent, child) { > + for_each_available_child_of_node_scoped(node, parent) { > + for_each_available_child_of_node_scoped(parent, child) { > i = pcie->num_slots; > > pcie->id_reset_gpio[i] = devm_fwnode_gpiod_get_index(dev, > @@ -424,14 +423,13 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > pcie->num_slots++; > if (pcie->num_slots > MAX_PCI_SLOTS) { > dev_err(dev, "Too many PCI slots!\n"); > - ret = -EINVAL; > - goto put_node; > + return -EINVAL; > } > > ret = of_pci_get_devfn(child); > if (ret < 0) { > dev_err(dev, "failed to parse devfn: %d\n", ret); > - goto put_node; > + return ret; > } > > slot = PCI_SLOT(ret); > @@ -439,10 +437,8 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > pcie->reset_names[i] = devm_kasprintf(dev, GFP_KERNEL, > "pcie_perst_%d", > slot); > - if (!pcie->reset_names[i]) { > - ret = -ENOMEM; > - goto put_node; > - } > + if (!pcie->reset_names[i]) > + return -ENOMEM; > > gpiod_set_consumer_name(pcie->id_reset_gpio[i], > pcie->reset_names[i]); > @@ -450,11 +446,6 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, > } > > return 0; > - > -put_node: > - of_node_put(child); > - of_node_put(parent); > - return ret; > } > > static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, > > --- > base-commit: d35b2284e966c0bef3e2182a5c5ea02177dd32e4 > change-id: 20240609-pcie-kirin-memleak-18c83a31d111 > > Best regards, > -- > Javier Carrasco <javier.carrasco.cruz@gmail.com> >
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c index d1f54f188e71..0a29136491b8 100644 --- a/drivers/pci/controller/dwc/pcie-kirin.c +++ b/drivers/pci/controller/dwc/pcie-kirin.c @@ -403,11 +403,10 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, struct device_node *node) { struct device *dev = &pdev->dev; - struct device_node *parent, *child; int ret, slot, i; - for_each_available_child_of_node(node, parent) { - for_each_available_child_of_node(parent, child) { + for_each_available_child_of_node_scoped(node, parent) { + for_each_available_child_of_node_scoped(parent, child) { i = pcie->num_slots; pcie->id_reset_gpio[i] = devm_fwnode_gpiod_get_index(dev, @@ -424,14 +423,13 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, pcie->num_slots++; if (pcie->num_slots > MAX_PCI_SLOTS) { dev_err(dev, "Too many PCI slots!\n"); - ret = -EINVAL; - goto put_node; + return -EINVAL; } ret = of_pci_get_devfn(child); if (ret < 0) { dev_err(dev, "failed to parse devfn: %d\n", ret); - goto put_node; + return ret; } slot = PCI_SLOT(ret); @@ -439,10 +437,8 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, pcie->reset_names[i] = devm_kasprintf(dev, GFP_KERNEL, "pcie_perst_%d", slot); - if (!pcie->reset_names[i]) { - ret = -ENOMEM; - goto put_node; - } + if (!pcie->reset_names[i]) + return -ENOMEM; gpiod_set_consumer_name(pcie->id_reset_gpio[i], pcie->reset_names[i]); @@ -450,11 +446,6 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, } return 0; - -put_node: - of_node_put(child); - of_node_put(parent); - return ret; } static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
The conversion of this file to use the agnostic GPIO API has introduced a new early return where the refcounts of two device nodes (parent and child) are not decremented. Given that the device nodes are not required outside the loops where they are used, and to avoid potential bugs every time a new error path is introduced to the loop, the _scoped() versions of the macros have been applied. The bug was introduced recently, and the fix is not relevant for old stable kernels that might not support the scoped() variant. Fixes: 1d38f9d89f85 ("PCI: kirin: Convert to use agnostic GPIO API") Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- This bug was found while analyzing the code and I don't have hardware to validate it beyond compilation and static analysis. Any test with real hardware to make sure there are no regressions is always welcome. The dev_err() messages have not been converted into dev_err_probe() to keep the current format, but I am open to convert them if preferred. --- drivers/pci/controller/dwc/pcie-kirin.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) --- base-commit: d35b2284e966c0bef3e2182a5c5ea02177dd32e4 change-id: 20240609-pcie-kirin-memleak-18c83a31d111 Best regards,