[PATCH v3 0/2] pci: endpoint: Fix double free in pci_epf_create()

[PATCH v3 0/2] pci: endpoint: Fix double free in pci_epf_create()

[Rolf Evers-Fischer]

This is version 3 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.

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 (2):
  pci: endpoint: Simplify name allocation for epf device
  pci: endpoint: Fix kernel panic after put_device()

 drivers/pci/endpoint/pci-epf-core.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

-- 
2.16.2

[PATCH v3 1/2] pci: endpoint: Simplify name allocation for epf device

[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

[PATCH v3 2/2] pci: endpoint: Fix kernel panic after put_device()

[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

Re: [PATCH v3 1/2] pci: endpoint: Simplify name allocation for epf device

[Kishon Vijay Abraham I]

Hi,

On Tuesday 27 February 2018 03:32 PM, Rolf Evers-Fischer wrote:
> 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);

shouldn't the string end with '\0'?

Thanks
Kishon

Re: [PATCH v3 1/2] pci: endpoint: Simplify name allocation for epf device

[Rolf Evers-Fischer]

Hi,

On Tue, 27 Feb 2018, Kishon Vijay Abraham I wrote:

> Hi,
> 
> On Tuesday 27 February 2018 03:32 PM, Rolf Evers-Fischer wrote:
> > 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);
> 
> shouldn't the string end with '\0'?
> 

I agree, it should end with '\0', but fortunately the 'kstrndup()' 
function already adds it for us, see mm/util.c, line 100:
	(...)
	memcpy(buf, s, len);
	buf[len] = '\0';
	(...)

Kind regards
 Rolf

Re: [PATCH v3 0/2] pci: endpoint: Fix double free in pci_epf_create()

[Lorenzo Pieralisi]

On Tue, Feb 27, 2018 at 11:02:28AM +0100, Rolf Evers-Fischer wrote:
> This is version 3 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.
> 
> 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 (2):
>   pci: endpoint: Simplify name allocation for epf device
>   pci: endpoint: Fix kernel panic after put_device()
    ^^^

s/pci/PCI

Bjorn pointed that out too, I can do it when merging the patches myself
just a hint in case you have to respin it again, no problem.

Thanks,
Lorenzo

>  drivers/pci/endpoint/pci-epf-core.c | 24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
> 
> -- 
> 2.16.2
> 

Re: [PATCH v3 1/2] pci: endpoint: Simplify name allocation for epf device

[Kishon Vijay Abraham I]

On Tuesday 27 February 2018 03:45 PM, Rolf Evers-Fischer wrote:
> Hi,
> 
> On Tue, 27 Feb 2018, Kishon Vijay Abraham I wrote:
> 
>> Hi,
>>
>> On Tuesday 27 February 2018 03:32 PM, Rolf Evers-Fischer wrote:
>>> 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);
>>
>> shouldn't the string end with '\0'?
>>
> 
> I agree, it should end with '\0', but fortunately the 'kstrndup()' 
> function already adds it for us, see mm/util.c, line 100:
> 	(...)
> 	memcpy(buf, s, len);
> 	buf[len] = '\0';
> 	(...)

oh okay. missed that..

Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> 
> Kind regards
>  Rolf
> 

Re: [PATCH v3 2/2] pci: endpoint: Fix kernel panic after put_device()

[Kishon Vijay Abraham I]

On Tuesday 27 February 2018 03:32 PM, Rolf Evers-Fischer wrote:
> 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>

Acked-by: Kishon Vijay Abraham I <kishon@ti.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);
> 

Re: [PATCH v3 0/2] pci: endpoint: Fix double free in pci_epf_create()

[Rolf Evers-Fischer]

Hello Lorenzo,

On Tue, 27 Feb 2018, Lorenzo Pieralisi wrote:

> On Tue, Feb 27, 2018 at 11:02:28AM +0100, Rolf Evers-Fischer wrote:
> > This is version 3 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.
> > 
> > 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 (2):
> >   pci: endpoint: Simplify name allocation for epf device
> >   pci: endpoint: Fix kernel panic after put_device()
>     ^^^
> 
> s/pci/PCI
> 
> Bjorn pointed that out too, I can do it when merging the patches myself
> just a hint in case you have to respin it again, no problem.
> 

thank you for pointing this out. I haven't recognized this detail. I 
thought that Bjorn wanted me to rebase my patch series upon the latest 
unmerged PCI endpoint patches - and I was surprised that there was no 
change.

