* [PATCH v4 0/3] pci: endpoint: Fix double free in pci_epf_create() @ 2018-02-28 13:07 Rolf Evers-Fischer 2018-02-28 13:07 ` [PATCH v4 1/3] PCI: endpoint: Simplify name allocation for EPF device Rolf Evers-Fischer ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Rolf Evers-Fischer @ 2018-02-28 13:07 UTC (permalink / raw) To: kishon Cc: lorenzo.pieralisi, bhelgaas, linux-pci, linux-kernel, andy.shevchenko, Rolf Evers-Fischer This is version 4 of a patchset to avoid double free in function 'pci_epf_create()'. When I accidentally created a new endpoint device with an empty name, the kernel warned about "attempted to be registered with empty name!" and crashed afterwards. It turned out that the crash was not caused by the 'device_add()' function itself, but by a double kfree of 'epf->name' and 'epf'. The first patch just simplifies the code, while the second patch fixes the problem. The third patch removes the goto labels. Thank you Andy and Kishon for your Ack/Review on v3 for patches 1 and 2. In v4 of these patches only the first lines of the commit messages have been changed. Changes in v4: - s/pci/PCI and s/epf/EPF in the first line of recent commit messages (thanks, Bjorn!) - The new patch #3 removes the goto labels in function 'pci_epf_create()' (thanks, Lorenzo!) Changes in v3: - Matched to other pending pci endpoint commits (thanks, Bjorn!) - Added "Fixes" tag in patch 2 (thanks, Andy!) Changes in v2: - Based on feedback from Lorenzo, Andy and Kishon (thanks!) - Change IDs removed - First patch completely reworked in order to eliminate the need for the second 'kstrdup' allocation and the 'kfree' of the first allocation. It was tested with name="pci_epf_test.0" and name="pci_epb": The 'epf->name' was "pci_epf_test" or "pci_epb" (=unchanged). Rolf Evers-Fischer (3): PCI: endpoint: Simplify name allocation for EPF device PCI: endpoint: Fix kernel panic after put_device() PCI: endpoint: pci_epf_create: remove goto labels drivers/pci/endpoint/pci-epf-core.c | 50 +++++++++---------------------------- 1 file changed, 12 insertions(+), 38 deletions(-) -- 2.16.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 1/3] PCI: endpoint: Simplify name allocation for EPF device 2018-02-28 13:07 [PATCH v4 0/3] pci: endpoint: Fix double free in pci_epf_create() Rolf Evers-Fischer @ 2018-02-28 13:07 ` Rolf Evers-Fischer 2018-02-28 13:07 ` [PATCH v4 2/3] PCI: endpoint: Fix kernel panic after put_device() Rolf Evers-Fischer 2018-02-28 13:07 ` [PATCH v4 3/3] PCI: endpoint: pci_epf_create: remove goto labels Rolf Evers-Fischer 2 siblings, 0 replies; 7+ messages in thread From: Rolf Evers-Fischer @ 2018-02-28 13:07 UTC (permalink / raw) To: kishon Cc: lorenzo.pieralisi, bhelgaas, linux-pci, linux-kernel, andy.shevchenko, Rolf Evers-Fischer From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com> This commit replaces allocating and freeing the intermediate 'buf'/'func_name' with a combination of 'kstrndup()' and 'len'. 'len' is the required length of 'epf->name'. 'epf->name' should be either the first part of 'name' preceding the '.' or the complete 'name', if there is no '.' in the name. Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com> --- drivers/pci/endpoint/pci-epf-core.c | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c index 766ce1dca2ec..1f2506f32bb9 100644 --- a/drivers/pci/endpoint/pci-epf-core.c +++ b/drivers/pci/endpoint/pci-epf-core.c @@ -200,8 +200,7 @@ struct pci_epf *pci_epf_create(const char *name) int ret; struct pci_epf *epf; struct device *dev; - char *func_name; - char *buf; + int len; epf = kzalloc(sizeof(*epf), GFP_KERNEL); if (!epf) { @@ -209,20 +208,11 @@ struct pci_epf *pci_epf_create(const char *name) goto err_ret; } - buf = kstrdup(name, GFP_KERNEL); - if (!buf) { - ret = -ENOMEM; - goto free_epf; - } - - func_name = buf; - buf = strchrnul(buf, '.'); - *buf = '\0'; - - epf->name = kstrdup(func_name, GFP_KERNEL); + len = strchrnul(name, '.') - name; + epf->name = kstrndup(name, len, GFP_KERNEL); if (!epf->name) { ret = -ENOMEM; - goto free_func_name; + goto free_epf; } dev = &epf->dev; @@ -238,16 +228,12 @@ struct pci_epf *pci_epf_create(const char *name) if (ret) goto put_dev; - kfree(func_name); return epf; put_dev: put_device(dev); kfree(epf->name); -free_func_name: - kfree(func_name); - free_epf: kfree(epf); -- 2.16.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v4 2/3] PCI: endpoint: Fix kernel panic after put_device() 2018-02-28 13:07 [PATCH v4 0/3] pci: endpoint: Fix double free in pci_epf_create() Rolf Evers-Fischer 2018-02-28 13:07 ` [PATCH v4 1/3] PCI: endpoint: Simplify name allocation for EPF device Rolf Evers-Fischer @ 2018-02-28 13:07 ` Rolf Evers-Fischer 2018-02-28 13:07 ` [PATCH v4 3/3] PCI: endpoint: pci_epf_create: remove goto labels Rolf Evers-Fischer 2 siblings, 0 replies; 7+ messages in thread From: Rolf Evers-Fischer @ 2018-02-28 13:07 UTC (permalink / raw) To: kishon Cc: lorenzo.pieralisi, bhelgaas, linux-pci, linux-kernel, andy.shevchenko, Rolf Evers-Fischer From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com> 'put_device()' calls the relase function 'pci_epf_dev_release()', which already frees 'epf->name' and 'epf'. Therefore we must not free them again after 'put_device()'. Fixes: 5e8cb4033807 ("PCI: endpoint: Add EP core layer to enable EP controller and EP functions") Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com> --- drivers/pci/endpoint/pci-epf-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c index 1f2506f32bb9..1878a6776519 100644 --- a/drivers/pci/endpoint/pci-epf-core.c +++ b/drivers/pci/endpoint/pci-epf-core.c @@ -232,7 +232,7 @@ struct pci_epf *pci_epf_create(const char *name) put_dev: put_device(dev); - kfree(epf->name); + return ERR_PTR(ret); free_epf: kfree(epf); -- 2.16.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v4 3/3] PCI: endpoint: pci_epf_create: remove goto labels 2018-02-28 13:07 [PATCH v4 0/3] pci: endpoint: Fix double free in pci_epf_create() Rolf Evers-Fischer 2018-02-28 13:07 ` [PATCH v4 1/3] PCI: endpoint: Simplify name allocation for EPF device Rolf Evers-Fischer 2018-02-28 13:07 ` [PATCH v4 2/3] PCI: endpoint: Fix kernel panic after put_device() Rolf Evers-Fischer @ 2018-02-28 13:07 ` Rolf Evers-Fischer 2018-02-28 13:27 ` Kishon Vijay Abraham I 2018-02-28 13:33 ` Lorenzo Pieralisi 2 siblings, 2 replies; 7+ messages in thread From: Rolf Evers-Fischer @ 2018-02-28 13:07 UTC (permalink / raw) To: kishon Cc: lorenzo.pieralisi, bhelgaas, linux-pci, linux-kernel, andy.shevchenko, Rolf Evers-Fischer From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com> Removes the goto labels completely, handles the errors at the respective call site and just returns instead of jumping around. Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com> --- drivers/pci/endpoint/pci-epf-core.c | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c index 1878a6776519..cf2c4f6590a0 100644 --- a/drivers/pci/endpoint/pci-epf-core.c +++ b/drivers/pci/endpoint/pci-epf-core.c @@ -203,16 +203,14 @@ struct pci_epf *pci_epf_create(const char *name) int len; epf = kzalloc(sizeof(*epf), GFP_KERNEL); - if (!epf) { - ret = -ENOMEM; - goto err_ret; - } + if (!epf) + return ERR_PTR(-ENOMEM); len = strchrnul(name, '.') - name; epf->name = kstrndup(name, len, GFP_KERNEL); if (!epf->name) { - ret = -ENOMEM; - goto free_epf; + kfree(epf); + return ERR_PTR(-ENOMEM); } dev = &epf->dev; @@ -221,24 +219,14 @@ struct pci_epf *pci_epf_create(const char *name) dev->type = &pci_epf_type; ret = dev_set_name(dev, "%s", name); - if (ret) - goto put_dev; - - ret = device_add(dev); - if (ret) - goto put_dev; - - return epf; + if (!ret) { + ret = device_add(dev); + if (!ret) + return epf; + } -put_dev: put_device(dev); return ERR_PTR(ret); - -free_epf: - kfree(epf); - -err_ret: - return ERR_PTR(ret); } EXPORT_SYMBOL_GPL(pci_epf_create); -- 2.16.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 3/3] PCI: endpoint: pci_epf_create: remove goto labels 2018-02-28 13:07 ` [PATCH v4 3/3] PCI: endpoint: pci_epf_create: remove goto labels Rolf Evers-Fischer @ 2018-02-28 13:27 ` Kishon Vijay Abraham I 2018-02-28 13:33 ` Lorenzo Pieralisi 1 sibling, 0 replies; 7+ messages in thread From: Kishon Vijay Abraham I @ 2018-02-28 13:27 UTC (permalink / raw) To: Rolf Evers-Fischer Cc: lorenzo.pieralisi, bhelgaas, linux-pci, linux-kernel, andy.shevchenko, Rolf Evers-Fischer Hi, On Wednesday 28 February 2018 06:37 PM, Rolf Evers-Fischer wrote: > From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com> > > Removes the goto labels completely, handles the errors at the > respective call site and just returns instead of jumping around. > > Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com> > --- > drivers/pci/endpoint/pci-epf-core.c | 30 +++++++++--------------------- > 1 file changed, 9 insertions(+), 21 deletions(-) > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c > index 1878a6776519..cf2c4f6590a0 100644 > --- a/drivers/pci/endpoint/pci-epf-core.c > +++ b/drivers/pci/endpoint/pci-epf-core.c > @@ -203,16 +203,14 @@ struct pci_epf *pci_epf_create(const char *name) > int len; > > epf = kzalloc(sizeof(*epf), GFP_KERNEL); > - if (!epf) { > - ret = -ENOMEM; > - goto err_ret; > - } > + if (!epf) > + return ERR_PTR(-ENOMEM); > > len = strchrnul(name, '.') - name; > epf->name = kstrndup(name, len, GFP_KERNEL); > if (!epf->name) { > - ret = -ENOMEM; > - goto free_epf; > + kfree(epf); > + return ERR_PTR(-ENOMEM); > } > > dev = &epf->dev; > @@ -221,24 +219,14 @@ struct pci_epf *pci_epf_create(const char *name) > dev->type = &pci_epf_type; > > ret = dev_set_name(dev, "%s", name); > - if (ret) > - goto put_dev; > - > - ret = device_add(dev); > - if (ret) > - goto put_dev; > - > - return epf; > + if (!ret) { > + ret = device_add(dev); > + if (!ret) > + return epf; I don't prefer the correct return value being hidden under two levels of 'if'. Thanks Kishon ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 3/3] PCI: endpoint: pci_epf_create: remove goto labels 2018-02-28 13:07 ` [PATCH v4 3/3] PCI: endpoint: pci_epf_create: remove goto labels Rolf Evers-Fischer 2018-02-28 13:27 ` Kishon Vijay Abraham I @ 2018-02-28 13:33 ` Lorenzo Pieralisi 2018-02-28 14:46 ` Rolf Evers-Fischer 1 sibling, 1 reply; 7+ messages in thread From: Lorenzo Pieralisi @ 2018-02-28 13:33 UTC (permalink / raw) To: Rolf Evers-Fischer Cc: kishon, bhelgaas, linux-pci, linux-kernel, andy.shevchenko, Rolf Evers-Fischer On Wed, Feb 28, 2018 at 02:07:19PM +0100, Rolf Evers-Fischer wrote: > From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com> > > Removes the goto labels completely, handles the errors at the > respective call site and just returns instead of jumping around. > > Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com> > --- > drivers/pci/endpoint/pci-epf-core.c | 30 +++++++++--------------------- > 1 file changed, 9 insertions(+), 21 deletions(-) > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c > index 1878a6776519..cf2c4f6590a0 100644 > --- a/drivers/pci/endpoint/pci-epf-core.c > +++ b/drivers/pci/endpoint/pci-epf-core.c > @@ -203,16 +203,14 @@ struct pci_epf *pci_epf_create(const char *name) > int len; > > epf = kzalloc(sizeof(*epf), GFP_KERNEL); > - if (!epf) { > - ret = -ENOMEM; > - goto err_ret; > - } > + if (!epf) > + return ERR_PTR(-ENOMEM); > > len = strchrnul(name, '.') - name; > epf->name = kstrndup(name, len, GFP_KERNEL); > if (!epf->name) { > - ret = -ENOMEM; > - goto free_epf; > + kfree(epf); > + return ERR_PTR(-ENOMEM); > } > > dev = &epf->dev; > @@ -221,24 +219,14 @@ struct pci_epf *pci_epf_create(const char *name) > dev->type = &pci_epf_type; > > ret = dev_set_name(dev, "%s", name); > - if (ret) > - goto put_dev; > - > - ret = device_add(dev); > - if (ret) > - goto put_dev; > - > - return epf; > + if (!ret) { > + ret = device_add(dev); > + if (!ret) > + return epf; > + } This is ugly, sorry. I should have been more explicit, I prefer this construct: ret = dev_set_name(dev, "%s", name); if (ret) { kfree(epf->name); kfree(epf); return ERR_PTR(ret); } ret = device_add(dev); if (ret) { put_device(dev); return ERR_PTR(ret); } return epf; Please send a v5 and let's be done with it. Thanks, Lorenzo > -put_dev: > put_device(dev); > return ERR_PTR(ret); > - > -free_epf: > - kfree(epf); > - > -err_ret: > - return ERR_PTR(ret); > } > EXPORT_SYMBOL_GPL(pci_epf_create); > > -- > 2.16.2 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 3/3] PCI: endpoint: pci_epf_create: remove goto labels 2018-02-28 13:33 ` Lorenzo Pieralisi @ 2018-02-28 14:46 ` Rolf Evers-Fischer 0 siblings, 0 replies; 7+ messages in thread From: Rolf Evers-Fischer @ 2018-02-28 14:46 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Rolf Evers-Fischer, kishon, bhelgaas, linux-pci, linux-kernel, andy.shevchenko, Rolf Evers-Fischer Hi Kishon and Lorenzo, On Wed, 28 Feb 2018, Lorenzo Pieralisi wrote: > On Wed, Feb 28, 2018 at 02:07:19PM +0100, Rolf Evers-Fischer wrote: > > From: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com> > > > > Removes the goto labels completely, handles the errors at the > > respective call site and just returns instead of jumping around. > > > > Signed-off-by: Rolf Evers-Fischer <rolf.evers.fischer@aptiv.com> > > --- > > drivers/pci/endpoint/pci-epf-core.c | 30 +++++++++--------------------- > > 1 file changed, 9 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c > > index 1878a6776519..cf2c4f6590a0 100644 > > --- a/drivers/pci/endpoint/pci-epf-core.c > > +++ b/drivers/pci/endpoint/pci-epf-core.c > > @@ -203,16 +203,14 @@ struct pci_epf *pci_epf_create(const char *name) > > int len; > > > > epf = kzalloc(sizeof(*epf), GFP_KERNEL); > > - if (!epf) { > > - ret = -ENOMEM; > > - goto err_ret; > > - } > > + if (!epf) > > + return ERR_PTR(-ENOMEM); > > > > len = strchrnul(name, '.') - name; > > epf->name = kstrndup(name, len, GFP_KERNEL); > > if (!epf->name) { > > - ret = -ENOMEM; > > - goto free_epf; > > + kfree(epf); > > + return ERR_PTR(-ENOMEM); > > } > > > > dev = &epf->dev; > > @@ -221,24 +219,14 @@ struct pci_epf *pci_epf_create(const char *name) > > dev->type = &pci_epf_type; > > > > ret = dev_set_name(dev, "%s", name); > > - if (ret) > > - goto put_dev; > > - > > - ret = device_add(dev); > > - if (ret) > > - goto put_dev; > > - > > - return epf; > > + if (!ret) { > > + ret = device_add(dev); > > + if (!ret) > > + return epf; > > + } > > This is ugly, sorry. I should have been more explicit, I prefer > this construct: > > ret = dev_set_name(dev, "%s", name); > if (ret) { > kfree(epf->name); > kfree(epf); > return ERR_PTR(ret); > } > > ret = device_add(dev); > if (ret) { > put_device(dev); > return ERR_PTR(ret); > } > > return epf; > > Please send a v5 and let's be done with it. > Thank you for your feedback. I agree with you. Hiding the correct return value under two levels of 'if' is not the best idea. I will send a v5 containing the proposal from Lorenzo. There is only one difference: We must call 'put_device()' in both error cases, because we have already initialized the device before. We had also jumped into 'put_dev:' in both situations before. The modified code will then look like this: ret = dev_set_name(dev, "%s", name); if (ret) { put_device(dev); return ERR_PTR(ret); } ret = device_add(dev); if (ret) { put_device(dev); return ERR_PTR(ret); } return epf; Best regards, Rolf > > -put_dev: > > put_device(dev); > > return ERR_PTR(ret); > > - > > -free_epf: > > - kfree(epf); > > - > > -err_ret: > > - return ERR_PTR(ret); > > } > > EXPORT_SYMBOL_GPL(pci_epf_create); > > > > -- > > 2.16.2 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-02-28 14:46 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-28 13:07 [PATCH v4 0/3] pci: endpoint: Fix double free in pci_epf_create() Rolf Evers-Fischer 2018-02-28 13:07 ` [PATCH v4 1/3] PCI: endpoint: Simplify name allocation for EPF device Rolf Evers-Fischer 2018-02-28 13:07 ` [PATCH v4 2/3] PCI: endpoint: Fix kernel panic after put_device() Rolf Evers-Fischer 2018-02-28 13:07 ` [PATCH v4 3/3] PCI: endpoint: pci_epf_create: remove goto labels Rolf Evers-Fischer 2018-02-28 13:27 ` Kishon Vijay Abraham I 2018-02-28 13:33 ` Lorenzo Pieralisi 2018-02-28 14:46 ` Rolf Evers-Fischer
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.