[PATCH] tty: Add missing lock in n_tty_write()

[PATCH] tty: Add missing lock in n_tty_write()

[Joerg Roedel]

The call of the tty->ops->write callback is protected by
output_lock in all other places I looked at. So it seems the
lock in this code-branch is missing, so take the output_lock
there too.

Cc: stable@vger.kernel.org
Signed-off-by: Joerg Roedel <joro@8bytes.org>
---
 drivers/tty/n_tty.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index d655416..ffb94a4 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -2058,10 +2058,14 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
 			if (tty->ops->flush_chars)
 				tty->ops->flush_chars(tty);
 		} else {
+			struct n_tty_data *l = tty->disc_data;
+
+			mutex_lock(&l->output_lock);
 			while (nr > 0) {
 				c = tty->ops->write(tty, b, nr);
 				if (c < 0) {
 					retval = c;
+					mutex_unlock(&l->output_lock);
 					goto break_out;
 				}
 				if (!c)
@@ -2069,6 +2073,7 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
 				b += c;
 				nr -= c;
 			}
+			mutex_unlock(&l->output_lock);
 		}
 		if (!nr)
 			break;
-- 
1.7.9.5

Re: [PATCH] tty: Add missing lock in n_tty_write()

[Jiri Slaby]

On 05/15/2013 12:56 PM, Joerg Roedel wrote:
> The call of the tty->ops->write callback is protected by
> output_lock in all other places I looked at. So it seems the
> lock in this code-branch is missing, so take the output_lock
> there too.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Joerg Roedel <joro@8bytes.org>
> ---
>  drivers/tty/n_tty.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index d655416..ffb94a4 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -2058,10 +2058,14 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
>  			if (tty->ops->flush_chars)
>  				tty->ops->flush_chars(tty);
>  		} else {
> +			struct n_tty_data *l = tty->disc_data;
> +
> +			mutex_lock(&l->output_lock);
>  			while (nr > 0) {
>  				c = tty->ops->write(tty, b, nr);
>  				if (c < 0) {
>  					retval = c;
> +					mutex_unlock(&l->output_lock);
>  					goto break_out;
>  				}
>  				if (!c)
> @@ -2069,6 +2073,7 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
>  				b += c;
>  				nr -= c;
>  			}
> +			mutex_unlock(&l->output_lock);
>  		}
>  		if (!nr)
>  			break;
> 

Are you fixing any bug here? output_lock does not protect
tty->ops->write on the other places, not tty->ops->write.

thanks,
-- 
js
suse labs

Re: [PATCH] tty: Add missing lock in n_tty_write()

[Joerg Roedel]

On Wed, May 15, 2013 at 05:03:17PM +0200, Jiri Slaby wrote:
> On 05/15/2013 12:56 PM, Joerg Roedel wrote:

> Are you fixing any bug here? output_lock does not protect
> tty->ops->write on the other places, not tty->ops->write.

Yes, I am trying to fix a BUG_ON that triggered in
drivers/tty/hvc/hvc_xen.c in function __write_console().  This function
was called from the place I am patching in this fix. My current
explanation for that BUG_ON is a race condition that comes
from concurrent calls to that function.

That is also the only explanation that makes sense because the
__write_console() function itself makes sure that the condition can not
hit.

In the comment for the n_tty_write function there is this remark:

 *      Locking: output_lock to protect column state and space left
 *               (note that the process_output*() functions take this
 *                lock themselves)

So the space left is managed in the ->write callback and needs
protection.

The process_output*() functions all (unless I am missing something)
take the output_lock before calling the tty->ops->write (directly or
indirectly).

The place I patched here is the only place in n_tty_write where the
->write call-back is invoked directly, and it happens without taking the
lock. I think this is a problem.


	Joerg

Re: [PATCH] tty: Add missing lock in n_tty_write()

[Peter Hurley]

On 05/15/2013 11:47 AM, Joerg Roedel wrote:
> On Wed, May 15, 2013 at 05:03:17PM +0200, Jiri Slaby wrote:
>> On 05/15/2013 12:56 PM, Joerg Roedel wrote:
>
>> Are you fixing any bug here? output_lock does not protect
>> tty->ops->write on the other places, not tty->ops->write.
>
> Yes, I am trying to fix a BUG_ON that triggered in
> drivers/tty/hvc/hvc_xen.c in function __write_console().  This function
> was called from the place I am patching in this fix. My current
> explanation for that BUG_ON is a race condition that comes
> from concurrent calls to that function.
>
> That is also the only explanation that makes sense because the
> __write_console() function itself makes sure that the condition can not
> hit.
>
> In the comment for the n_tty_write function there is this remark:
>
>   *      Locking: output_lock to protect column state and space left
>   *               (note that the process_output*() functions take this
>   *                lock themselves)
>
> So the space left is managed in the ->write callback and needs
> protection.

