All of lore.kernel.org
 help / color / mirror / Atom feed
* A data race between tty_port_open() and uart_do_autoconfig()
@ 2021-04-14  0:17 Gong, Sishuai
  2021-04-14  6:20 ` Greg KH
  0 siblings, 1 reply; 5+ messages in thread
From: Gong, Sishuai @ 2021-04-14  0:17 UTC (permalink / raw)
  To: jirislaby, a.darwish, aik, johan; +Cc: linux-serial

Hi,

We found a data race between two tty functions tty_port_open() and uart_do_autoconfig() in linux-5.12-rc3 and we are able to reproduce it under x86. In general, when tty_port_open() and uart_do_autoconfig() are running in parallel, uart_do_autoconfig() may fetch an out-of-date value of port->count and enter into a different execution path, as shown below.

Currently, we haven’t found any explicit errors due to this data race but we noticed the developer has used lock to read port->count, so we want to point out this data race in case this is unexpected.

------------------------------------------
Execution interleaving

Thread 1					Thread 2
tty_port_open()			uart_do_autoconfig()

spin_lock_irq(&port->lock);
						if (mutex_lock_interruptible(&port->mutex))
						…
						if (tty_port_users(port) == 1) {
							uart_shutdown(tty, state);

++port->count;
spin_unlock_irq(&port->lock);




Thanks,
Sishuai


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

* Re: A data race between tty_port_open() and uart_do_autoconfig()
  2021-04-14  0:17 A data race between tty_port_open() and uart_do_autoconfig() Gong, Sishuai
@ 2021-04-14  6:20 ` Greg KH
  2021-04-14 14:35   ` Gong, Sishuai
  0 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2021-04-14  6:20 UTC (permalink / raw)
  To: Gong, Sishuai; +Cc: jirislaby, a.darwish, aik, johan, linux-serial

On Wed, Apr 14, 2021 at 12:17:54AM +0000, Gong, Sishuai wrote:
> Hi,
> 
> We found a data race between two tty functions tty_port_open() and uart_do_autoconfig() in linux-5.12-rc3 and we are able to reproduce it under x86. In general, when tty_port_open() and uart_do_autoconfig() are running in parallel, uart_do_autoconfig() may fetch an out-of-date value of port->count and enter into a different execution path, as shown below.
> 
> Currently, we haven’t found any explicit errors due to this data race but we noticed the developer has used lock to read port->count, so we want to point out this data race in case this is unexpected.
> 
> ------------------------------------------
> Execution interleaving
> 
> Thread 1					Thread 2
> tty_port_open()			uart_do_autoconfig()
> 
> spin_lock_irq(&port->lock);
> 						if (mutex_lock_interruptible(&port->mutex))
> 						…
> 						if (tty_port_users(port) == 1) {
> 							uart_shutdown(tty, state);
> 
> ++port->count;
> spin_unlock_irq(&port->lock);
> 
> 
> 

Can you send a proposed patch for this to fix the issue as you sem to
have a reproducer for this that you can test if the change resolves the
issue or not?

thanks,

greg k-h

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

* Re: A data race between tty_port_open() and uart_do_autoconfig()
  2021-04-14  6:20 ` Greg KH
@ 2021-04-14 14:35   ` Gong, Sishuai
  2021-04-14 15:08     ` Greg KH
  0 siblings, 1 reply; 5+ messages in thread
From: Gong, Sishuai @ 2021-04-14 14:35 UTC (permalink / raw)
  To: Greg KH; +Cc: jirislaby, a.darwish, aik, johan, linux-serial

For now we are not sure about if this is a harmful data race or not. A potential fix could be using the same mutex lock port->mutex instead of port->lock, to protect every write on port->count so the reader can get a consistent view. However, this seems to be a big change so we are wondering it is worthy of this change.

Thanks,
Sishuai

