On Wed, Dec 09, 2020 at 05:59:21PM +0800, Yan.Gao wrote:
> n_tty_flush_buffer can happen in parallel with n_tty_close that the
> tty->disc_data will be set to NULL. n_tty_flush_buffer accesses
> tty->disc_data, so we must prevent n_tty_close clear tty->disc_data
> while n_tty_flush_buffer has a non-NULL view of tty->disc_data.
>
> So we need to make sure that accesses to disc_data are atomic using
> spinlock.
>
> There is an example I meet:
> When n_tty_flush_buffer accesses tty struct, the disc_data is right.
> However, then reset_buffer_flags accesses tty->disc_data, disc_data
> become NULL, So kernel crash when accesses tty->disc_data->real_tail.
> I guess there could be another thread change tty->disc_data to NULL,
> and during N_TTY line discipline, n_tty_close will set tty->disc_data
> to be NULL. So add spinlock to protect disc_data between close and
> flush_buffer.
>
> IP: reset_buffer_flags+0x9/0xf0
> PGD 0 P4D 0
> Oops: 0002 [#1] SMP
> CPU: 23 PID: 2087626 Comm: (agetty) Kdump: loaded Tainted: G
> Hardware name: UNISINSIGHT X3036P-G3/ST01M2C7S, BIOS 2.00.13 01/11/2019
> task: ffff9c4e9da71e80 task.stack: ffffb30cfe898000
> RIP: 0010:reset_buffer_flags+0x9/0xf0
> RSP: 0018:ffffb30cfe89bca8 EFLAGS: 00010246
> RAX: ffff9c4e9da71e80 RBX: ffff9c368d1bac00 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff9c4ea17b50f0 RDI: 0000000000000000
> RBP: ffffb30cfe89bcc8 R08: 0000000000000100 R09: 0000000000000001
> R10: 0000000000000001 R11: 0000000000000000 R12: ffff9c368d1bacc0
> R13: ffff9c20cfd18428 R14: ffff9c4ea17b50f0 R15: ffff9c368d1bac00
> FS: 00007f9fbbe97940(0000) GS:ffff9c375c740000(0000)
> knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000002260 CR3: 0000002f72233003 CR4: 00000000007606e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> PKRU: 55555554
> Call Trace:
> ? n_tty_flush_buffer+0x2a/0x60
> tty_buffer_flush+0x76/0x90
> tty_ldisc_flush+0x22/0x40
> vt_ioctl+0x5a7/0x10b0
> ? n_tty_ioctl_helper+0x27/0x110
> tty_ioctl+0xef/0x8c0
> do_vfs_ioctl+0xa7/0x5e0
> ? __audit_syscall_entry+0xaf/0x100
> ? syscall_trace_enter+0x1d0/0x2b0
> SyS_ioctl+0x79/0x90
> do_syscall_64+0x6c/0x1b0
> entry_SYSCALL64_slow_path+0x25/0x25
>
> n_tty_flush_buffer --->tty->disc_data is OK
> ->reset_buffer_flags -->tty->disc_data is NULL
>
> Signed-off-by: Yan.Gao <[email protected]>
> Reviewed-by: Xianting Tian <[email protected]>
> ---
> drivers/tty/n_tty.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 7e5e36315..f4b152f20 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -87,6 +87,8 @@
> # define n_tty_trace(f, args...) no_printk(f, ##args)
> #endif
>
> +static DEFINE_SPINLOCK(disc_data_lock);
We want to lock data, not code, and this is locking code.
Why can't we use the tty's lock here?
> +
> struct n_tty_data {
> /* producer-published */
> size_t read_head;
> @@ -371,8 +373,10 @@ static void n_tty_packet_mode_flush(struct tty_struct *tty)
> static void n_tty_flush_buffer(struct tty_struct *tty)
> {
> down_write(&tty->termios_rwsem);
> + spin_lock(&disc_data_lock);
> reset_buffer_flags(tty->disc_data);
> n_tty_kick_worker(tty);
> + spin_unlock(&disc_data_lock);
We already have the termios_rwsem lock here, why do we need another one?
>
> if (tty->link)
> n_tty_packet_mode_flush(tty);
> @@ -1892,8 +1896,10 @@ static void n_tty_close(struct tty_struct *tty)
> if (tty->link)
> n_tty_packet_mode_flush(tty);
>
> + spin_lock_irq(&disc_data_lock);
> vfree(ldata);
> tty->disc_data = NULL;
> + spin_unlock_irq(&disc_data_lock);
Why can't you just grab the termios_rwsem lock?
thanks,
greg k-h
Hi Greg KH,
Thank you for reviewing the patch, it helped me a lot. According to your suggestion, I change the
code. Please help me to review the v2 patch. Thanks again.
https://lkml.org/lkml/2020/12/9/1412
----- Original mail -----
发件人: Greg KH [mailto:[email protected]]
发送时间: 2020年12月9日 22:38
收件人: gaoyan (RD) <[email protected]>
抄送: [email protected]; [email protected]; tianxianting (RD) <[email protected]>
主题: Re: [PATCH] tty: Protect disc_data in n_tty_close and n_tty_flush_buffer
On Wed, Dec 09, 2020 at 05:59:21PM +0800, Yan.Gao wrote:
> n_tty_flush_buffer can happen in parallel with n_tty_close that the
> tty->disc_data will be set to NULL. n_tty_flush_buffer accesses
> tty->disc_data, so we must prevent n_tty_close clear tty->disc_data
> while n_tty_flush_buffer has a non-NULL view of tty->disc_data.
>
> So we need to make sure that accesses to disc_data are atomic using
> spinlock.
>
> There is an example I meet:
> When n_tty_flush_buffer accesses tty struct, the disc_data is right.
> However, then reset_buffer_flags accesses tty->disc_data, disc_data
> become NULL, So kernel crash when accesses tty->disc_data->real_tail.
> I guess there could be another thread change tty->disc_data to NULL,
> and during N_TTY line discipline, n_tty_close will set tty->disc_data
> to be NULL. So add spinlock to protect disc_data between close and
> flush_buffer.
>
> IP: reset_buffer_flags+0x9/0xf0
> PGD 0 P4D 0
> Oops: 0002 [#1] SMP
> CPU: 23 PID: 2087626 Comm: (agetty) Kdump: loaded Tainted: G Hardware
> name: UNISINSIGHT X3036P-G3/ST01M2C7S, BIOS 2.00.13 01/11/2019
> task: ffff9c4e9da71e80 task.stack: ffffb30cfe898000
> RIP: 0010:reset_buffer_flags+0x9/0xf0
> RSP: 0018:ffffb30cfe89bca8 EFLAGS: 00010246
> RAX: ffff9c4e9da71e80 RBX: ffff9c368d1bac00 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff9c4ea17b50f0 RDI: 0000000000000000
> RBP: ffffb30cfe89bcc8 R08: 0000000000000100 R09: 0000000000000001
> R10: 0000000000000001 R11: 0000000000000000 R12: ffff9c368d1bacc0
> R13: ffff9c20cfd18428 R14: ffff9c4ea17b50f0 R15: ffff9c368d1bac00
> FS: 00007f9fbbe97940(0000) GS:ffff9c375c740000(0000)
> knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000002260 CR3: 0000002f72233003 CR4: 00000000007606e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> PKRU: 55555554
> Call Trace:
> ? n_tty_flush_buffer+0x2a/0x60
> tty_buffer_flush+0x76/0x90
> tty_ldisc_flush+0x22/0x40
> vt_ioctl+0x5a7/0x10b0
> ? n_tty_ioctl_helper+0x27/0x110
> tty_ioctl+0xef/0x8c0
> do_vfs_ioctl+0xa7/0x5e0
> ? __audit_syscall_entry+0xaf/0x100
> ? syscall_trace_enter+0x1d0/0x2b0
> SyS_ioctl+0x79/0x90
> do_syscall_64+0x6c/0x1b0
> entry_SYSCALL64_slow_path+0x25/0x25
>
> n_tty_flush_buffer --->tty->disc_data is OK
> ->reset_buffer_flags -->tty->disc_data is NULL
>
> Signed-off-by: Yan.Gao <[email protected]>
> Reviewed-by: Xianting Tian <[email protected]>
> ---
> drivers/tty/n_tty.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index
> 7e5e36315..f4b152f20 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -87,6 +87,8 @@
> # define n_tty_trace(f, args...) no_printk(f, ##args)
> #endif
>
> +static DEFINE_SPINLOCK(disc_data_lock);
We want to lock data, not code, and this is locking code.
Why can't we use the tty's lock here?
> +
> struct n_tty_data {
> /* producer-published */
> size_t read_head;
> @@ -371,8 +373,10 @@ static void n_tty_packet_mode_flush(struct
> tty_struct *tty) static void n_tty_flush_buffer(struct tty_struct
> *tty) {
> down_write(&tty->termios_rwsem);
> + spin_lock(&disc_data_lock);
> reset_buffer_flags(tty->disc_data);
> n_tty_kick_worker(tty);
> + spin_unlock(&disc_data_lock);
We already have the termios_rwsem lock here, why do we need another one?
>
> if (tty->link)
> n_tty_packet_mode_flush(tty);
> @@ -1892,8 +1896,10 @@ static void n_tty_close(struct tty_struct *tty)
> if (tty->link)
> n_tty_packet_mode_flush(tty);
>
> + spin_lock_irq(&disc_data_lock);
> vfree(ldata);
> tty->disc_data = NULL;
> + spin_unlock_irq(&disc_data_lock);
Why can't you just grab the termios_rwsem lock?
thanks,
greg k-h