[PATCH 0/3] ser_gigaset fixes

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

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 <redacted>
Reported-by: Paul Bolle <redacted>
---
 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

Commit f34d7a5b changed tty->driver to tty->ops but left NULL checks
for tty->driver untouched. Fix.

Signed-off-by: Tilman Schmidt <redacted>
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

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 <redacted>
Fixes: 2869b23e4b95 ("drivers/isdn/gigaset: new M101 driver (v2)")
Reported-by: Sasha Levin <redacted>
---
 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

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 <redacted>
Fixes: f34d7a5b7010 ("tty: The big operations rework")

Should we backport this all the way to v2.6.32 (currently the oldest
stable tree)?

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 <redacted>


Paul Bolle

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 <redacted>

Nacked-by: Alan Cox [off-list ref]

Hi Tilman,

On di, 2015-12-08 at 12:00 +0100, Tilman Schmidt wrote:

1 more quoted lines

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

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 <redacted>
Reported-by: Paul Bolle <redacted>

s/Reported-by/Acked-by/

(Having both lines would be overdoing things.)

Thanks,


Paul Bolle

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.

quoted

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.

quoted

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 <redacted>

Nacked-by: Alan Cox [off-list ref]

So you feel it's better to maintain the current inconsistent state
created by commit f34d7a5b? Please elaborate.

Am 09.12.2015 um 00:12 schrieb Paul Bolle:

quoted

--- 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.

quoted

+	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.

On Wed, 9 Dec 2015 11:45:57 +0100
Tilman Schmidt [off-list ref] wrote:

Am 08.12.2015 um 23:16 schrieb One Thousand Gnomes:

quoted

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 ?

quoted

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.

quoted

quoted

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 <redacted>

Nacked-by: Alan Cox [off-list ref]

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

On wo, 2015-12-09 at 12:12 +0000, One Thousand Gnomes wrote:

On Wed, 9 Dec 2015 11:45:57 +0100
Tilman Schmidt [off-list ref] 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

On wo, 2015-12-09 at 12:10 +0100, Tilman Schmidt wrote:

Am 09.12.2015 um 00:12 schrieb Paul Bolle:

quoted

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.

quoted

quoted

+	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

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 [off-list ref]

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

Hi Tilman,

On 12/09/2015 03:10 AM, Tilman Schmidt wrote:

Am 09.12.2015 um 00:12 schrieb Paul Bolle:

quoted

quoted

--- 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.

quoted

dev equals cs->hw.ser->dev.dev, doesn't it?

Correct.

quoted

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.

quoted

quoted

+	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

quoted

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.

quoted

(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

Hi Peter,

Am 10.12.2015 um 15:04 schrieb Peter Hurley:

quoted

quoted

quoted

--- 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.

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 [off-list ref]

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.

Hi Tilman,

On za, 2015-12-12 at 19:09 +0100, Tilman Schmidt wrote:

Am 10.12.2015 um 12:31 schrieb Paul Bolle:

quoted

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 [off-list ref]

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!

quoted

(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