Re: + n_tty-loss-of-sync-following-a-buffer-overflow.patch added to -mm tree - Kernel

This is a discussion on Re: + n_tty-loss-of-sync-following-a-buffer-overflow.patch added to -mm tree - Kernel ; On Wed, 2008-03-12 at 09:32 -0700, Rupesh Sugathan wrote: > The max line size from sender is 100 bytes. The read call waits for a > maximum of 200 bytes. OK, I understand what is happening. Multiple lines are received ...

+ Reply to Thread
Results 1 to 4 of 4

Thread: Re: + n_tty-loss-of-sync-following-a-buffer-overflow.patch added to -mm tree

  1. Re: + n_tty-loss-of-sync-following-a-buffer-overflow.patch added to -mm tree

    On Wed, 2008-03-12 at 09:32 -0700, Rupesh Sugathan wrote:
    > The max line size from sender is 100 bytes. The read call waits for a
    > maximum of 200 bytes.


    OK, I understand what is happening.

    Multiple lines are received while the N_TTY buffer is
    full which increments canon_data multiple time without
    setting multiple bits in read_flags. Essentially the
    same bit in read_flags is set over and over.

    When read_chan starts returning data, canon_data is
    decremented once for each bit in read_flags which
    leaves canon_data > 0 when there is no data to be read.

    Even for the case of a single NL when N_TTY buffer is
    full, the read_flag bit is set for tty->read_head when
    that slot already contains data (same as tty->read_tail).

    So the current code is wrong for the buffer full case.

    What I suggest is the following patch for processing NL
    in canonical mode.

    If buffer full and previous character is not NL then
    overwrite that char with NL truncating that line so
    it is not prepended to subsequent data or left waiting
    without a final NL.

    If buffer full and previous character is NL (read_flag set)
    then do nothing so canon_data remains in sync with read_flags.

    Rupesh, can you try this out please?
    Also, none of this answers why your N_TTY buffer is overflowing
    it simply keeps things sane when it happens.

    --- a/drivers/char/n_tty.c 2008-01-24 16:58:37.000000000 -0600
    +++ b/drivers/char/n_tty.c 2008-03-12 12:16:06.000000000 -0500
    @@ -839,10 +839,24 @@ send_signal:

    handle_newline:
    spin_lock_irqsave(&tty->read_lock, flags);
    - set_bit(tty->read_head, tty->read_flags);
    - put_tty_queue_nolock(c, tty);
    - tty->canon_head = tty->read_head;
    - tty->canon_data++;
    + if (tty->read_cnt == N_TTY_BUF_SIZE) {
    + int prev_head;
    + if (tty->read_head)
    + prev_head = tty->read_head - 1;
    + else
    + prev_head = N_TTY_BUF_SIZE - 1;
    + if (!test_bit(prev_head, tty->read_flags)) {
    + /* overwrite previous non NL char */
    + tty->read_cnt--;
    + tty->read_head = prev_head;
    + }
    + }
    + if (tty->read_cnt < N_TTY_BUF_SIZE) {
    + set_bit(tty->read_head, tty->read_flags);
    + put_tty_queue_nolock(c, tty);
    + tty->canon_head = tty->read_head;
    + tty->canon_data++;
    + }
    spin_unlock_irqrestore(&tty->read_lock, flags);
    kill_fasync(&tty->fasync, SIGIO, POLL_IN);
    if (waitqueue_active(&tty->read_wait))


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

  2. Re: + n_tty-loss-of-sync-following-a-buffer-overflow.patch added to -mm tree

    Paul Fulghum wrote:
    > What I suggest is the following patch for processing NL
    > in canonical mode.


    FYI:

    I tested my patch by provoking the bug with
    a contrived pair of programs. Everything works
    as expected. Partially receive lines during buffer
    overflow are truncated. Subsequent lines after the
    ldisc buffer clears are intact, canon_data stays
    consistent with read_flags.

    *But* to force the bug I had to disable the
    check in flush_to_ldisc() that monitors the
    amount of receive room in the ldisc buffer
    before pushing data to the ldisc. This check
    was introduced in the flip buffer rewrite in 2.6

    So the bug is real, the fix works, but in 2.6.24
    this can never occur as the flip buffer holds data
    until N_TTY is ready to process it.

    Rupesh is working with 2.4.x which shares the
    same canonical NL processing but has the old
    flip buffer code that shoves data at N_TTY without
    regard for the receive room in N_TTY.

    I leave it to others to decide if applying the
    patch to 2.6 makes sense.

    --
    Paul Fulghum
    Microgate Systems, Ltd.
    --
    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/

  3. Re: + n_tty-loss-of-sync-following-a-buffer-overflow.patch added to -mm tree

    > Paul Fulghum wrote:
    > > What I suggest is the following patch for processing NL
    > > in canonical mode.


    I tested the patch in 2.4.x and seems to meet all the discussed
    requirements.

    I have another suggestion to this subject. When the buffer oveflows in
    icaonon mode, it would be *best* if the application either gets a
    complete line or does not get it at all. On a buffer overflow, it would
    be good that the n_tty discard the whole line data in the buffer (part
    of which has overflown) and make more room in the buffer.

    Does it make sense to any of you?

    --
    Rupesh Sugathan

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

  4. Re: + n_tty-loss-of-sync-following-a-buffer-overflow.patch added to -mm tree

    Rupesh Sugathan wrote:
    > I have another suggestion to this subject. When the buffer oveflows in
    > icaonon mode, it would be *best* if the application either gets a
    > complete line or does not get it at all. On a buffer overflow, it would
    > be good that the n_tty discard the whole line data in the buffer (part
    > of which has overflown) and make more room in the buffer.
    >
    > Does it make sense to any of you?


    I don't know if there is a standard behavior under
    these conditions so it is hard to argue it should
    be handled a particular way other than leaving the
    device in a consistent and recoverable state.

    I doubt there would be support for making such changes.
    Making that decision in the kernel and
    having an application depend on that non-portable
    behavior does not make sense.

    Given that a n_tty receive overflow is not possible in
    the current kernel (though data can still be lost elsewhere),
    I doubt even my patch merits inclusion.

    --
    Paul

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

+ Reply to Thread