> On Apr 14, 2021, at 2:20 AM, Greg KH <greg@kroah.com> wrote:
> 
> On Wed, Apr 14, 2021 at 12:17:54AM +0000, Gong, Sishuai wrote:
>> Hi,
>> 
>> We found a data race between two tty functions tty_port_open() and uart_do_autoconfig() in linux-5.12-rc3 and we are able to reproduce it under x86. In general, when tty_port_open() and uart_do_autoconfig() are running in parallel, uart_do_autoconfig() may fetch an out-of-date value of port->count and enter into a different execution path, as shown below.
>> 
>> Currently, we haven’t found any explicit errors due to this data race but we noticed the developer has used lock to read port->count, so we want to point out this data race in case this is unexpected.
>> 
>> ------------------------------------------
>> Execution interleaving
>> 
>> Thread 1					Thread 2
>> tty_port_open()			uart_do_autoconfig()
>> 
>> spin_lock_irq(&port->lock);
>> 						if (mutex_lock_interruptible(&port->mutex))
>> 						…
>> 						if (tty_port_users(port) == 1) {
>> 							uart_shutdown(tty, state);
>> 
>> ++port->count;
>> spin_unlock_irq(&port->lock);
>> 
>> 
>> 
> 
> Can you send a proposed patch for this to fix the issue as you sem to
> have a reproducer for this that you can test if the change resolves the
> issue or not?
> 
> thanks,
> 
> greg k-h


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

* Re: A data race between tty_port_open() and uart_do_autoconfig()
  2021-04-14 14:35   ` Gong, Sishuai
@ 2021-04-14 15:08     ` Greg KH
  2021-04-14 16:31       ` Gong, Sishuai
  0 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2021-04-14 15:08 UTC (permalink / raw)
  To: Gong, Sishuai; +Cc: jirislaby, a.darwish, aik, johan, linux-serial

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 Wed, Apr 14, 2021 at 02:35:24PM +0000, Gong, Sishuai wrote:
> For now we are not sure about if this is a harmful data race or not.

Can you please work to determine that?

> A potential fix could be using the same mutex lock port->mutex instead
> of port->lock, to protect every write on port->count so the reader can
> get a consistent view. However, this seems to be a big change so we
> are wondering it is worthy of this change.

Try it out and see!

thanks,

greg k-h

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

* Re: A data race between tty_port_open() and uart_do_autoconfig()
  2021-04-14 15:08     ` Greg KH
@ 2021-04-14 16:31       ` Gong, Sishuai
  0 siblings, 0 replies; 5+ messages in thread
From: Gong, Sishuai @ 2021-04-14 16:31 UTC (permalink / raw)
  To: Greg KH; +Cc: jirislaby, a.darwish, aik, johan, linux-serial

On Apr 14, 2021, at 11:08 AM, Greg KH <greg@kroah.com> wrote:
> 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
Thanks for pointing it out:)
> 
> On Wed, Apr 14, 2021 at 02:35:24PM +0000, Gong, Sishuai wrote:
>> For now we are not sure about if this is a harmful data race or not.
> 
> Can you please work to determine that?
I found an ancient comment left in uart_set_info_user() saying 
mutex_lock(&port->mutex) “This semaphore protects port->count”.
This is untrue since changes on port->count only grab spin_lock(&port->lock).
So it looks like this needs to be fixed. 
> 
>> A potential fix could be using the same mutex lock port->mutex instead
>> of port->lock, to protect every write on port->count so the reader can
>> get a consistent view. However, this seems to be a big change so we
>> are wondering it is worthy of this change.
> 
> Try it out and see!
We will start to think about the patch, and try to submit it after testing soon.
> thanks,
> 
> greg k-h

Thanks,
Sishuai

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

end of thread, other threads:[~2021-04-14 16:31 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-14  0:17 A data race between tty_port_open() and uart_do_autoconfig() Gong, Sishuai
2021-04-14  6:20 ` Greg KH
2021-04-14 14:35   ` Gong, Sishuai
2021-04-14 15:08     ` Greg KH
2021-04-14 16:31       ` Gong, Sishuai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.