答复: [PATCH][v2] tty: fix race between flush_to_ldisc and tty_open
> -邮件原件- > 发件人: Kohli, Gaurav [mailto:gko...@codeaurora.org] > 发送时间: 2019年1月11日 21:57 > 收件人: Li,Rongqing ; linux-kernel@vger.kernel.org; > jsl...@suse.com; gre...@linuxfoundation.org; a...@linux.intel.com > 主题: Re: [PATCH][v2] tty: fix race between flush_to_ldisc and tty_open > > Hi, > > it don't seems to be good idea to put lock on one function and unlock in some > other function. If in future some one has to call tty_init_dev, how he can > track > the unlocking as well of ldisc lock. > > Regards > Gaurav > This similar condition has existed for a long time, tty_unlock(tty) must be called in some other functions who call tty_init_dev, since tty_init_dev hold tty_lock, and does not release it so I think it is user responsibility to fully understand tty_init_dev; and I can add some comments for tty_init_dev if this patch can be acceptable; or a workaround like below: diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index a42a028a9d4e..2f5ad256b6ad 100644 --- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -425,7 +425,7 @@ static void flush_to_ldisc(struct work_struct *work) struct tty_ldisc *disc; tty = port->itty; - if (tty == NULL) + if (tty == NULL || tty->driver_data == NULL) return; disc = tty_ldisc_ref(tty); thanks -RongQing
Re: [PATCH][v2] tty: fix race between flush_to_ldisc and tty_open
Hi, it don't seems to be good idea to put lock on one function and unlock in some other function. If in future some one has to call tty_init_dev, how he can track the unlocking as well of ldisc lock. Regards Gaurav On 12/24/2018 8:13 AM, Li RongQing wrote: There still is a race window after the commit b027e2298bd588 ("tty: fix data race between tty_init_dev and flush of buf"), if receive_buf call comes before tty initialization completes in n_tty_open and tty->driver_data may be NULL. CPU0 CPU1 n_tty_open tty_init_dev tty_ldisc_unlock schedule flush_to_ldisc n_tty_receive_buf uart_flush_chars uart_start /*tty->driver_data is NULL*/ tty->ops->open /*init tty->driver_data*/ Extending ldisc semaphore lock in tty_init_dev to driver_data initialized completely after tty->ops->open(). Signed-off-by: Zhang Yu Signed-off-by: Li RongQing --- drivers/staging/speakup/spk_ttyio.c | 1 + drivers/tty/pty.c | 2 ++ drivers/tty/serdev/serdev-ttyport.c | 2 ++ drivers/tty/tty_io.c| 14 +++--- drivers/tty/tty_ldisc.c | 1 + 5 files changed, 17 insertions(+), 3 deletions(-) diff --git a/drivers/staging/speakup/spk_ttyio.c b/drivers/staging/speakup/spk_ttyio.c index 979e3ae249c1..c31f08c98383 100644 --- a/drivers/staging/speakup/spk_ttyio.c +++ b/drivers/staging/speakup/spk_ttyio.c @@ -155,6 +155,7 @@ static int spk_ttyio_initialise_ldisc(struct spk_synth *synth) else ret = -ENODEV; + tty_ldisc_unlock(tty); if (ret) { tty_unlock(tty); return ret; diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index 00099a8439d2..1b9684d4f718 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -873,9 +873,11 @@ static int ptmx_open(struct inode *inode, struct file *filp) tty_debug_hangup(tty, "opening (count=%d)\n", tty->count); + tty_ldisc_unlock(tty); tty_unlock(tty); return 0; err_release: + tty_ldisc_unlock(tty); tty_unlock(tty); // This will also put-ref the fsi tty_release(inode, filp); diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c index fa1672993b4c..ce16cb303e28 100644 --- a/drivers/tty/serdev/serdev-ttyport.c +++ b/drivers/tty/serdev/serdev-ttyport.c @@ -123,6 +123,7 @@ static int ttyport_open(struct serdev_controller *ctrl) if (ret) goto err_close; + tty_ldisc_unlock(tty); tty_unlock(serport->tty); /* Bring the UART into a known 8 bits no parity hw fc state */ @@ -145,6 +146,7 @@ static int ttyport_open(struct serdev_controller *ctrl) err_close: tty->ops->close(tty, NULL); err_unlock: + tty_ldisc_unlock(tty); tty_unlock(tty); tty_release_struct(tty, serport->tty_idx); diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index 687250ec8032..199f45e2e1b1 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -1351,7 +1351,6 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx) retval = tty_ldisc_setup(tty, tty->link); if (retval) goto err_release_tty; - tty_ldisc_unlock(tty); /* Return the tty locked so that it cannot vanish under the caller */ return tty; @@ -1926,7 +1925,7 @@ EXPORT_SYMBOL_GPL(tty_kopen); * - concurrent tty removal from driver table */ static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode, -struct file *filp) +struct file *filp, bool *unlock) { struct tty_struct *tty; struct tty_driver *driver = NULL; @@ -1970,6 +1969,7 @@ static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode, } } else { /* Returns with the tty_lock held for now */ tty = tty_init_dev(driver, index); + *unlock = true; mutex_unlock(_mutex); } out: @@ -2007,6 +2007,7 @@ static int tty_open(struct inode *inode, struct file *filp) int noctty, retval; dev_t device = inode->i_rdev; unsigned saved_flags = filp->f_flags; + bool unlock = false; nonseekable_open(inode, filp); @@ -2017,7 +2018,7 @@ static int tty_open(struct inode *inode, struct file *filp) tty = tty_open_current_tty(device, filp); if (!tty) - tty = tty_open_by_driver(device, inode, filp); + tty = tty_open_by_driver(device, inode, filp, ); if (IS_ERR(tty)) { tty_free_file(filp); @@ -2042,6 +2043,10 @@ static int
[PATCH][v2] tty: fix race between flush_to_ldisc and tty_open
There still is a race window after the commit b027e2298bd588 ("tty: fix data race between tty_init_dev and flush of buf"), if receive_buf call comes before tty initialization completes in n_tty_open and tty->driver_data may be NULL. CPU0 CPU1 n_tty_open tty_init_dev tty_ldisc_unlock schedule flush_to_ldisc n_tty_receive_buf uart_flush_chars uart_start /*tty->driver_data is NULL*/ tty->ops->open /*init tty->driver_data*/ Extending ldisc semaphore lock in tty_init_dev to driver_data initialized completely after tty->ops->open(). Signed-off-by: Zhang Yu Signed-off-by: Li RongQing --- drivers/staging/speakup/spk_ttyio.c | 1 + drivers/tty/pty.c | 2 ++ drivers/tty/serdev/serdev-ttyport.c | 2 ++ drivers/tty/tty_io.c| 14 +++--- drivers/tty/tty_ldisc.c | 1 + 5 files changed, 17 insertions(+), 3 deletions(-) diff --git a/drivers/staging/speakup/spk_ttyio.c b/drivers/staging/speakup/spk_ttyio.c index 979e3ae249c1..c31f08c98383 100644 --- a/drivers/staging/speakup/spk_ttyio.c +++ b/drivers/staging/speakup/spk_ttyio.c @@ -155,6 +155,7 @@ static int spk_ttyio_initialise_ldisc(struct spk_synth *synth) else ret = -ENODEV; + tty_ldisc_unlock(tty); if (ret) { tty_unlock(tty); return ret; diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index 00099a8439d2..1b9684d4f718 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -873,9 +873,11 @@ static int ptmx_open(struct inode *inode, struct file *filp) tty_debug_hangup(tty, "opening (count=%d)\n", tty->count); + tty_ldisc_unlock(tty); tty_unlock(tty); return 0; err_release: + tty_ldisc_unlock(tty); tty_unlock(tty); // This will also put-ref the fsi tty_release(inode, filp); diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c index fa1672993b4c..ce16cb303e28 100644 --- a/drivers/tty/serdev/serdev-ttyport.c +++ b/drivers/tty/serdev/serdev-ttyport.c @@ -123,6 +123,7 @@ static int ttyport_open(struct serdev_controller *ctrl) if (ret) goto err_close; + tty_ldisc_unlock(tty); tty_unlock(serport->tty); /* Bring the UART into a known 8 bits no parity hw fc state */ @@ -145,6 +146,7 @@ static int ttyport_open(struct serdev_controller *ctrl) err_close: tty->ops->close(tty, NULL); err_unlock: + tty_ldisc_unlock(tty); tty_unlock(tty); tty_release_struct(tty, serport->tty_idx); diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index 687250ec8032..199f45e2e1b1 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -1351,7 +1351,6 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx) retval = tty_ldisc_setup(tty, tty->link); if (retval) goto err_release_tty; - tty_ldisc_unlock(tty); /* Return the tty locked so that it cannot vanish under the caller */ return tty; @@ -1926,7 +1925,7 @@ EXPORT_SYMBOL_GPL(tty_kopen); * - concurrent tty removal from driver table */ static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode, -struct file *filp) +struct file *filp, bool *unlock) { struct tty_struct *tty; struct tty_driver *driver = NULL; @@ -1970,6 +1969,7 @@ static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode, } } else { /* Returns with the tty_lock held for now */ tty = tty_init_dev(driver, index); + *unlock = true; mutex_unlock(_mutex); } out: @@ -2007,6 +2007,7 @@ static int tty_open(struct inode *inode, struct file *filp) int noctty, retval; dev_t device = inode->i_rdev; unsigned saved_flags = filp->f_flags; + bool unlock = false; nonseekable_open(inode, filp); @@ -2017,7 +2018,7 @@ static int tty_open(struct inode *inode, struct file *filp) tty = tty_open_current_tty(device, filp); if (!tty) - tty = tty_open_by_driver(device, inode, filp); + tty = tty_open_by_driver(device, inode, filp, ); if (IS_ERR(tty)) { tty_free_file(filp); @@ -2042,6 +2043,10 @@ static int tty_open(struct inode *inode, struct file *filp) if (retval) { tty_debug_hangup(tty, "open error %d, releasing\n", retval); + if (unlock) { + unlock = false; + tty_ldisc_unlock(tty); +