linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] [v2] tty: Protect disc_data in n_tty_close and n_tty_flush_buffer
@ 2020-12-10  2:25 Yan.Gao
  2020-12-10  6:21 ` Greg KH
  0 siblings, 1 reply; 5+ messages in thread
From: Yan.Gao @ 2020-12-10  2:25 UTC (permalink / raw)
  To: gregkh, jirislaby; +Cc: linux-kernel, tian.xianting, Yan.Gao

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
tty->termios_rwsem.

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 use tty->termios_rwsem 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 <gao.yanB@h3c.com>
Reviewed-by: Xianting Tian <tian.xianting@h3c.com>
---
 drivers/tty/n_tty.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 7e5e36315..e78124ce1 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1892,8 +1892,10 @@ static void n_tty_close(struct tty_struct *tty)
 	if (tty->link)
 		n_tty_packet_mode_flush(tty);
 
+	down_write(&tty->termios_rwsem);
 	vfree(ldata);
 	tty->disc_data = NULL;
+	up_write(&tty->termios_rwsem);
 }
 
 /**
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] [v2] tty: Protect disc_data in n_tty_close and n_tty_flush_buffer
  2020-12-10  2:25 [PATCH] [v2] tty: Protect disc_data in n_tty_close and n_tty_flush_buffer Yan.Gao
@ 2020-12-10  6:21 ` Greg KH
  2020-12-11  6:09   ` 答复: " Gaoyan
  0 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2020-12-10  6:21 UTC (permalink / raw)
  To: Yan.Gao; +Cc: jirislaby, linux-kernel, tian.xianting

On Thu, Dec 10, 2020 at 10:25:07AM +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
> tty->termios_rwsem.
> 
> 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 use tty->termios_rwsem 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 <gao.yanB@h3c.com>
> Reviewed-by: Xianting Tian <tian.xianting@h3c.com>
> ---
>  drivers/tty/n_tty.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 7e5e36315..e78124ce1 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -1892,8 +1892,10 @@ static void n_tty_close(struct tty_struct *tty)
>  	if (tty->link)
>  		n_tty_packet_mode_flush(tty);
>  
> +	down_write(&tty->termios_rwsem);
>  	vfree(ldata);
>  	tty->disc_data = NULL;
> +	up_write(&tty->termios_rwsem);
>  }
>  
>  /**

So does this solve your problem in testing?  Do you have a reproducer
for this problem?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread

* 答复: [PATCH] [v2] tty: Protect disc_data in n_tty_close and n_tty_flush_buffer
  2020-12-10  6:21 ` Greg KH
@ 2020-12-11  6:09   ` Gaoyan
  2020-12-15  2:45     ` Tianxianting
  0 siblings, 1 reply; 5+ messages in thread
From: Gaoyan @ 2020-12-11  6:09 UTC (permalink / raw)
  To: Greg KH; +Cc: jirislaby, linux-kernel, Tianxianting

Hi Greg KH:
	I try to reproduce this problem in testing, but it is difficult to happen again. It is hard to grasp the timing
that n_tty_flush_buffer accesses the disc_data which was just set to NULL by n_tty_close.

Thanks
Gao Yan

-----邮件原件-----
发件人: Greg KH [mailto:gregkh@linuxfoundation.org] 
发送时间: 2020年12月10日 14:22
收件人: gaoyan (RD) <gao.yanB@h3c.com>
抄送: jirislaby@kernel.org; linux-kernel@vger.kernel.org; tianxianting (RD) <tian.xianting@h3c.com>
主题: Re: [PATCH] [v2] tty: Protect disc_data in n_tty_close and n_tty_flush_buffer

