Re: [PATCH] drivers:tty:pty: Fix a race causing data loss on close

[Greg Kroah-Hartman]

On Thu, Oct 01, 2020 at 09:16:30PM -0500, minyard@xxxxxxx wrote:
> From: Corey Minyard <cminyard@xxxxxxxxxx>
> 
> If you write to a pty master an immediately close the pty master, the
> receiver might get a chunk of data dropped, but then receive some later
> data.  That's obviously something rather unexpected for a user.  It
> certainly confused my test program.
> 
> It turns out that tty_vhangup() gets called from pty_close(), and that
> causes the data on the slave side to be flushed, but due to races more
> data can be copied into the slave side's buffer after that.  Consider
> the following sequence:
> 
> thread1            thread2            thread3
>                    write data into buffer,
>                       n_tty buffer is filled
>                    pty_close()
>                     tty_vhangup()
>                      tty_ldisc_hangup()
>                       n_tty_flush_buffer()
>                        reset_buffer_flags()
> n_tty_read()
>  up_read(&tty->termios_rwsem);
>                         down_read(&tty->termios_rwsem);
>                         clear n_tty buffer contents
>                         up_read(&tty->termios_rwsem);
>  tty_buffer_flush_work()
>   schedules work calling
>         flush_to_ldisc()
>                                       flush_to_ldisc()
>                                        receive_buf()
>                                         tty_port_default_receive_buf()
>                                          tty_ldisc_receive_buf()
>                                           tty_ldisc_receive_buf()
>                                            n_tty_receive_buf2()
>                                             n_tty_receive_buf_common()
>                                              down_read(&tty->termios_rwsem);
>                                              __receive_buf()
>                                               copies data into n_tty buffer
>                                              up_read(&tty->termios_rwsem);
>  down_read(&tty->termios_rwsem);
>  copy buffer data to user

Your text got line-wrapped here for this explaination and it doesn't
make much sense :(

Can you resend this?

> This change checks to see if the tty is being hung up before copying
> anything in n_tty_receive_buf_common().  It has to be done after the
> tty->termios_rwsem semaphore is claimed, for reasons that should be
> apparent from the sequence above.

That kind of makes sense, but it's tricky, if you resend with the above
fixed it might be more obvious...

thanks,

greg k-h