[PATCH] tty: make sure a BUG is hit if tty_port will be destroyed before tty

[PATCH] tty: make sure a BUG is hit if tty_port will be destroyed before tty

[Alexander Holler]

tty depends on tty_port until tty_release() was called. Make sure a BUG
will be hit, if tty_port will be destroyed before tty.

Signed-off-by: Alexander Holler <holler@ahsoftware.de>
Cc: Peter Hurley <peter@hurleysoftware.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: <stable@vger.kernel.org>
---

 Currently things are changing fast in the tty subsystem, therefor I don't
 know if the patch should be applied to kernel 3.10 too because the
 reference to tty_port in tty_ldisc_halt() is gone in 3.10-rc1.

 So it might be a patch only for the stable kernels since commit
 ecbbfd4 (kernels 3.8 and 3.9).

 drivers/tty/tty_port.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 121aeb9..a40c52b 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -140,6 +140,8 @@ EXPORT_SYMBOL(tty_port_destroy);
 static void tty_port_destructor(struct kref *kref)
 {
 	struct tty_port *port = container_of(kref, struct tty_port, kref);
+	/* tty_port has to live until tty_release() was called. */
+	BUG_ON(port->itty);
 	if (port->xmit_buf)
 		free_page((unsigned long)port->xmit_buf);
 	tty_port_destroy(port);
-- 
1.8.1.4

Re: [PATCH] tty: make sure a BUG is hit if tty_port will be destroyed before tty

[Greg Kroah-Hartman]

On Fri, May 17, 2013 at 09:12:08AM +0200, Alexander Holler wrote:
> tty depends on tty_port until tty_release() was called. Make sure a BUG
> will be hit, if tty_port will be destroyed before tty.

So you want to ensure that we crash a machine?  No, please never add
BUG() statements to the kernel, unless something _really_ bad is going
to happen if we don't call it.  I never want to stop a machine from
running, do you?

I can't take this as-is, why not just fix the root problem?

thanks,

greg k-h

Re: [PATCH] tty: make sure a BUG is hit if tty_port will be destroyed before tty

[Alexander Holler]

Am 17.05.2013 17:31, schrieb Greg Kroah-Hartman:
> On Fri, May 17, 2013 at 09:12:08AM +0200, Alexander Holler wrote:
>> tty depends on tty_port until tty_release() was called. Make sure a BUG
>> will be hit, if tty_port will be destroyed before tty.
>
> So you want to ensure that we crash a machine?  No, please never add

Exactly. Let me quote myself:

 >> As described before, it ends up with memory corruption because freed
 >> memory is used, so if a BUG() happens, it doesn't help much. E.g. with
 >> kernel 3.9.2 I never have seen a bug, just a rebooting machine
 >> (sometimes minutes after the real bug happened).

> BUG() statements to the kernel, unless something _really_ bad is going
> to happen if we don't call it.  I never want to stop a machine from
> running, do you?

Yes. I'm not sure how you define _really_ bad, but a memory corruption 
with undefined result is exactly how I would define such.

And in the case of rfcomm, the box doesn't stop, at least not here. Just 
the process is killed together with an easy to identfiy oops. And the 
BUG_ON() prevents that memory will become corrupted and the machine is 
still usable afterwards. If that isn't a use case for BUG_ON(), I really 
don't know what else would be a use case for it.

> I can't take this as-is, why not just fix the root problem?

First I'm still not sure about the root problem and awaiting some 
response to my mail before that patch. As noted in the mail with the 
patch, 3.10-rc1 looks different, so the it might already be fixed there, 
even if rfcomm doesn't handle the tty as it (now in 3.8 and 3.9) should 
be (I haven't tested 3.10-rc1 up to now).

Second, if I would fix the bug in rfcomm, as Peter suggested, I still 
would not know if the same problem doesn't appear in any other user of 
ttys too, so even if I would fix rfcomm, I still would want that 
BUG_ON() to make sure I don't get a memory corruption whenever another 
similiar bug is hit.

Regards,

Alexander Holler

Re: [PATCH] tty: make sure a BUG is hit if tty_port will be destroyed before tty

[Peter Hurley]

On 05/17/2013 12:41 PM, Alexander Holler wrote:
> Am 17.05.2013 17:31, schrieb Greg Kroah-Hartman:
>> On Fri, May 17, 2013 at 09:12:08AM +0200, Alexander Holler wrote:
>>> tty depends on tty_port until tty_release() was called. Make sure a BUG
>>> will be hit, if tty_port will be destroyed before tty.
>>
>> So you want to ensure that we crash a machine?  No, please never add
>
> Exactly. Let me quote myself:
>
>  >> As described before, it ends up with memory corruption because freed
>  >> memory is used, so if a BUG() happens, it doesn't help much. E.g. with
>  >> kernel 3.9.2 I never have seen a bug, just a rebooting machine
>  >> (sometimes minutes after the real bug happened).
>
>> BUG() statements to the kernel, unless something _really_ bad is going
>> to happen if we don't call it.  I never want to stop a machine from
>> running, do you?
>
> Yes. I'm not sure how you define _really_ bad, but a memory corruption with undefined result is exactly how I would define such.

First, I like the idea of a diagnostic here. But I'm with Greg on
this; BUG() is overkill. Just because the specific path which you found
only kills the process doesn't mean that other callers might not
prompt machine halt.

The memory corruption happens as a result of the tty_port being freed
by tty_port_destructor(). So a suitable diagnostic is to detect the
condition, WARN, and return without actually performing the destroy, yes?

> And in the case of rfcomm, the box doesn't stop, at least not here. Just the process is killed together with an easy to identfiy oops. And the BUG_ON() prevents that memory will become corrupted and the machine is still usable afterwards. If that isn't a use case for BUG_ON(), I really don't know what else would be a use case for it.
>
>> I can't take this as-is, why not just fix the root problem?
>
> First I'm still not sure about the root problem and awaiting some response to my mail before that patch. As noted in the mail with the patch, 3.10-rc1 looks different, so the it might already be fixed there, even if rfcomm doesn't handle the tty as it (now in 3.8 and 3.9) should be (I haven't tested 3.10-rc1 up to now).

This problem exists from the commit identified up through current
mainline. It has not been fixed yet. I'll specifically respond to what
I believe is the correct solution in that thread.

> Second, if I would fix the bug in rfcomm, as Peter suggested, I still would not know if the same problem doesn't appear in any other user of ttys too, so even if I would fix rfcomm, I still would want that BUG_ON() to make sure I don't get a memory corruption whenever another similiar bug is hit.

Only a handful of tty drivers use the ref-counted destroy method of
tty_port cleanup (the other method being calling tty_port_destroy()
directly). They are:

   drivers/tty/pty.c
   drivers/mmc/card/sdio_uart.c
   drivers/idsn/capi/capi.c
   drivers/usb/class/cdc-acm.c
   drivers/tty/n_gsm.c
   drivers/tty/hvc/hvc_console.c
   drivers/tty/hvc/hvcs.c
   net/bluetooth/rfcomm/tty.c

Of these, I know and have reviewed the first two drivers; their
usage is correct.

While reviewing these additional drivers, maybe we should review
whether it makes sense to
   * require and document the tty_port to live at least
     to ops->cleanup() (currently the default)
   * allow tty_port lifetime to be completely independent of
     a tty's lifetime
   * remove ref-counting from tty_port

Regards,
Peter Hurley

Re: [PATCH] tty: make sure a BUG is hit if tty_port will be destroyed before tty

[Alexander Holler]

Am 17.05.2013 20:06, schrieb Peter Hurley:
> On 05/17/2013 12:41 PM, Alexander Holler wrote:
>> Am 17.05.2013 17:31, schrieb Greg Kroah-Hartman:
>>> On Fri, May 17, 2013 at 09:12:08AM +0200, Alexander Holler wrote:
>>>> tty depends on tty_port until tty_release() was called. Make sure a BUG
>>>> will be hit, if tty_port will be destroyed before tty.
>>>
>>> So you want to ensure that we crash a machine?  No, please never add
>>
>> Exactly. Let me quote myself:
>>
>>  >> As described before, it ends up with memory corruption because freed
>>  >> memory is used, so if a BUG() happens, it doesn't help much. E.g.
>> with
>>  >> kernel 3.9.2 I never have seen a bug, just a rebooting machine
>>  >> (sometimes minutes after the real bug happened).
>>
>>> BUG() statements to the kernel, unless something _really_ bad is going
>>> to happen if we don't call it.  I never want to stop a machine from
>>> running, do you?
>>
>> Yes. I'm not sure how you define _really_ bad, but a memory corruption
>> with undefined result is exactly how I would define such.
> 
> First, I like the idea of a diagnostic here. But I'm with Greg on
> this; BUG() is overkill. Just because the specific path which you found
> only kills the process doesn't mean that other callers might not
> prompt machine halt.
> 
> The memory corruption happens as a result of the tty_port being freed
> by tty_port_destructor(). So a suitable diagnostic is to detect the
> condition, WARN, and return without actually performing the destroy, yes?
> 
>> And in the case of rfcomm, the box doesn't stop, at least not here.
>> Just the process is killed together with an easy to identfiy oops. And
>> the BUG_ON() prevents that memory will become corrupted and the
>> machine is still usable afterwards. If that isn't a use case for
>> BUG_ON(), I really don't know what else would be a use case for it.

Sorry, I didn't express it such, that this can't be misunderstood.
Without that BUG_ON() in my proposed patch, my boxes always died
afterwards. And that with a lot of different results before. Sometimes
nothing happened and the machine just rebooted, sometimes I've just seen
a warn_slowpath before the machine stopped/rebooted, sometimes I've just
got the BUG I posted in a previous mail, and often I've seen many OOPSes
before the machine rebooted. But in every case, the machine died unexpectly.

The case that the machine didn't die, but just the process, only happens
when my proposed patch is applied, which prevents the memory corruption.

Regards,

Alexander Holler

Re: [PATCH] tty: make sure a BUG is hit if tty_port will be destroyed before tty

[Alexander Holler]

Am 17.05.2013 21:22, schrieb Alexander Holler:

> The case that the machine didn't die, but just the process, only happens
> when my proposed patch is applied, which prevents the memory corruption.

In short, the proposed BUG_ON() prevents the memory corruption because
it is hit before something bad can happen. The result is that just the
process in question will be killed (and a tty is not released), but only
that BUG_ON() prevents that something _really_ bad happens.

I hope I could describe it now clearly. ;)

Regards,

Alexander

Re: [PATCH] tty: make sure a BUG is hit if tty_port will be destroyed before tty

[Peter Hurley]

On 05/17/2013 03:43 PM, Alexander Holler wrote:
> Am 17.05.2013 21:22, schrieb Alexander Holler:
>
>> The case that the machine didn't die, but just the process, only happens
>> when my proposed patch is applied, which prevents the memory corruption.
>
> In short, the proposed BUG_ON() prevents the memory corruption because
> it is hit before something bad can happen. The result is that just the
> process in question will be killed (and a tty is not released), but only
> that BUG_ON() prevents that something _really_ bad happens.
>
> I hope I could describe it now clearly. ;)

Your descriptions have been clear and I understood your meaning. However,
I think you may have misunderstood my suggestion.

Would you please test the patch below?

--- >% ---
Subject: [PATCH] tty: Prevent tty_port destruction if tty not released

If the tty driver mistakenly drops the last port reference
before the tty has been released, issue a diagnostic and
abort the port destruction.

This will leak memory and may zombify the port, but should
otherwise keep the machine in runnable state.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
  drivers/tty/tty_port.c | 4 ++++
  1 file changed, 4 insertions(+)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 6d9e0b2..a4f4fa9 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -140,6 +140,10 @@ EXPORT_SYMBOL(tty_port_destroy);
  static void tty_port_destructor(struct kref *kref)
  {
  	struct tty_port *port = container_of(kref, struct tty_port, kref);
+
+	/* check if last port ref was dropped before tty release */
+	if (WARN_ON(port->itty))
+		return;
  	if (port->xmit_buf)
  		free_page((unsigned long)port->xmit_buf);
  	tty_port_destroy(port);
-- 
1.8.1.2

Re: [PATCH] tty: make sure a BUG is hit if tty_port will be destroyed before tty

[Alexander Holler]

Am 18.05.2013 00:51, schrieb Peter Hurley:
> On 05/17/2013 03:43 PM, Alexander Holler wrote:
>> Am 17.05.2013 21:22, schrieb Alexander Holler:
>>
>>> The case that the machine didn't die, but just the process, only happens
>>> when my proposed patch is applied, which prevents the memory corruption.
>>
>> In short, the proposed BUG_ON() prevents the memory corruption because
>> it is hit before something bad can happen. The result is that just the
>> process in question will be killed (and a tty is not released), but only
>> that BUG_ON() prevents that something _really_ bad happens.
>>
>> I hope I could describe it now clearly. ;)
>
> Your descriptions have been clear and I understood your meaning. However,
> I think you may have misunderstood my suggestion.
>
> Would you please test the patch below?
>
> --- >% ---
> Subject: [PATCH] tty: Prevent tty_port destruction if tty not released
>
> If the tty driver mistakenly drops the last port reference
> before the tty has been released, issue a diagnostic and
> abort the port destruction.
>
> This will leak memory and may zombify the port, but should
> otherwise keep the machine in runnable state.
>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>   drivers/tty/tty_port.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index 6d9e0b2..a4f4fa9 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -140,6 +140,10 @@ EXPORT_SYMBOL(tty_port_destroy);
>   static void tty_port_destructor(struct kref *kref)
>   {
>       struct tty_port *port = container_of(kref, struct tty_port, kref);
> +
> +    /* check if last port ref was dropped before tty release */
> +    if (WARN_ON(port->itty))
> +        return;
>       if (port->xmit_buf)
>           free_page((unsigned long)port->xmit_buf);
>       tty_port_destroy(port);

I don't have to test this, I see what will happen. Sorry, but I'm 
exhausted and need a break dealing with lkml and maintainers.

Regards,

Alexander Holler