| Message ID | 1420644571-18928-2-git-send-email-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Jan 07, 2015 at 03:29:29PM +0000, Lorenzo Pieralisi wrote: > In the function of_pci_get_host_bridge_resources() if the parsing of > ranges fails, previously allocated resources inclusive of bus_range > are not freed and are not expected to be freed by the function caller > on error return. > > This patch fixes the issues by adding code that properly frees resources > and bus_range before exiting the function with an error return value. > > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Liviu Dudau <liviu.dudau@arm.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Rob Herring <robh+dt@kernel.org> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > --- > drivers/of/of_pci.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > index 88471d3..6fbfe99 100644 > --- a/drivers/of/of_pci.c > +++ b/drivers/of/of_pci.c > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > struct of_pci_range_parser parser; > char range_type[4]; > int err; > + struct pci_host_bridge_window *window; > > if (io_base) > *io_base = (resource_size_t)OF_BAD_ADDR; > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > conversion_failed: > kfree(res); > parse_failed: > + list_for_each_entry(window, resources, list) > + kfree(window->res); > pci_free_resource_list(resources); > + kfree(bus_range); > return err; > } > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); Hi Lorenzo et all, Here is my personal view and I am happy to hear from others on the desired behaviour: When I wrote this function what I had in mind was that it will parse as much as possible from the device tree and return a list of resources that could be successfully converted. If the entire list of ranges could not be converted then an error code will be returned, but the caller still had the list as constructed up to the error. It was the job of the caller to free the list in either cases, as stated in the comment. The historical reason why the function was written that way was because at some moment after parsing I've had an additional step where arches could cleanup / veto the list and they could return an error value to signal their discontent. Also I was (am) not sure how lenient we could be with the device tree not being sane (at least one host bridge binding lists the config space as a range, which was accepted as broken). So, from that point of view, I would NAK this patch, as the function works as intended. If others find this mode of operation too convoluted, then the patch should probably make clear that cleanup only needs to be done on function returning success. Best regards, > -- > 2.2.1 > >
On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: [...] > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > struct of_pci_range_parser parser; > > char range_type[4]; > > int err; > > + struct pci_host_bridge_window *window; > > > > if (io_base) > > *io_base = (resource_size_t)OF_BAD_ADDR; > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > conversion_failed: > > kfree(res); > > parse_failed: > > + list_for_each_entry(window, resources, list) > > + kfree(window->res); > > pci_free_resource_list(resources); > > + kfree(bus_range); > > return err; > > } > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > Hi Lorenzo et all, > > Here is my personal view and I am happy to hear from others on the desired > behaviour: > > When I wrote this function what I had in mind was that it will parse as > much as possible from the device tree and return a list of resources that > could be successfully converted. If the entire list of ranges could not > be converted then an error code will be returned, but the caller still > had the list as constructed up to the error. It was the job of the caller > to free the list in either cases, as stated in the comment. That's what I am questioning. If the function takes an error path, the windows list is freed, so the resource pointers are gone. There is no way the caller can grab those resource pointers and free them. So either way, the function needs patching. Either we do not free the windows list (we remove pci_free_resource_list) or we apply my fix (or we refactor the API which is likely to be what I will do). Lorenzo > > The historical reason why the function was written that way was because at > some moment after parsing I've had an additional step where arches could > cleanup / veto the list and they could return an error value to signal > their discontent. Also I was (am) not sure how lenient we could be with > the device tree not being sane (at least one host bridge binding lists the > config space as a range, which was accepted as broken). > > So, from that point of view, I would NAK this patch, as the function works > as intended. If others find this mode of operation too convoluted, then > the patch should probably make clear that cleanup only needs to be done on > function returning success. > > Best regards, > > > -- > > 2.2.1 > > > > > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ¯\_(?)_/¯ -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote: > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: > > [...] > > > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > struct of_pci_range_parser parser; > > > char range_type[4]; > > > int err; > > > + struct pci_host_bridge_window *window; > > > > > > if (io_base) > > > *io_base = (resource_size_t)OF_BAD_ADDR; > > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > conversion_failed: > > > kfree(res); > > > parse_failed: > > > + list_for_each_entry(window, resources, list) > > > + kfree(window->res); > > > pci_free_resource_list(resources); > > > + kfree(bus_range); > > > return err; > > > } > > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > > > Hi Lorenzo et all, > > > > Here is my personal view and I am happy to hear from others on the desired > > behaviour: > > > > When I wrote this function what I had in mind was that it will parse as > > much as possible from the device tree and return a list of resources that > > could be successfully converted. If the entire list of ranges could not > > be converted then an error code will be returned, but the caller still > > had the list as constructed up to the error. It was the job of the caller > > to free the list in either cases, as stated in the comment. > > That's what I am questioning. If the function takes an error path, the > windows list is freed, so the resource pointers are gone. There is no > way the caller can grab those resource pointers and free them. I stand corrected. Your patch is needed. Thanks, Liviu > > So either way, the function needs patching. Either we do not free the > windows list (we remove pci_free_resource_list) or we apply my fix (or > we refactor the API which is likely to be what I will do). > > Lorenzo > > > > > The historical reason why the function was written that way was because at > > some moment after parsing I've had an additional step where arches could > > cleanup / veto the list and they could return an error value to signal > > their discontent. Also I was (am) not sure how lenient we could be with > > the device tree not being sane (at least one host bridge binding lists the > > config space as a range, which was accepted as broken). > > > > So, from that point of view, I would NAK this patch, as the function works > > as intended. If others find this mode of operation too convoluted, then > > the patch should probably make clear that cleanup only needs to be done on > > function returning success. > > > > Best regards, > > > > > -- > > > 2.2.1 > > > > > >
On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote: > On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote: > > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: > > > > [...] > > > > > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > struct of_pci_range_parser parser; > > > > char range_type[4]; > > > > int err; > > > > + struct pci_host_bridge_window *window; > > > > > > > > if (io_base) > > > > *io_base = (resource_size_t)OF_BAD_ADDR; > > > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > conversion_failed: > > > > kfree(res); > > > > parse_failed: > > > > + list_for_each_entry(window, resources, list) > > > > + kfree(window->res); > > > > pci_free_resource_list(resources); > > > > + kfree(bus_range); > > > > return err; > > > > } > > > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > > > > > Hi Lorenzo et all, > > > > > > Here is my personal view and I am happy to hear from others on the desired > > > behaviour: > > > > > > When I wrote this function what I had in mind was that it will parse as > > > much as possible from the device tree and return a list of resources that > > > could be successfully converted. If the entire list of ranges could not > > > be converted then an error code will be returned, but the caller still > > > had the list as constructed up to the error. It was the job of the caller > > > to free the list in either cases, as stated in the comment. > > > > That's what I am questioning. If the function takes an error path, the > > windows list is freed, so the resource pointers are gone. There is no > > way the caller can grab those resource pointers and free them. > > I stand corrected. Your patch is needed. Mind acking it ? I will ask Bjorn to take it then. Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 26, 2015 at 11:21:11AM +0000, Lorenzo Pieralisi wrote: > On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote: > > On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote: > > > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: > > > > > > [...] > > > > > > > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > > struct of_pci_range_parser parser; > > > > > char range_type[4]; > > > > > int err; > > > > > + struct pci_host_bridge_window *window; > > > > > > > > > > if (io_base) > > > > > *io_base = (resource_size_t)OF_BAD_ADDR; > > > > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > > conversion_failed: > > > > > kfree(res); > > > > > parse_failed: > > > > > + list_for_each_entry(window, resources, list) > > > > > + kfree(window->res); > > > > > pci_free_resource_list(resources); > > > > > + kfree(bus_range); > > > > > return err; > > > > > } > > > > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > > > > > > > Hi Lorenzo et all, > > > > > > > > Here is my personal view and I am happy to hear from others on the desired > > > > behaviour: > > > > > > > > When I wrote this function what I had in mind was that it will parse as > > > > much as possible from the device tree and return a list of resources that > > > > could be successfully converted. If the entire list of ranges could not > > > > be converted then an error code will be returned, but the caller still > > > > had the list as constructed up to the error. It was the job of the caller > > > > to free the list in either cases, as stated in the comment. > > > > > > That's what I am questioning. If the function takes an error path, the > > > windows list is freed, so the resource pointers are gone. There is no > > > way the caller can grab those resource pointers and free them. > > > > I stand corrected. Your patch is needed. > > Mind acking it ? I will ask Bjorn to take it then. Sure. Acked-by: Liviu Dudau <Liviu.Dudau@arm.com> > > Thanks, > Lorenzo >
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c index 88471d3..6fbfe99 100644 --- a/drivers/of/of_pci.c +++ b/drivers/of/of_pci.c @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, struct of_pci_range_parser parser; char range_type[4]; int err; + struct pci_host_bridge_window *window; if (io_base) *io_base = (resource_size_t)OF_BAD_ADDR; @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, conversion_failed: kfree(res); parse_failed: + list_for_each_entry(window, resources, list) + kfree(window->res); pci_free_resource_list(resources); + kfree(bus_range); return err; } EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
In the function of_pci_get_host_bridge_resources() if the parsing of ranges fails, previously allocated resources inclusive of bus_range are not freed and are not expected to be freed by the function caller on error return. This patch fixes the issues by adding code that properly frees resources and bus_range before exiting the function with an error return value. Cc: Arnd Bergmann <arnd@arndb.de> Cc: Liviu Dudau <liviu.dudau@arm.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Rob Herring <robh+dt@kernel.org> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> --- drivers/of/of_pci.c | 4 ++++ 1 file changed, 4 insertions(+)