nack.

"space left" is not honored when OPOST is clear, so it is not protected
in this case. IOW, tty->ops->write_room() is not called, so by-definition
there is "space left".

Are you certain your stack trace takes you through this particular
invocation of tty->ops->write()?  Could it be that the compiler has
inlined process_output_block() into n_tty_write() and that's what your
seeing?

Can you attach the BUG report?
Are you certain OPOST is cleared? (output of stty -a -F </dev/xxxx>)
Is CONFIG_CONSOLE_POLL=y?
Is this happening during boot or much later?

> The process_output*() functions all (unless I am missing something)
> take the output_lock before calling the tty->ops->write (directly or
> indirectly).
>
> The place I patched here is the only place in n_tty_write where the
> ->write call-back is invoked directly, and it happens without taking the
> lock. I think this is a problem.

But not the only path to __write_console().

For example, what serializes hvc_console_print() with hvc_write()
for the same console index?

Regards,
Peter Hurley

Re: [PATCH] tty: Add missing lock in n_tty_write()

[Joerg Roedel]

(also adding Konrad)

On Wed, May 15, 2013 at 02:45:52PM -0400, Peter Hurley wrote:
> "space left" is not honored when OPOST is clear, so it is not protected
> in this case. IOW, tty->ops->write_room() is not called, so by-definition
> there is "space left".

Okay, so "space left" has to do with something tty-layer internal and
does not mean potential output-buffers handled by the console-drivers.

> Are you certain your stack trace takes you through this particular
> invocation of tty->ops->write()?  Could it be that the compiler has
> inlined process_output_block() into n_tty_write() and that's what your
> seeing?

I am sure that the backtrace pointed to that invocation. I looked up the
return-address from the stack-trace in the objdump and it pointed to
that line after that invocation.

> Can you attach the BUG report?
> Are you certain OPOST is cleared? (output of stty -a -F </dev/xxxx>)

Havn't checked OPOST. It is also hard to do because all I have is the
BUG and the kernel binary. I have no direct access to the machine.

> Is CONFIG_CONSOLE_POLL=y?

Will check.

> Is this happening during boot or much later?

Much later. It actually happened on a 3.2 kernel on a machine that ran
for several 100 days already. After that happened the box just rebooted
into a new kernel. I also checked the git-log from 3.2 to now and didn't
found a fix, also the code looks pretty similar so I guess the bug is
still there.

> But not the only path to __write_console().
> 
> For example, what serializes hvc_console_print() with hvc_write()
> for the same console index?

You are right, that does not look to be protected from each other. The
hvc_write() function has a spin_lock. But that does not prevent
hvc_console_print() from calling the put_chars function too.

I'll look something more into that. There is definitly a problem when
__write_console is called concurrently. I have one question about the
tty-layer: Do the console drivers have to expect parallel calls to
ops->write()?

Thanks,

	Joerg

Re: [PATCH] tty: Add missing lock in n_tty_write()

[Peter Hurley]

On 05/15/2013 03:48 PM, Joerg Roedel wrote:
> (also adding Konrad)
>
> On Wed, May 15, 2013 at 02:45:52PM -0400, Peter Hurley wrote:
>> "space left" is not honored when OPOST is clear, so it is not protected
>> in this case. IOW, tty->ops->write_room() is not called, so by-definition
>> there is "space left".
>
> Okay, so "space left" has to do with something tty-layer internal and
> does not mean potential output-buffers handled by the console-drivers.

Well, "space left" does mean 'potential output-buffers'.  However,
without OPOST, there is no output flow control as implemented through
the write_room() method. The driver is expected to write as much as
it can and return how much it wrote.

>> Are you certain your stack trace takes you through this particular
>> invocation of tty->ops->write()?  Could it be that the compiler has
>> inlined process_output_block() into n_tty_write() and that's what your
>> seeing?
>
> I am sure that the backtrace pointed to that invocation. I looked up the
> return-address from the stack-trace in the objdump and it pointed to
> that line after that invocation.

Ok.

But that implies that OPOST has been cleared (termios changed) which
doesn't really make sense for a console, which is why I asked.

