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

Groups > linux.kernel > #1286395 > unrolled thread

[PATCH 0/3] ser_gigaset fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] ser_gigaset fixes Tilman Schmidt <tilman@imap.cc> - 2015-12-08 12:20 +0100
    [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method Tilman Schmidt <tilman@imap.cc> - 2015-12-08 12:20 +0100
      Re: [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from  release method Paul Bolle <pebolle@tiscali.nl> - 2015-12-09 00:20 +0100
    [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure Tilman Schmidt <tilman@imap.cc> - 2015-12-08 12:20 +0100
      Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device  structure Paul Bolle <pebolle@tiscali.nl> - 2015-12-09 00:20 +0100
        Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device  structure Tilman Schmidt <tilman@imap.cc> - 2015-12-09 12:20 +0100
          Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device  structure Paul Bolle <pebolle@tiscali.nl> - 2015-12-10 12:30 +0100
          Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device  structure Peter Hurley <peter@hurleysoftware.com> - 2015-12-10 15:10 +0100
    [PATCH 1/3] ser_gigaset: fix up NULL checks Tilman Schmidt <tilman@imap.cc> - 2015-12-08 12:20 +0100
      Re: [PATCH 1/3] ser_gigaset: fix up NULL checks Paul Bolle <pebolle@tiscali.nl> - 2015-12-08 20:50 +0100
        Re: [PATCH 1/3] ser_gigaset: fix up NULL checks One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2015-12-08 23:20 +0100
          Re: [PATCH 1/3] ser_gigaset: fix up NULL checks Tilman Schmidt <tilman@imap.cc> - 2015-12-09 11:50 +0100
            Re: [PATCH 1/3] ser_gigaset: fix up NULL checks One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2015-12-09 13:20 +0100
              Re: [PATCH 1/3] ser_gigaset: fix up NULL checks Paul Bolle <pebolle@tiscali.nl> - 2015-12-09 20:20 +0100
    Re: [PATCH 0/3] ser_gigaset fixes Paul Bolle <pebolle@tiscali.nl> - 2015-12-10 12:40 +0100

#1286395 — [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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1286396 — [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1286913 — Re: [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1286400 — [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1286912 — Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1287421 — Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure

[Multipart message — attachments visible in raw view] — view raw

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.

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

#1288453 — Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1288521 — Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure

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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1286401 — [PATCH 1/3] ser_gigaset: fix up NULL checks

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1286802 — Re: [PATCH 1/3] ser_gigaset: fix up NULL checks

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1286892 — Re: [PATCH 1/3] ser_gigaset: fix up NULL checks

> 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>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1287410 — Re: [PATCH 1/3] ser_gigaset: fix up NULL checks

[Multipart message — attachments visible in raw view] — view raw

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.

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

#1287456 — Re: [PATCH 1/3] ser_gigaset: fix up NULL checks

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1287747 — Re: [PATCH 1/3] ser_gigaset: fix up NULL checks

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1288462

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web