Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1499563 > unrolled thread

[PATCH 3.4 088/125] ser_gigaset: fix deallocation of platform device structure

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3.4 088/125] ser_gigaset: fix deallocation of platform device structure lizf@kernel.org - 2016-10-12 14:50 +0200
    Re: [PATCH 3.4 088/125] ser_gigaset: fix deallocation of platform  device structure Paul Bolle <pebolle@tiscali.nl> - 2016-10-12 15:00 +0200
      Re: [PATCH 3.4 088/125] ser_gigaset: fix deallocation of platform  device structure Zefan Li <lizefan@huawei.com> - 2016-10-13 05:00 +0200
        Re: [PATCH 3.4 088/125] ser_gigaset: fix deallocation of platform  device structure Paul Bolle <pebolle@tiscali.nl> - 2016-10-13 10:20 +0200
          Re: [PATCH 3.4 088/125] ser_gigaset: fix deallocation of platform  device structure Zefan Li <lizefan@huawei.com> - 2016-10-13 11:00 +0200

#1499563 — [PATCH 3.4 088/125] ser_gigaset: fix deallocation of platform device structure

From: Tilman Schmidt <tilman@imap.cc>

3.4.113-rc1 review patch.  If anyone has any objections, please let me know.

------------------


commit 4c5e354a974214dfb44cd23fa0429327693bc3ea upstream.

When shutting down the device, the struct ser_cardstate must not be
kfree()d immediately after the call to platform_device_unregister()
since the embedded struct platform_device is still in use.
Move the kfree() call to the release method instead.

Signed-off-by: Tilman Schmidt <tilman@imap.cc>
Fixes: 2869b23e4b95 ("drivers/isdn/gigaset: new M101 driver (v2)")
Reported-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Zefan Li <lizefan@huawei.com>
---
 drivers/isdn/gigaset/ser-gigaset.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index 6f3fd4c..3cdfcd0 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -371,19 +371,23 @@ static void gigaset_freecshw(struct cardstate *cs)
 	tasklet_kill(&cs->write_tasklet);
 	if (!cs->hw.ser)
 		return;
-	dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
 	platform_device_unregister(&cs->hw.ser->dev);
