Re: [stable] Success: tty_io flush_to_ldisc() error message triggered

Re: [stable] Success: tty_io flush_to_ldisc() error message triggered

[Chuck Ebbert]

In-Reply-To: <20060725184158.GH9021@kroah.com>

On Tue, 25 Jul 2006 11:41:58 -0700, Greg KH wrote:
> 
> > > Is this simpler change (what I'm running but without the warning
> > > messages) the preferred fix for -stable?
> > 
> > It fixes the problem.
>
> So do you feel this patch should be added to the -stable kernel tree?

I think it's the right fix.

        1.  It fixes a real bug and that's been verified by testing.
        2.  It's the simplest change that does so. (The fix in 2.6.18-rc
            touches a lot of code.)

-- 
Chuck

Re: [stable] Success: tty_io flush_to_ldisc() error message triggered

[Paul Fulghum]

Chuck Ebbert wrote:
> In-Reply-To: <20060725184158.GH9021@kroah.com>
> 
> On Tue, 25 Jul 2006 11:41:58 -0700, Greg KH wrote:
> 
>>>>Is this simpler change (what I'm running but without the warning
>>>>messages) the preferred fix for -stable?
>>>
>>>It fixes the problem.
>>
>>So do you feel this patch should be added to the -stable kernel tree?
> 
> 
> I think it's the right fix.
> 
>         1.  It fixes a real bug and that's been verified by testing.
>         2.  It's the simplest change that does so. (The fix in 2.6.18-rc
>             touches a lot of code.)

OK, I have no objections (it saves me time).
I'll let you guys decide.

-- 
Paul Fulghum
Microgate Systems, Ltd.

Re: Success: tty_io flush_to_ldisc() error message triggered

[Chuck Ebbert]

In-Reply-To: <44C2307C.9060607@microgate.com>

On Sat, 22 Jul 2006 09:04:44 -0500, Paul Fulghum wrote:

> That confirms my thoughts on what went wrong:
> multiple copies of the queued work (flush_to_ldisc)
> running in parallel and corrupting the free buffer list.
> 
> A cleaner fix for this is already
> in the 2.6.18 rc series.

The cleaner fix looks more intrusive, though.

Is this simpler change (what I'm running but without the warning
messages) the preferred fix for -stable?


From: Paul Fulghum <paulkf@microgate.com>

Serialize flush_to_ldisc() per-device. Fixes free list corruption
that causes lockup on SMP systems.

Signed-off-by: Paul Fulghum <paulkf@microgate.com>
Acked-by: Chuck Ebbert <76306.1226@compuserve.com>