Thank you for your kind offer to fix it when merging the patches.

Best regards,
 Rolf

Re: [PATCH v3 0/2] pci: endpoint: Fix double free in pci_epf_create()

[Bjorn Helgaas]

On Tue, Feb 27, 2018 at 10:20:30AM +0000, Lorenzo Pieralisi wrote:
> On Tue, Feb 27, 2018 at 11:02:28AM +0100, Rolf Evers-Fischer wrote:
> > This is version 3 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.
> > 
> > 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 (2):
> >   pci: endpoint: Simplify name allocation for epf device
> >   pci: endpoint: Fix kernel panic after put_device()
>     ^^^
> 
> s/pci/PCI
> 
> Bjorn pointed that out too, I can do it when merging the patches myself
> just a hint in case you have to respin it again, no problem.

I wonder if Rolf posted the wrong version accidentally, since he
mentioned making these match, but I don't see anything different in
this posting.  Oh, never mind, I see the response about rebasing.
I should have been more clear about what I was looking for.

Also s/epf/EPF/, since "epf" isn't a real word.

Re: [PATCH v3 0/2] pci: endpoint: Fix double free in pci_epf_create()

[Andy Shevchenko]

On Tue, Feb 27, 2018 at 12:02 PM, Rolf Evers-Fischer
<embedded24@evers-fischer.de> wrote:
> This is version 3 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.
>

After addressing Bjorn's comments, FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> 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 (2):
>   pci: endpoint: Simplify name allocation for epf device
>   pci: endpoint: Fix kernel panic after put_device()
>
>  drivers/pci/endpoint/pci-epf-core.c | 24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
>
> --
> 2.16.2
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] pci: endpoint: Fix kernel panic after put_device()

[Lorenzo Pieralisi]

On Tue, Feb 27, 2018 at 11:02:30AM +0100, Rolf Evers-Fischer wrote:
> 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);

Another thing you could do, which would get rid of these multiple return
statements (yes there is another one) would consist in removing the goto
labels completely and handle the errors at the respective call site and
just return instead of jumping around.

Thanks,
Lorenzo

Re: [PATCH v3 2/2] pci: endpoint: Fix kernel panic after put_device()

[Rolf Evers-Fischer]

Hello Lorenzo!

On Tue, 27 Feb 2018, Lorenzo Pieralisi wrote:

> On Tue, Feb 27, 2018 at 11:02:30AM +0100, Rolf Evers-Fischer wrote:
> > 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);
> 
> Another thing you could do, which would get rid of these multiple return
> statements (yes there is another one) would consist in removing the goto
> labels completely and handle the errors at the respective call site and
> just return instead of jumping around.
> 

Thank you for sharing this idea! I've already changed it locally and it 
seems that we can get rid of 11 code lines with that.
It is being verified. If it really works, there will be a new commit
soon.

Kind regards,
 Rolf

Re: [PATCH v3 0/2] pci: endpoint: Fix double free in pci_epf_create()

[Rolf Evers-Fischer]

Hello Bjorn!

On Tue, 27 Feb 2018, Bjorn Helgaas wrote:

> On Tue, Feb 27, 2018 at 10:20:30AM +0000, Lorenzo Pieralisi wrote:
> > On Tue, Feb 27, 2018 at 11:02:28AM +0100, Rolf Evers-Fischer wrote:
> > > This is version 3 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.
> > > 
> > > 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 (2):
> > >   pci: endpoint: Simplify name allocation for epf device
> > >   pci: endpoint: Fix kernel panic after put_device()
> >     ^^^
> > 
> > s/pci/PCI
> > 
> > Bjorn pointed that out too, I can do it when merging the patches myself
> > just a hint in case you have to respin it again, no problem.
> 
> I wonder if Rolf posted the wrong version accidentally, since he
> mentioned making these match, but I don't see anything different in
> this posting.  Oh, never mind, I see the response about rebasing.
> I should have been more clear about what I was looking for.
> 
> Also s/epf/EPF/, since "epf" isn't a real word.
> 

It was my mistake. I should have asked you to clarify your request - 
especially after detecting that rebasing the commits didn't change 
anything.

Nevertheless, it seems that there will be soon a v4, where the s/pci/PCI 
and the s/epf/EPF can be done along with.

Best regards,
 Rolf