On Thu, Dec 10, 2020 at 10:25:07AM +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
> tty->termios_rwsem.
> 
> 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 use tty->termios_rwsem 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 <gao.yanB@h3c.com>
> Reviewed-by: Xianting Tian <tian.xianting@h3c.com>
> ---
>  drivers/tty/n_tty.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 
> 7e5e36315..e78124ce1 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -1892,8 +1892,10 @@ static void n_tty_close(struct tty_struct *tty)
>  	if (tty->link)
>  		n_tty_packet_mode_flush(tty);
>  
> +	down_write(&tty->termios_rwsem);
>  	vfree(ldata);
>  	tty->disc_data = NULL;
> +	up_write(&tty->termios_rwsem);
>  }
>  
>  /**

So does this solve your problem in testing?  Do you have a reproducer for this problem?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] [v2] tty: Protect disc_data in n_tty_close and n_tty_flush_buffer
  2020-12-11  6:09   ` 答复: " Gaoyan
@ 2020-12-15  2:45     ` Tianxianting
  2020-12-15  7:07       ` Greg KH
  0 siblings, 1 reply; 5+ messages in thread
From: Tianxianting @ 2020-12-15  2:45 UTC (permalink / raw)
  To: Gaoyan, Greg KH; +Cc: jirislaby, linux-kernel

Hi Greg KH
Could we get your comments for the updates?  Thanks 

-----Original Message-----
From: gaoyan (RD) 
Sent: Friday, December 11, 2020 2:09 PM
To: Greg KH <gregkh@linuxfoundation.org>
Cc: jirislaby@kernel.org; linux-kernel@vger.kernel.org; tianxianting (RD) <tian.xianting@h3c.com>
Subject: 答复: [PATCH] [v2] tty: Protect disc_data in n_tty_close and n_tty_flush_buffer

Hi Greg KH:
	I try to reproduce this problem in testing, but it is difficult to happen again. It is hard to grasp the timing that n_tty_flush_buffer accesses the disc_data which was just set to NULL by n_tty_close.

Thanks
Gao Yan

-----邮件原件-----
发件人: Greg KH [mailto:gregkh@linuxfoundation.org]
发送时间: 2020年12月10日 14:22
收件人: gaoyan (RD) <gao.yanB@h3c.com>
抄送: jirislaby@kernel.org; linux-kernel@vger.kernel.org; tianxianting (RD) <tian.xianting@h3c.com>
主题: Re: [PATCH] [v2] tty: Protect disc_data in n_tty_close and n_tty_flush_buffer

On Thu, Dec 10, 2020 at 10:25:07AM +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
> tty->termios_rwsem.
> 
> 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 use tty->termios_rwsem 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 <gao.yanB@h3c.com>
> Reviewed-by: Xianting Tian <tian.xianting@h3c.com>
> ---
>  drivers/tty/n_tty.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index
> 7e5e36315..e78124ce1 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -1892,8 +1892,10 @@ static void n_tty_close(struct tty_struct *tty)
>  	if (tty->link)
>  		n_tty_packet_mode_flush(tty);
>  
> +	down_write(&tty->termios_rwsem);
>  	vfree(ldata);
>  	tty->disc_data = NULL;
> +	up_write(&tty->termios_rwsem);
>  }
>  
>  /**

So does this solve your problem in testing?  Do you have a reproducer for this problem?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] [v2] tty: Protect disc_data in n_tty_close and n_tty_flush_buffer
  2020-12-15  2:45     ` Tianxianting
@ 2020-12-15  7:07       ` Greg KH
  0 siblings, 0 replies; 5+ messages in thread
From: Greg KH @ 2020-12-15  7:07 UTC (permalink / raw)
  To: Tianxianting; +Cc: Gaoyan, jirislaby, linux-kernel


A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top


On Tue, Dec 15, 2020 at 02:45:01AM +0000, Tianxianting wrote:
> Hi Greg KH
> Could we get your comments for the updates?  Thanks 

Sorry, it is the middle of the merge window, I'll get to this after
5.11-rc1 is released.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-12-15  7:08 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-10  2:25 [PATCH] [v2] tty: Protect disc_data in n_tty_close and n_tty_flush_buffer Yan.Gao
2020-12-10  6:21 ` Greg KH
2020-12-11  6:09   ` 答复: " Gaoyan
2020-12-15  2:45     ` Tianxianting
2020-12-15  7:07       ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).