>> Can you attach the BUG report?
>> Are you certain OPOST is cleared? (output of stty -a -F </dev/xxxx>)
>
> Havn't checked OPOST. It is also hard to do because all I have is the
> BUG and the kernel binary. I have no direct access to the machine.
>
>> Is CONFIG_CONSOLE_POLL=y?
>
> Will check.
>
>> Is this happening during boot or much later?
>
> Much later. It actually happened on a 3.2 kernel on a machine that ran
> for several 100 days already. After that happened the box just rebooted
> into a new kernel. I also checked the git-log from 3.2 to now and didn't
> found a fix, also the code looks pretty similar so I guess the bug is
> still there.
>
>> But not the only path to __write_console().
>>
>> For example, what serializes hvc_console_print() with hvc_write()
>> for the same console index?
>
> You are right, that does not look to be protected from each other. The
> hvc_write() function has a spin_lock. But that does not prevent
> hvc_console_print() from calling the put_chars function too.
>
> I'll look something more into that. There is definitly a problem when
> __write_console is called concurrently.

Agreed. Those functions look written for single-producer/single-consumer
i/o model. (That's why I asked about CONFIG_CONSOLE_POLL=y as well because
that doesn't look thread-safe either).

> I have one question about the
> tty-layer: Do the console drivers have to expect parallel calls to
> ops->write()?

Just to be clear here: there's a difference between a console driver
and a tty driver.

The console driver's write() method is serialized with the global
console_lock() so parallel console writes are not possible.

No such guarantee exists for the tty driver write() method, although it
probably wouldn't be difficult to provide that guarantee (since the
line discipline write() is already serialized by tty->atomic_write_lock).

Regards,
Peter Hurley

Re: [PATCH] tty: Add missing lock in n_tty_write()

[Joerg Roedel]

Hi Peter,

thanks for you explanations. They helped me to better understand what is
happening now.

On Wed, May 15, 2013 at 07:10:43PM -0400, Peter Hurley wrote:
> On 05/15/2013 03:48 PM, Joerg Roedel wrote:

> Agreed. Those functions look written for single-producer/single-consumer
> i/o model. (That's why I asked about CONFIG_CONSOLE_POLL=y as well because
> that doesn't look thread-safe either).

Ok, I checked that. CONFIG_CONSOLE_POLL is on in that kernel.

> Just to be clear here: there's a difference between a console driver
> and a tty driver.
> 
> The console driver's write() method is serialized with the global
> console_lock() so parallel console writes are not possible.
> 
> No such guarantee exists for the tty driver write() method, although it
> probably wouldn't be difficult to provide that guarantee (since the
> line discipline write() is already serialized by tty->atomic_write_lock).

Okay, so it is safe to say that currently the drivers write() (and
put_chars()) functions need to expect to be called concurrently and
therefore they have to serialize themselves when they need it, right?


	Joerg

Re: [PATCH] tty: Add missing lock in n_tty_write()

[Peter Hurley]

On 05/17/2013 07:48 AM, Joerg Roedel wrote:
> Hi Peter,
>
> thanks for you explanations. They helped me to better understand what is
> happening now.
>
> On Wed, May 15, 2013 at 07:10:43PM -0400, Peter Hurley wrote:
>> On 05/15/2013 03:48 PM, Joerg Roedel wrote:
>
>> Agreed. Those functions look written for single-producer/single-consumer
>> i/o model. (That's why I asked about CONFIG_CONSOLE_POLL=y as well because
>> that doesn't look thread-safe either).
>
> Ok, I checked that. CONFIG_CONSOLE_POLL is on in that kernel.
>
>> Just to be clear here: there's a difference between a console driver
>> and a tty driver.
>>
>> The console driver's write() method is serialized with the global
>> console_lock() so parallel console writes are not possible.
>>
>> No such guarantee exists for the tty driver write() method, although it
>> probably wouldn't be difficult to provide that guarantee (since the
>> line discipline write() is already serialized by tty->atomic_write_lock).
>
> Okay, so it is safe to say that currently the drivers write() (and
> put_chars()) functions need to expect to be called concurrently and
> therefore they have to serialize themselves when they need it, right?

If only it were that simple :)

Yes, console write() and tty write() can be concurrent. However, the
console write() can also be _recursive_ wrt. tty write(). This can happen,
for example, if something oopses in the tty write() path.

If you review serial8250_console_write() in drivers/tty/serial/8250/8250_core.c,
you'll see how some of this is worked around.

But looking at this from a wider perspective, the goal should be
to limit the overlap as much as possible.

Regards,
Peter Hurley