* [PATCH 0/3] ser_gigaset fixes
@ 2015-12-08 11:00 Tilman Schmidt
2015-12-08 11:00 ` [PATCH 1/3] ser_gigaset: fix up NULL checks Tilman Schmidt
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Tilman Schmidt @ 2015-12-08 11:00 UTC (permalink / raw)
To: Paul Bolle, netdev
Cc: Peter Hurley, Sasha Levin, syzkaller, David Miller, Karsten Keil,
isdn4linux, gigaset307x-common, linux-kernel
Hi Paul,
this series is the result of our discussion on the "freeing an
active object" bug. I split my proposed patch into two patches
for the separate topics of moving the ser_cardstate kfree() and
dropping the useless kfree()s, and also included an unrelated
patch (1/3) that had fallen through the cracks in my last series.
Patch 2/3 should go into stable releases all the way back to 2.6.32.
It applies cleanly to release 3.*/4.* with at most offset 1.
For release 2.6.32 there is a trivial merge conflict with a removed
comment line.
Thanks,
Tilman
Tilman Schmidt (3):
ser_gigaset: fix up NULL checks
ser_gigaset: fix deallocation of platform device structure
ser_gigaset: remove unnecessary kfree() calls from release method
drivers/isdn/gigaset/ser-gigaset.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
--
1.9.2.459.g68773ac
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method
2015-12-08 11:00 [PATCH 0/3] ser_gigaset fixes Tilman Schmidt
2015-12-08 11:00 ` [PATCH 1/3] ser_gigaset: fix up NULL checks Tilman Schmidt
2015-12-08 11:00 ` [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure Tilman Schmidt
@ 2015-12-08 11:00 ` Tilman Schmidt
2015-12-08 23:15 ` Paul Bolle
2015-12-10 11:31 ` [PATCH 0/3] ser_gigaset fixes Paul Bolle
3 siblings, 1 reply; 18+ messages in thread
From: Tilman Schmidt @ 2015-12-08 11:00 UTC (permalink / raw)
To: Paul Bolle, netdev
Cc: Peter Hurley, Sasha Levin, syzkaller, David Miller, Karsten Keil,
isdn4linux, gigaset307x-common, linux-kernel
device->platform_data and platform_device->resource are never used
and remain NULL through their entire life. Drops the kfree() calls
for them from the device release method.
Signed-off-by: Tilman Schmidt <tilman@imap.cc>
Reported-by: Paul Bolle <pebolle@tiscali.nl>
---
drivers/isdn/gigaset/ser-gigaset.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index 2693cb2..b7e0329 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -375,13 +375,8 @@ static void gigaset_freecshw(struct cardstate *cs)
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);
--
1.9.2.459.g68773ac
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 1/3] ser_gigaset: fix up NULL checks
2015-12-08 11:00 [PATCH 0/3] ser_gigaset fixes Tilman Schmidt
@ 2015-12-08 11:00 ` Tilman Schmidt
2015-12-08 19:45 ` Paul Bolle
2015-12-08 11:00 ` [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure Tilman Schmidt
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Tilman Schmidt @ 2015-12-08 11:00 UTC (permalink / raw)
To: Paul Bolle, netdev
Cc: Peter Hurley, Sasha Levin, syzkaller, David Miller, Karsten Keil,
isdn4linux, gigaset307x-common, linux-kernel
Commit f34d7a5b changed tty->driver to tty->ops but left NULL checks
for tty->driver untouched. Fix.
Signed-off-by: Tilman Schmidt <tilman@imap.cc>
Fixes: f34d7a5b7010 ("tty: The big operations rework")
---
drivers/isdn/gigaset/ser-gigaset.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index 375be50..d8771b5 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -67,7 +67,7 @@ static int write_modem(struct cardstate *cs)
struct sk_buff *skb = bcs->tx_skb;
int sent = -EOPNOTSUPP;
- if (!tty || !tty->driver || !skb)
+ if (!tty || !tty->ops || !skb)
return -EINVAL;
if (!skb->len) {
@@ -109,7 +109,7 @@ static int send_cb(struct cardstate *cs)
unsigned long flags;
int sent = 0;
- if (!tty || !tty->driver)
+ if (!tty || !tty->ops)
return -EFAULT;
cb = cs->cmdbuf;
@@ -432,7 +432,7 @@ static int gigaset_set_modem_ctrl(struct cardstate *cs, unsigned old_state,
struct tty_struct *tty = cs->hw.ser->tty;
unsigned int set, clear;
- if (!tty || !tty->driver || !tty->ops->tiocmset)
+ if (!tty || !tty->ops || !tty->ops->tiocmset)
return -EINVAL;
set = new_state & ~old_state;
clear = old_state & ~new_state;
--
1.9.2.459.g68773ac
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
2015-12-08 11:00 [PATCH 0/3] ser_gigaset fixes Tilman Schmidt
2015-12-08 11:00 ` [PATCH 1/3] ser_gigaset: fix up NULL checks Tilman Schmidt
@ 2015-12-08 11:00 ` Tilman Schmidt
2015-12-08 23:12 ` Paul Bolle
2015-12-08 11:00 ` [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method Tilman Schmidt
2015-12-10 11:31 ` [PATCH 0/3] ser_gigaset fixes Paul Bolle
3 siblings, 1 reply; 18+ messages in thread
From: Tilman Schmidt @ 2015-12-08 11:00 UTC (permalink / raw)
To: Paul Bolle, netdev
Cc: Peter Hurley, Sasha Levin, syzkaller, David Miller, Karsten Keil,
isdn4linux, gigaset307x-common, linux-kernel
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>
---
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 d8771b5..2693cb2 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -370,19 +370,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.2.459.g68773ac
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] ser_gigaset: fix up NULL checks
2015-12-08 11:00 ` [PATCH 1/3] ser_gigaset: fix up NULL checks Tilman Schmidt
@ 2015-12-08 19:45 ` Paul Bolle
2015-12-08 22:16 ` One Thousand Gnomes
0 siblings, 1 reply; 18+ messages in thread
From: Paul Bolle @ 2015-12-08 19:45 UTC (permalink / raw)
To: Tilman Schmidt, netdev
Cc: Peter Hurley, Sasha Levin, syzkaller, David Miller, Karsten Keil,
isdn4linux, gigaset307x-common, linux-kernel
Hi Tilman,
On di, 2015-12-08 at 12:00 +0100, Tilman Schmidt wrote:
> Commit f34d7a5b changed tty->driver to tty->ops but left NULL checks
(This makes checkpatch complain, but the correct commit description
style is used in the Fixes: tag, so it's not a big deal.)
> for tty->driver untouched. Fix.
>
> Signed-off-by: Tilman Schmidt <tilman@imap.cc>
> Fixes: f34d7a5b7010 ("tty: The big operations rework")
Should we backport this all the way to v2.6.32 (currently the oldest
stable tree)?
> diff --git a/drivers/isdn/gigaset/ser-gigaset.c
> b/drivers/isdn/gigaset/ser-gigaset.c
> index 375be50..d8771b5 100644
> --- a/drivers/isdn/gigaset/ser-gigaset.c
> +++ b/drivers/isdn/gigaset/ser-gigaset.c
> @@ -67,7 +67,7 @@ static int write_modem(struct cardstate *cs)
> struct sk_buff *skb = bcs->tx_skb;
> int sent = -EOPNOTSUPP;
>
> - if (!tty || !tty->driver || !skb)
> + if (!tty || !tty->ops || !skb)
> return -EINVAL;
>
> if (!skb->len) {
> @@ -109,7 +109,7 @@ static int send_cb(struct cardstate *cs)
> unsigned long flags;
> int sent = 0;
>
> - if (!tty || !tty->driver)
> + if (!tty || !tty->ops)
> return -EFAULT;
>
> cb = cs->cmdbuf;
> @@ -432,7 +432,7 @@ static int gigaset_set_modem_ctrl(struct cardstate
> *cs, unsigned old_state,
> struct tty_struct *tty = cs->hw.ser->tty;
> unsigned int set, clear;
>
> - if (!tty || !tty->driver || !tty->ops->tiocmset)
> + if (!tty || !tty->ops || !tty->ops->tiocmset)
> return -EINVAL;
> set = new_state & ~old_state;
> clear = old_state & ~new_state;
It's pretty obvious that this should have been part of commit
f34d7a5b7010 ("tty: The big operations rework"). That being said, these
test puzzle me. It's not obvious why they're needed. Ie, can the null
dereferences they try to catch really happen? But I can try to figure
out that in the future, if I ever feel the urge to do so. Anyhow:
Acked-by: Paul Bolle <pebolle@tiscali.nl>
Paul Bolle
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] ser_gigaset: fix up NULL checks
2015-12-08 19:45 ` Paul Bolle
@ 2015-12-08 22:16 ` One Thousand Gnomes
2015-12-09 10:45 ` Tilman Schmidt
0 siblings, 1 reply; 18+ messages in thread
From: One Thousand Gnomes @ 2015-12-08 22:16 UTC (permalink / raw)
To: Paul Bolle
Cc: Tilman Schmidt, netdev, Peter Hurley, Sasha Levin, syzkaller,
David Miller, Karsten Keil, isdn4linux, gigaset307x-common,
linux-kernel
> Should we backport this all the way to v2.6.32 (currently the oldest
> stable tree)?
We need to be able to explain how the case being tested can occur, then
explain the situation in which it actually prevents a race condition.
If nobody can do that then it shouldn't be backported because its change
without value and just risk.
The right fix as far as I can see is to remove the tests although
WARN_ON() combined with your tty->ops change might be safer.
> It's pretty obvious that this should have been part of commit
> f34d7a5b7010 ("tty: The big operations rework"). That being said, these
It ahould probably have been fixed around the same time or in one of the
tty locking reviews, but drivers/isdn and net/irda weren't traditionally
part of the general tty maintenance but handled separately/
> test puzzle me. It's not obvious why they're needed. Ie, can the null
> dereferences they try to catch really happen? But I can try to figure
> out that in the future, if I ever feel the urge to do so. Anyhow:
>
> Acked-by: Paul Bolle <pebolle@tiscali.nl>
Nacked-by: Alan Cox <alan@linux.intel.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
2015-12-08 11:00 ` [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure Tilman Schmidt
@ 2015-12-08 23:12 ` Paul Bolle
2015-12-09 11:10 ` Tilman Schmidt
0 siblings, 1 reply; 18+ messages in thread
From: Paul Bolle @ 2015-12-08 23:12 UTC (permalink / raw)
To: Tilman Schmidt, netdev
Cc: Peter Hurley, Sasha Levin, syzkaller, David Miller, Karsten Keil,
isdn4linux, gigaset307x-common, linux-kernel
Hi Tilman,
On di, 2015-12-08 at 12:00 +0100, Tilman Schmidt 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>
> ---
> 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 d8771b5..2693cb2 100644
> --- a/drivers/isdn/gigaset/ser-gigaset.c
> +++ b/drivers/isdn/gigaset/ser-gigaset.c
> @@ -370,19 +370,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);
dev equals cs->hw.ser->dev.dev, doesn't it? So what does setting
cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy us?
> + kfree(cs->hw.ser);
> + cs->hw.ser = NULL;
I might be missing something, but what does setting this to NULL buy us
here?
(I realize that I'm asking questions to code that isn't actually new but
only moved around, but I think that's still an opportunity to have
another look at that code.)
> }
>
> /*
Thanks,
Paul Bolle
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method
2015-12-08 11:00 ` [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method Tilman Schmidt
@ 2015-12-08 23:15 ` Paul Bolle
0 siblings, 0 replies; 18+ messages in thread
From: Paul Bolle @ 2015-12-08 23:15 UTC (permalink / raw)
To: Tilman Schmidt, netdev
Cc: Peter Hurley, Sasha Levin, syzkaller, David Miller, Karsten Keil,
isdn4linux, gigaset307x-common, linux-kernel
Hi Tilman,
On di, 2015-12-08 at 12:00 +0100, Tilman Schmidt wrote:
> device->platform_data and platform_device->resource are never used
> and remain NULL through their entire life. Drops the kfree() calls
> for them from the device release method.
>
> Signed-off-by: Tilman Schmidt <tilman@imap.cc>
> Reported-by: Paul Bolle <pebolle@tiscali.nl>
s/Reported-by/Acked-by/
(Having both lines would be overdoing things.)
Thanks,
Paul Bolle
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] ser_gigaset: fix up NULL checks
2015-12-08 22:16 ` One Thousand Gnomes
@ 2015-12-09 10:45 ` Tilman Schmidt
2015-12-09 12:12 ` One Thousand Gnomes
0 siblings, 1 reply; 18+ messages in thread
From: Tilman Schmidt @ 2015-12-09 10:45 UTC (permalink / raw)
To: One Thousand Gnomes, Paul Bolle
Cc: netdev, Peter Hurley, Sasha Levin, syzkaller, David Miller,
Karsten Keil, isdn4linux, gigaset307x-common, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]
Am 08.12.2015 um 23:16 schrieb One Thousand Gnomes:
> The right fix as far as I can see is to remove the tests although
> WARN_ON() combined with your tty->ops change might be safer.
Feel free to submit a patch.
>> It's pretty obvious that this should have been part of commit
>> f34d7a5b7010 ("tty: The big operations rework"). That being said, these
>
> It ahould probably have been fixed around the same time or in one of the
> tty locking reviews, but drivers/isdn and net/irda weren't traditionally
> part of the general tty maintenance but handled separately/
Or just ignored.
>> test puzzle me. It's not obvious why they're needed. Ie, can the null
>> dereferences they try to catch really happen? But I can try to figure
>> out that in the future, if I ever feel the urge to do so. Anyhow:
>>
>> Acked-by: Paul Bolle <pebolle@tiscali.nl>
>
> Nacked-by: Alan Cox <alan@linux.intel.com>
So you feel it's better to maintain the current inconsistent state
created by commit f34d7a5b? Please elaborate.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
2015-12-08 23:12 ` Paul Bolle
@ 2015-12-09 11:10 ` Tilman Schmidt
2015-12-10 11:20 ` Paul Bolle
2015-12-10 14:04 ` Peter Hurley
0 siblings, 2 replies; 18+ messages in thread
From: Tilman Schmidt @ 2015-12-09 11:10 UTC (permalink / raw)
To: Paul Bolle, netdev
Cc: Peter Hurley, Sasha Levin, syzkaller, David Miller, Karsten Keil,
isdn4linux, gigaset307x-common, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]
Am 09.12.2015 um 00:12 schrieb Paul Bolle:
>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>> @@ -370,19 +370,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);
>
> dev equals cs->hw.ser->dev.dev, doesn't it?
Correct.
> So what does setting
> cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy us?
We're freeing cs->hw.ser, not cs->hw.ser->dev.
Clearing the reference to cs from the device structure before freeing cs
guards against possible use-after-free.
>> + kfree(cs->hw.ser);
>> + cs->hw.ser = NULL;
>
> I might be missing something, but what does setting this to NULL buy us
> here?
Just defensive programming. Guarding against possible use-after-free or
double-free.
>
> (I realize that I'm asking questions to code that isn't actually new but
> only moved around, but I think that's still an opportunity to have
> another look at that code.)
I'm a big fan of one change per patch. If we also want to modify the
moved code then that should be done in a separate patch. It makes
bisecting so much easier. Same reason why I separated out patch 3/3. And
btw same reason why I think patch 1/3 should go in as-is, as an obvious
fix to commit f34d7a5b, and any concerns about whether those tests are
useful should be addressed by a separate patch.
Regards,
Tilman
--
Tilman Schmidt E-Mail: tilman@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] ser_gigaset: fix up NULL checks
2015-12-09 10:45 ` Tilman Schmidt
@ 2015-12-09 12:12 ` One Thousand Gnomes
2015-12-09 19:18 ` Paul Bolle
0 siblings, 1 reply; 18+ messages in thread
From: One Thousand Gnomes @ 2015-12-09 12:12 UTC (permalink / raw)
To: Tilman Schmidt
Cc: Paul Bolle, netdev, Peter Hurley, Sasha Levin, syzkaller,
David Miller, Karsten Keil, isdn4linux, gigaset307x-common,
linux-kernel
On Wed, 9 Dec 2015 11:45:57 +0100
Tilman Schmidt <tilman@imap.cc> wrote:
> Am 08.12.2015 um 23:16 schrieb One Thousand Gnomes:
> > The right fix as far as I can see is to remove the tests although
> > WARN_ON() combined with your tty->ops change might be safer.
>
> Feel free to submit a patch.
Will do later today. Want a patch on top of Paul's change or a single
patch including both and crediting him ?
> > tty locking reviews, but drivers/isdn and net/irda weren't traditionally
> > part of the general tty maintenance but handled separately/
>
> Or just ignored.
Unfortunately at the time that seemed to happen a lot.
> >> test puzzle me. It's not obvious why they're needed. Ie, can the null
> >> dereferences they try to catch really happen? But I can try to figure
> >> out that in the future, if I ever feel the urge to do so. Anyhow:
> >>
> >> Acked-by: Paul Bolle <pebolle@tiscali.nl>
> >
> > Nacked-by: Alan Cox <alan@linux.intel.com>
>
> So you feel it's better to maintain the current inconsistent state
> created by commit f34d7a5b? Please elaborate.
No I'd rather we didn't make it look magically better then forget about
the mess in question.
Alan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] ser_gigaset: fix up NULL checks
2015-12-09 12:12 ` One Thousand Gnomes
@ 2015-12-09 19:18 ` Paul Bolle
0 siblings, 0 replies; 18+ messages in thread
From: Paul Bolle @ 2015-12-09 19:18 UTC (permalink / raw)
To: One Thousand Gnomes, Tilman Schmidt
Cc: netdev, Peter Hurley, Sasha Levin, syzkaller, David Miller,
Karsten Keil, isdn4linux, gigaset307x-common, linux-kernel
On wo, 2015-12-09 at 12:12 +0000, One Thousand Gnomes wrote:
> On Wed, 9 Dec 2015 11:45:57 +0100
> Tilman Schmidt <tilman@imap.cc> wrote:
> Want a patch on top of Paul's change or a single
> patch including both and crediting him ?
There's no change that can be attributed to me, I think. We're
discussing a series submitted by Tilman.
Confused,
Paul Bolle
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
2015-12-09 11:10 ` Tilman Schmidt
@ 2015-12-10 11:20 ` Paul Bolle
2015-12-10 14:04 ` Peter Hurley
1 sibling, 0 replies; 18+ messages in thread
From: Paul Bolle @ 2015-12-10 11:20 UTC (permalink / raw)
To: Tilman Schmidt, netdev
Cc: Peter Hurley, Sasha Levin, syzkaller, David Miller, Karsten Keil,
isdn4linux, gigaset307x-common, linux-kernel
On wo, 2015-12-09 at 12:10 +0100, Tilman Schmidt wrote:
> Am 09.12.2015 um 00:12 schrieb Paul Bolle:
> > So what does setting
> > cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy
> > us?
>
> We're freeing cs->hw.ser, not cs->hw.ser->dev.
> Clearing the reference to cs from the device structure before freeing
> cs guards against possible use-after-free.
>
> > > + kfree(cs->hw.ser);
> > > + cs->hw.ser = NULL;
> >
> > I might be missing something, but what does setting this to NULL buy
> > us here?
>
> Just defensive programming. Guarding against possible use-after-free
> or double-free.
I'm inclined to think this is not the best way to guard against such
nasty bugs. But then again, I'm only a few months into my shift of
looking after the gigaset drivers and haven't had to track down such
bugs yet. But I'd be surprised if many other drivers do it that way and
think this is a job for (tree wide) debugging tools. But, whatever the
merits of our views, we can defer this discussion to some future date.
See below.
> I'm a big fan of one change per patch. If we also want to modify the
> moved code then that should be done in a separate patch. It makes
> bisecting so much easier. Same reason why I separated out patch 3/3.
Fair enough.
Paul Bolle
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] ser_gigaset fixes
2015-12-08 11:00 [PATCH 0/3] ser_gigaset fixes Tilman Schmidt
` (2 preceding siblings ...)
2015-12-08 11:00 ` [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method Tilman Schmidt
@ 2015-12-10 11:31 ` Paul Bolle
2015-12-12 18:09 ` Tilman Schmidt
3 siblings, 1 reply; 18+ messages in thread
From: Paul Bolle @ 2015-12-10 11:31 UTC (permalink / raw)
To: Tilman Schmidt, netdev
Cc: Peter Hurley, Sasha Levin, syzkaller, David Miller, Karsten Keil,
isdn4linux, gigaset307x-common, linux-kernel
Hi Tilman,
On di, 2015-12-08 at 12:00 +0100, Tilman Schmidt wrote:
> this series is the result of our discussion on the "freeing an
> active object" bug. I split my proposed patch into two patches
> for the separate topics of moving the ser_cardstate kfree() and
> dropping the useless kfree()s, and also included an unrelated
> patch (1/3) that had fallen through the cracks in my last series.
>
> Patch 2/3 should go into stable releases all the way back to 2.6.32.
> It applies cleanly to release 3.*/4.* with at most offset 1.
> For release 2.6.32 there is a trivial merge conflict with a removed
> comment line.
1/3 ran into objections and, I think, Alan Cox is working on an
alternative for it. Would you mind resending 2/3 and 3/3 as a two
patches series? Feel free to add
Acked-by: Paul Bolle <pebolle@tiscali.nl>
to both.
(The previous gigaset series, which you sent in July this year, was
picked up from netdev directly by David Miller. Unless people actually
prefer these patches to also be signed-off by me, I'm perfectly fine
with that.)
Thanks,
Paul Bolle
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
2015-12-09 11:10 ` Tilman Schmidt
2015-12-10 11:20 ` Paul Bolle
@ 2015-12-10 14:04 ` Peter Hurley
2015-12-12 17:52 ` Tilman Schmidt
1 sibling, 1 reply; 18+ messages in thread
From: Peter Hurley @ 2015-12-10 14:04 UTC (permalink / raw)
To: Tilman Schmidt, Paul Bolle
Cc: netdev, Sasha Levin, syzkaller, David Miller, Karsten Keil,
isdn4linux, gigaset307x-common, linux-kernel
Hi Tilman,
On 12/09/2015 03:10 AM, Tilman Schmidt wrote:
> Am 09.12.2015 um 00:12 schrieb Paul Bolle:
>
>>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>>> @@ -370,19 +370,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);
This is of marginal value and (I think) unnecessary; it implies
the core will use the device after release, which would trigger
many problems if true.
>> dev equals cs->hw.ser->dev.dev, doesn't it?
>
> Correct.
>
>> So what does setting
>> cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy us?
>
> We're freeing cs->hw.ser, not cs->hw.ser->dev.
> Clearing the reference to cs from the device structure before freeing cs
> guards against possible use-after-free.
>
>>> + kfree(cs->hw.ser);
>>> + cs->hw.ser = NULL;
This pattern is common, and defends against much more common
driver bugs.
Unfortunately, much of the good this pattern is intended to do in finding
use-after-free bugs is undone by explicit tests for null everywhere else.
Not saying that's the case here; rather, generally speaking.
Like the
if (!tty && !tty->ops && ....)
code.
Better just to let it crash.
Regards,
Peter Hurley
>> I might be missing something, but what does setting this to NULL buy us
>> here?
>
> Just defensive programming. Guarding against possible use-after-free or
> double-free.
>
>>
>> (I realize that I'm asking questions to code that isn't actually new but
>> only moved around, but I think that's still an opportunity to have
>> another look at that code.)
>
> I'm a big fan of one change per patch. If we also want to modify the
> moved code then that should be done in a separate patch. It makes
> bisecting so much easier. Same reason why I separated out patch 3/3. And
> btw same reason why I think patch 1/3 should go in as-is, as an obvious
> fix to commit f34d7a5b, and any concerns about whether those tests are
> useful should be addressed by a separate patch.
>
> Regards,
> Tilman
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure
2015-12-10 14:04 ` Peter Hurley
@ 2015-12-12 17:52 ` Tilman Schmidt
0 siblings, 0 replies; 18+ messages in thread
From: Tilman Schmidt @ 2015-12-12 17:52 UTC (permalink / raw)
To: Peter Hurley
Cc: Paul Bolle, netdev, Sasha Levin, syzkaller, David Miller,
Karsten Keil, isdn4linux, gigaset307x-common, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]
Hi Peter,
Am 10.12.2015 um 15:04 schrieb Peter Hurley:
>>>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>>>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>>>> @@ -370,19 +370,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);
>
> This is of marginal value and (I think) unnecessary; it implies
> the core will use the device after release, which would trigger
> many problems if true.
Agreed, but I'm just moving existing code here. Dropping the
dev_set_drvdata() call would be an unrelated change which should be done
in a separate patch if I understand the rules correctly.
Regards,
Tilman
--
Tilman Schmidt E-Mail: tilman@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] ser_gigaset fixes
2015-12-10 11:31 ` [PATCH 0/3] ser_gigaset fixes Paul Bolle
@ 2015-12-12 18:09 ` Tilman Schmidt
2015-12-12 18:32 ` Paul Bolle
0 siblings, 1 reply; 18+ messages in thread
From: Tilman Schmidt @ 2015-12-12 18:09 UTC (permalink / raw)
To: Paul Bolle, netdev
Cc: Peter Hurley, Sasha Levin, syzkaller, David Miller, Karsten Keil,
isdn4linux, gigaset307x-common, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]
Hi Paul,
Am 10.12.2015 um 12:31 schrieb Paul Bolle:
> 1/3 ran into objections and, I think, Alan Cox is working on an
> alternative for it. Would you mind resending 2/3 and 3/3 as a two
> patches series? Feel free to add
> Acked-by: Paul Bolle <pebolle@tiscali.nl>
>
> to both.
Alan's patch dated 09 Dec 2015 20:33:24 applies on top of 1/3 so I
reckon it should be kept.
> (The previous gigaset series, which you sent in July this year, was
> picked up from netdev directly by David Miller. Unless people actually
> prefer these patches to also be signed-off by me, I'm perfectly fine
> with that.)
I think as a maintainer you are supposed to sign off patches for the
code you maintain. My signed-off as recently retired ex-maintainer was
probably still good enough in July. I'm unsure whether it still is today.
Regards,
Tilman
--
Tilman Schmidt E-Mail: tilman@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] ser_gigaset fixes
2015-12-12 18:09 ` Tilman Schmidt
@ 2015-12-12 18:32 ` Paul Bolle
0 siblings, 0 replies; 18+ messages in thread
From: Paul Bolle @ 2015-12-12 18:32 UTC (permalink / raw)
To: Tilman Schmidt, netdev
Cc: Peter Hurley, Sasha Levin, syzkaller, David Miller, Karsten Keil,
isdn4linux, gigaset307x-common, linux-kernel
Hi Tilman,
On za, 2015-12-12 at 19:09 +0100, Tilman Schmidt wrote:
> Am 10.12.2015 um 12:31 schrieb Paul Bolle:
> > 1/3 ran into objections and, I think, Alan Cox is working on an
> > alternative for it. Would you mind resending 2/3 and 3/3 as a two
> > patches series? Feel free to add
> > Acked-by: Paul Bolle <pebolle@tiscali.nl>
> >
> > to both.
>
> Alan's patch dated 09 Dec 2015 20:33:24 applies on top of 1/3 so I
> reckon it should be kept.
Which apparently never hit my INBOX. If it had, we wouldn't have been
discussing my request to resend. Thanks for pointing me at that patch!
> > (The previous gigaset series, which you sent in July this year, was
> > picked up from netdev directly by David Miller. Unless people
> > actually
> > prefer these patches to also be signed-off by me, I'm perfectly fine
> > with that.)
>
> I think as a maintainer you are supposed to sign off patches for the
> code you maintain. My signed-off as recently retired ex-maintainer was
> probably still good enough in July. I'm unsure whether it still is
> today.
So I'll jot you down as someone preferring that I sign off.
I'll take your series, and Alan's patch - which you just forwarded to me
in a separate message - and kick them around a bit more. Then I expect
to send everything to netdev by next Monday.
Thanks,
Paul Bolle
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-12-12 18:32 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-08 11:00 [PATCH 0/3] ser_gigaset fixes Tilman Schmidt
2015-12-08 11:00 ` [PATCH 1/3] ser_gigaset: fix up NULL checks Tilman Schmidt
2015-12-08 19:45 ` Paul Bolle
2015-12-08 22:16 ` One Thousand Gnomes
2015-12-09 10:45 ` Tilman Schmidt
2015-12-09 12:12 ` One Thousand Gnomes
2015-12-09 19:18 ` Paul Bolle
2015-12-08 11:00 ` [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure Tilman Schmidt
2015-12-08 23:12 ` Paul Bolle
2015-12-09 11:10 ` Tilman Schmidt
2015-12-10 11:20 ` Paul Bolle
2015-12-10 14:04 ` Peter Hurley
2015-12-12 17:52 ` Tilman Schmidt
2015-12-08 11:00 ` [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method Tilman Schmidt
2015-12-08 23:15 ` Paul Bolle
2015-12-10 11:31 ` [PATCH 0/3] ser_gigaset fixes Paul Bolle
2015-12-12 18:09 ` Tilman Schmidt
2015-12-12 18:32 ` Paul Bolle
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.