--- 2.6.16.20-d4.orig/include/linux/tty.h
+++ 2.6.16.20-d4/include/linux/tty.h
@@ -266,6 +266,7 @@ struct tty_struct {
 #define TTY_PTY_LOCK 		16	/* pty private */
 #define TTY_NO_WRITE_SPLIT 	17	/* Preserve write boundaries to driver */
 #define TTY_HUPPED 		18	/* Post driver->hangup() */
+#define TTY_FLUSHING 		19	/* Flushing tty buffers to line discipline */
 
 #define TTY_WRITE_FLUSH(tty) tty_write_flush((tty))
 
--- 2.6.16.20-d4.orig/drivers/char/tty_io.c
+++ 2.6.16.20-d4/drivers/char/tty_io.c
@@ -2780,10 +2780,8 @@ static void flush_to_ldisc(void *private
 	if (disc == NULL)	/*  !TTY_LDISC */
 		return;
 
-	if (test_bit(TTY_DONT_FLIP, &tty->flags)) {
-		/*
-		 * Do it after the next timer tick:
-		 */
+	if (test_bit(TTY_DONT_FLIP, &tty->flags) ||
+	    test_and_set_bit(TTY_FLUSHING, &tty->flags)) {
 		schedule_delayed_work(&tty->buf.work, 1);
 		goto out;
 	}
@@ -2805,6 +2803,7 @@ static void flush_to_ldisc(void *private
 		tty_buffer_free(tty, tbuf);
 	}
 	spin_unlock_irqrestore(&tty->buf.lock, flags);
+	clear_bit(TTY_FLUSHING, &tty->flags);
 out:
 	tty_ldisc_deref(disc);
 }
-- 
Chuck

Re: Success: tty_io flush_to_ldisc() error message triggered

[Paul Fulghum]

Chuck Ebbert wrote:
> The cleaner fix looks more intrusive, though.
> 
> Is this simpler change (what I'm running but without the warning
> messages) the preferred fix for -stable?

It fixes the problem.

The ugly thing about that patch is adding
another TTY_XXX macro that is not really necessary.
I created it as a quick and dirty way of testing
my hypothesis about the problem.

I don't see a problem using it as a simple
(albeit naive) approach to fix stable.

--
Paul Fulghum
Microgate Systems, Ltd

Re: [stable] Success: tty_io flush_to_ldisc() error message triggered

[Greg KH]

On Sat, Jul 22, 2006 at 11:41:44AM -0500, Paul Fulghum wrote:
> Chuck Ebbert wrote:
> > The cleaner fix looks more intrusive, though.
> > 
> > Is this simpler change (what I'm running but without the warning
> > messages) the preferred fix for -stable?
> 
> It fixes the problem.

So do you feel this patch should be added to the -stable kernel tree?

thanks,

greg k-h

Re: [stable] Success: tty_io flush_to_ldisc() error message triggered

[Paul Fulghum]

Greg KH wrote:
> On Sat, Jul 22, 2006 at 11:41:44AM -0500, Paul Fulghum wrote:
> 
>>Chuck Ebbert wrote:
>>
>>>The cleaner fix looks more intrusive, though.
>>>
>>>Is this simpler change (what I'm running but without the warning
>>>messages) the preferred fix for -stable?
>>
>>It fixes the problem.
> 
> 
> So do you feel this patch should be added to the -stable kernel tree?

No. Now that I think about it, adding that extra
macro is just wrong even if temporary.

The real fix is equally simple, but in 2.6.18-rc
it is intertwined with other more intrusive changes.

Let me make a new separate patch that does things
the right way, which is simply removing the list
head while processing the list so two instances
to not trip over each other. I would have done so
earlier, but I've been insanely busy with multiple
work related deadlines (lame excuse I know).

I should post something tomorrow afternoon.

-- 
Paul Fulghum
Microgate Systems, Ltd.

Re: [stable] Success: tty_io flush_to_ldisc() error message triggered

[Greg KH]

On Tue, Jul 25, 2006 at 02:12:28PM -0500, Paul Fulghum wrote:
> Greg KH wrote:
> > On Sat, Jul 22, 2006 at 11:41:44AM -0500, Paul Fulghum wrote:
> > 
> >>Chuck Ebbert wrote:
> >>
> >>>The cleaner fix looks more intrusive, though.
> >>>
> >>>Is this simpler change (what I'm running but without the warning
> >>>messages) the preferred fix for -stable?
> >>
> >>It fixes the problem.
> > 
> > 
> > So do you feel this patch should be added to the -stable kernel tree?
> 
> No. Now that I think about it, adding that extra
> macro is just wrong even if temporary.
> 
> The real fix is equally simple, but in 2.6.18-rc
> it is intertwined with other more intrusive changes.
> 
> Let me make a new separate patch that does things
> the right way, which is simply removing the list
> head while processing the list so two instances
> to not trip over each other. I would have done so
> earlier, but I've been insanely busy with multiple
> work related deadlines (lame excuse I know).
> 
> I should post something tomorrow afternoon.

Ok, we can wait, I'd rather have the proper fix instead of the band-aid.

Just send it to stable@kernel.org when you have something that you feel
comfortable with.

thanks,

greg k-h