| Message ID | 1455827348-8574-2-git-send-email-pebolle@tiscali.nl |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Am 18.02.2016 um 21:29 schrieb Paul Bolle: > The purpose of gigaset_device_release() is to kfree() the struct > ser_cardstate that contains our struct device. This is done via a bit of > a detour. First we make our struct device's driver_data point to the > container of our struct ser_cardstate (which is a struct cardstate). In > gigaset_device_release() we then retrieve that driver_data again. And > after that we finally kfree() the struct ser_cardstate that was saved in > the struct cardstate. > > All of this can be achieved much easier by using container_of() to get > from our struct device to its container, struct ser_cardstate. Do so. You're absolutely right. Very nice! > Note that at the time the detour was implemented commit b8b2c7d845d5 > ("base/platform: assert that dev_pm_domain callbacks are called > unconditionally") had just entered the tree. That commit disconnected > our platform_device and our platform_driver. These were reconnected > again in v4.5-rc2 through commit 25cad69f21f5 ("base/platform: Fix > platform drivers with no probe callback"). And one of the consequences > of that fix was that it broke the detour via driver_data. That's because > it made __device_release_driver() stop being a NOP for our struct device > and actually do stuff again. One of the things it now does, is setting > our driver_data to NULL. That, in turn, makes it impossible for > gigaset_device_release() to get to our struct cardstate. Which has the > net effect of leaking a struct ser_cardstate at every call of this > driver's tty close() operation. So using container_of() has the > additional benefit of actually working. > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Tested-by: Dmitry Vyukov <dvyukov@google.com> > Signed-off-by: Paul Bolle <pebolle@tiscali.nl> Acked-by: Tilman Schmidt <tilman@imap.cc> Thanks for cleaning up the mess I left behind. Tilman
On do, 2016-02-18 at 22:42 +0100, Tilman Schmidt wrote: > Acked-by: Tilman Schmidt <tilman@imap.cc> > > Thanks for cleaning up the mess I left behind. That's not how I look at it. Commit 4c5e354a9742 ("ser_gigaset: fix deallocation of platform device structure") provided a plausible fix for the issue we were trying to fix two months ago. If the platform code hadn't been broken when you wrote it - broken for ser_gigaset's corner case, that is - you would have noticed the lifetime rules of driver_data the hard way. (I suspect I might have never noticed if syzkaller hadn't insisted in rubbing my nose in it.) So chances are you would have come up with something similar to this fix would commit 25cad69f21f5 ("base/platform: Fix platform drivers with no probe callback") already have landed. Thanks, Paul Bolle
On Thu, Feb 18, 2016 at 09:29:08PM +0100, Paul Bolle wrote: > The purpose of gigaset_device_release() is to kfree() the struct > ser_cardstate that contains our struct device. This is done via a bit of > a detour. First we make our struct device's driver_data point to the > container of our struct ser_cardstate (which is a struct cardstate). In > gigaset_device_release() we then retrieve that driver_data again. And > after that we finally kfree() the struct ser_cardstate that was saved in > the struct cardstate. > > All of this can be achieved much easier by using container_of() to get > from our struct device to its container, struct ser_cardstate. Do so. > > Note that at the time the detour was implemented commit b8b2c7d845d5 > ("base/platform: assert that dev_pm_domain callbacks are called > unconditionally") had just entered the tree. That commit disconnected > our platform_device and our platform_driver. These were reconnected > again in v4.5-rc2 through commit 25cad69f21f5 ("base/platform: Fix > platform drivers with no probe callback"). And one of the consequences > of that fix was that it broke the detour via driver_data. That's because > it made __device_release_driver() stop being a NOP for our struct device > and actually do stuff again. One of the things it now does, is setting > our driver_data to NULL. That, in turn, makes it impossible for > gigaset_device_release() to get to our struct cardstate. Which has the > net effect of leaking a struct ser_cardstate at every call of this > driver's tty close() operation. So using container_of() has the > additional benefit of actually working. > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Tested-by: Dmitry Vyukov <dvyukov@google.com> > Signed-off-by: Paul Bolle <pebolle@tiscali.nl> > --- > drivers/isdn/gigaset/ser-gigaset.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read Documentation/stable_kernel_rules.txt for how to do this properly. </formletter>
Hi Greg, On do, 2016-02-18 at 15:46 -0800, Greg KH wrote: > <formletter> > > This is not the correct way to submit patches for inclusion in the > stable kernel tree. Please read Documentation/stable_kernel_rules.txt > for how to do this properly. > > </formletter> What in my submission triggers this formletter? Thanks, Paul Bolle
From: Paul Bolle <pebolle@tiscali.nl> Date: Thu, 18 Feb 2016 21:29:08 +0100 > The purpose of gigaset_device_release() is to kfree() the struct > ser_cardstate that contains our struct device. This is done via a bit of > a detour. First we make our struct device's driver_data point to the > container of our struct ser_cardstate (which is a struct cardstate). In > gigaset_device_release() we then retrieve that driver_data again. And > after that we finally kfree() the struct ser_cardstate that was saved in > the struct cardstate. > > All of this can be achieved much easier by using container_of() to get > from our struct device to its container, struct ser_cardstate. Do so. > > Note that at the time the detour was implemented commit b8b2c7d845d5 > ("base/platform: assert that dev_pm_domain callbacks are called > unconditionally") had just entered the tree. That commit disconnected > our platform_device and our platform_driver. These were reconnected > again in v4.5-rc2 through commit 25cad69f21f5 ("base/platform: Fix > platform drivers with no probe callback"). And one of the consequences > of that fix was that it broke the detour via driver_data. That's because > it made __device_release_driver() stop being a NOP for our struct device > and actually do stuff again. One of the things it now does, is setting > our driver_data to NULL. That, in turn, makes it impossible for > gigaset_device_release() to get to our struct cardstate. Which has the > net effect of leaking a struct ser_cardstate at every call of this > driver's tty close() operation. So using container_of() has the > additional benefit of actually working. > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Tested-by: Dmitry Vyukov <dvyukov@google.com> > Signed-off-by: Paul Bolle <pebolle@tiscali.nl> Applied, thanks.
diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c index 2a506fe0c8a4..d1f8ab915b15 100644 --- a/drivers/isdn/gigaset/ser-gigaset.c +++ b/drivers/isdn/gigaset/ser-gigaset.c @@ -373,13 +373,7 @@ static void gigaset_freecshw(struct cardstate *cs) static void gigaset_device_release(struct device *dev) { - struct cardstate *cs = dev_get_drvdata(dev); - - if (!cs) - return; - dev_set_drvdata(dev, NULL); - kfree(cs->hw.ser); - cs->hw.ser = NULL; + kfree(container_of(dev, struct ser_cardstate, dev.dev)); } /* @@ -408,7 +402,6 @@ static int gigaset_initcshw(struct cardstate *cs) cs->hw.ser = NULL; return rc; } - dev_set_drvdata(&cs->hw.ser->dev.dev, cs); tasklet_init(&cs->write_tasklet, gigaset_modem_fill, (unsigned long) cs);
The purpose of gigaset_device_release() is to kfree() the struct ser_cardstate that contains our struct device. This is done via a bit of a detour. First we make our struct device's driver_data point to the container of our struct ser_cardstate (which is a struct cardstate). In gigaset_device_release() we then retrieve that driver_data again. And after that we finally kfree() the struct ser_cardstate that was saved in the struct cardstate. All of this can be achieved much easier by using container_of() to get from our struct device to its container, struct ser_cardstate. Do so. Note that at the time the detour was implemented commit b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are called unconditionally") had just entered the tree. That commit disconnected our platform_device and our platform_driver. These were reconnected again in v4.5-rc2 through commit 25cad69f21f5 ("base/platform: Fix platform drivers with no probe callback"). And one of the consequences of that fix was that it broke the detour via driver_data. That's because it made __device_release_driver() stop being a NOP for our struct device and actually do stuff again. One of the things it now does, is setting our driver_data to NULL. That, in turn, makes it impossible for gigaset_device_release() to get to our struct cardstate. Which has the net effect of leaking a struct ser_cardstate at every call of this driver's tty close() operation. So using container_of() has the additional benefit of actually working. Reported-by: Dmitry Vyukov <dvyukov@google.com> Tested-by: Dmitry Vyukov <dvyukov@google.com> Signed-off-by: Paul Bolle <pebolle@tiscali.nl> --- drivers/isdn/gigaset/ser-gigaset.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)