-	kfree(cs->hw.ser);
-	cs->hw.ser = NULL;
 }
 
 static void gigaset_device_release(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
+	struct cardstate *cs = dev_get_drvdata(dev);
 
 	/* adapted from platform_device_release() in drivers/base/platform.c */
 	kfree(dev->platform_data);
 	kfree(pdev->resource);
+
+	if (!cs)
+		return;
+	dev_set_drvdata(dev, NULL);
+	kfree(cs->hw.ser);
+	cs->hw.ser = NULL;
 }
 
 /*
-- 
1.9.1

[toc] | [next] | [standalone]

#1499595 — Re: [PATCH 3.4 088/125] ser_gigaset: fix deallocation of platform device structure

Zefan Li,

On Wed, 2016-10-12 at 20:33 +0800, lizf@kernel.org wrote:
> When shutting down the device, the struct ser_cardstate must not be
> kfree()d immediately after the call to platform_device_unregister()
> since the embedded struct platform_device is still in use.
> Move the kfree() call to the release method instead.
> 
> Signed-off-by: Tilman Schmidt <tilman@imap.cc>
> Fixes: 2869b23e4b95 ("drivers/isdn/gigaset: new M101 driver (v2)")
> Reported-by: Sasha Levin <sasha.levin@oracle.com>
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Zefan Li <lizefan@huawei.com>

There has been a follow up for this fix. I'll have to dive into my
archive to see why that was needed.

It was complicated, because there has been a short period in which this
fix was correct. Something like that, I'm speaking from memory.
(Perhaps Tilman's memory is less imperfect.)

I'll try get back to this shortly (in a day or so).

Thanks,


Paul Bolle

> ---
>  drivers/isdn/gigaset/ser-gigaset.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/isdn/gigaset/ser-gigaset.c
> b/drivers/isdn/gigaset/ser-gigaset.c
> index 6f3fd4c..3cdfcd0 100644
> --- a/drivers/isdn/gigaset/ser-gigaset.c
> +++ b/drivers/isdn/gigaset/ser-gigaset.c
> @@ -371,19 +371,23 @@ static void gigaset_freecshw(struct cardstate
> *cs)
>  	tasklet_kill(&cs->write_tasklet);
>  	if (!cs->hw.ser)
>  		return;
> -	dev_set_drvdata(&cs->hw.ser->dev.dev, NULL);
>  	platform_device_unregister(&cs->hw.ser->dev);
> -	kfree(cs->hw.ser);
> -	cs->hw.ser = NULL;
>  }
>  
>  static void gigaset_device_release(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> +	struct cardstate *cs = dev_get_drvdata(dev);
>  
>  	/* adapted from platform_device_release() in
> drivers/base/platform.c */
>  	kfree(dev->platform_data);
>  	kfree(pdev->resource);
> +
> +	if (!cs)
> +		return;
> +	dev_set_drvdata(dev, NULL);
> +	kfree(cs->hw.ser);
> +	cs->hw.ser = NULL;
>  }
>  
>  /*

[toc] | [prev] | [next] | [standalone]

#1500007 — Re: [PATCH 3.4 088/125] ser_gigaset: fix deallocation of platform device structure

On 2016/10/12 20:52, Paul Bolle wrote:
> Zefan Li,
> 
> On Wed, 2016-10-12 at 20:33 +0800, lizf@kernel.org wrote:
>> When shutting down the device, the struct ser_cardstate must not be
>> kfree()d immediately after the call to platform_device_unregister()
>> since the embedded struct platform_device is still in use.
>> Move the kfree() call to the release method instead.
>>
>> Signed-off-by: Tilman Schmidt <tilman@imap.cc>
>> Fixes: 2869b23e4b95 ("drivers/isdn/gigaset: new M101 driver (v2)")
>> Reported-by: Sasha Levin <sasha.levin@oracle.com>
>> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
>> Signed-off-by: David S. Miller <davem@davemloft.net>
>> Signed-off-by: Zefan Li <lizefan@huawei.com>
> 
> There has been a follow up for this fix. I'll have to dive into my
> archive to see why that was needed.
> 
> It was complicated, because there has been a short period in which this
> fix was correct. Something like that, I'm speaking from memory.
> (Perhaps Tilman's memory is less imperfect.)
> 
> I'll try get back to this shortly (in a day or so).
> 

Thanks for looking into this.

[toc] | [prev] | [next] | [standalone]

#1500087 — Re: [PATCH 3.4 088/125] ser_gigaset: fix deallocation of platform device structure

On Thu, 2016-10-13 at 10:52 +0800, Zefan Li wrote:
> On 2016/10/12 20:52, Paul Bolle wrote:
> > There has been a follow up for this fix. I'll have to dive into my
> > archive to see why that was needed.
> > 
> > It was complicated, because there has been a short period in which this
> > fix was correct. Something like that, I'm speaking from memory.
> > (Perhaps Tilman's memory is less imperfect.)
> > 
> > I'll try get back to this shortly (in a day or so).
> > 
> 
> Thanks for looking into this.

So what I think you also need _on top of_ this patch:
- commit 8aeb3c3d655e ("ser_gigaset: remove unnecessary kfree() calls
from release method"), for context changes; and
- commit 8d2c3ab44456 ("ser_gigaset: use container_of() instead of
detour"), the proper fix.

I could not get v3.4 to build _at all_ on my current Fedora 24 machine.
(v3.4 was probably released when Fedora 16 was still shiny and new.)
Lack of coffee? So I've only visually inspected these three commits on
top of v3.4.112. Is that acceptable to you?


Paul Bolle

[toc] | [prev] | [next] | [standalone]

#1500104 — Re: [PATCH 3.4 088/125] ser_gigaset: fix deallocation of platform device structure

On 2016/10/13 16:11, Paul Bolle wrote:
> On Thu, 2016-10-13 at 10:52 +0800, Zefan Li wrote:
>> On 2016/10/12 20:52, Paul Bolle wrote:
>>> There has been a follow up for this fix. I'll have to dive into my
>>> archive to see why that was needed.
>>>
>>> It was complicated, because there has been a short period in which this
>>> fix was correct. Something like that, I'm speaking from memory.
>>> (Perhaps Tilman's memory is less imperfect.)
>>>
>>> I'll try get back to this shortly (in a day or so).
>>>
>>
>> Thanks for looking into this.
> 
> So what I think you also need _on top of_ this patch:
> - commit 8aeb3c3d655e ("ser_gigaset: remove unnecessary kfree() calls
> from release method"), for context changes; and
> - commit 8d2c3ab44456 ("ser_gigaset: use container_of() instead of
> detour"), the proper fix.
> 
> I could not get v3.4 to build _at all_ on my current Fedora 24 machine.
> (v3.4 was probably released when Fedora 16 was still shiny and new.)
> Lack of coffee? So I've only visually inspected these three commits on
> top of v3.4.112. Is that acceptable to you?
> 

Yeah, I'll take it from here. Thanks!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web