Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* AB BA lock inversion in ucsi driver caused by "usb: typec: ucsi: displayport: Fix a potential race during registration"
@ 2020-07-17 10:04 Hans de Goede
  2020-07-27 12:37 ` Heikki Krogerus
  0 siblings, 1 reply; 4+ messages in thread
From: Hans de Goede @ 2020-07-17 10:04 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: linux-usb

Hi Heikki,

I've been running my personal kernel builds with lockdep enabled
(more people should do that) and it found an AB BA lock inversion in the
ucsi driver. This has been introduced by commit 081da1325d35 ("usb: typec:
ucsi: displayport: Fix a potential race during registration").

The problem is as follows:

AB order:

1. ucsi_init takes ucsi->ppm_lock (it runs with that locked for the duration of the function)
2. usci_init eventually end up calling ucsi_register_displayport, which takes
    ucsi_connector->lock

BA order:

1. ucsi_handle_connector_change work is started, takes ucsi_connector->lock
2. ucsi_handle_connector_change calls ucsi_send_command which takes ucsi->ppm_lock

I think this can be fixed by doing the following:

a. Make ucsi_init drop the ucsi->ppm_lock before it starts registering ports; and
    replacing any ucsi_run_command calls after this point with ucsi_send_command
    (which is a wrapper around run_command taking the lock while handling the command)

b. Move the taking of the ucsi_connector->lock from ucsi_register_displayport into
    ucsi_register_port() to make sure that nothing can touch the connector/port until
    ucsi_register_port() has completed.


b. is not stricly necessary but it brings the locking during init more inline
with locking done during runtime so this seems like a good idea.

Regards,

Hans


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

* Re: AB BA lock inversion in ucsi driver caused by "usb: typec: ucsi: displayport: Fix a potential race during registration"
  2020-07-17 10:04 AB BA lock inversion in ucsi driver caused by "usb: typec: ucsi: displayport: Fix a potential race during registration" Hans de Goede
@ 2020-07-27 12:37 ` Heikki Krogerus
  2020-07-27 12:54   ` Hans de Goede
  0 siblings, 1 reply; 4+ messages in thread
From: Heikki Krogerus @ 2020-07-27 12:37 UTC (permalink / raw)
  To: Hans de Goede; +Cc: linux-usb

Hi Hans,

Sorry about the late reply. I just returned from vacation.

On Fri, Jul 17, 2020 at 12:04:58PM +0200, Hans de Goede wrote:
> Hi Heikki,
> 
> I've been running my personal kernel builds with lockdep enabled
> (more people should do that) and it found an AB BA lock inversion in the
> ucsi driver. This has been introduced by commit 081da1325d35 ("usb: typec:
> ucsi: displayport: Fix a potential race during registration").
> 
> The problem is as follows:
> 
> AB order:
> 
> 1. ucsi_init takes ucsi->ppm_lock (it runs with that locked for the duration of the function)
> 2. usci_init eventually end up calling ucsi_register_displayport, which takes
>    ucsi_connector->lock
> 
> BA order:
> 
> 1. ucsi_handle_connector_change work is started, takes ucsi_connector->lock
> 2. ucsi_handle_connector_change calls ucsi_send_command which takes ucsi->ppm_lock
> 
> I think this can be fixed by doing the following:
> 
> a. Make ucsi_init drop the ucsi->ppm_lock before it starts registering ports; and
>    replacing any ucsi_run_command calls after this point with ucsi_send_command
>    (which is a wrapper around run_command taking the lock while handling the command)
> 
> b. Move the taking of the ucsi_connector->lock from ucsi_register_displayport into
>    ucsi_register_port() to make sure that nothing can touch the connector/port until
>    ucsi_register_port() has completed.
> 
> 
> b. is not stricly necessary but it brings the locking during init more inline
> with locking done during runtime so this seems like a good idea.

Makes sense. So b. it is. Can you prepare the patch for that?

thanks,

-- 
heikki

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

* Re: AB BA lock inversion in ucsi driver caused by "usb: typec: ucsi: displayport: Fix a potential race during registration"
  2020-07-27 12:37 ` Heikki Krogerus
@ 2020-07-27 12:54   ` Hans de Goede
  2020-07-28 11:00     ` Heikki Krogerus
  0 siblings, 1 reply; 4+ messages in thread
From: Hans de Goede @ 2020-07-27 12:54 UTC (permalink / raw)
  To: Heikki Krogerus; +Cc: linux-usb

Hi,

On 7/27/20 2:37 PM, Heikki Krogerus wrote:
> Hi Hans,
> 
> Sorry about the late reply. I just returned from vacation.

NP. I hope you've enjoyed your vacation.

> On Fri, Jul 17, 2020 at 12:04:58PM +0200, Hans de Goede wrote:
>> Hi Heikki,
>>
>> I've been running my personal kernel builds with lockdep enabled
>> (more people should do that) and it found an AB BA lock inversion in the
>> ucsi driver. This has been introduced by commit 081da1325d35 ("usb: typec:
>> ucsi: displayport: Fix a potential race during registration").
>>
>> The problem is as follows:
>>
>> AB order:
>>
>> 1. ucsi_init takes ucsi->ppm_lock (it runs with that locked for the duration of the function)
>> 2. usci_init eventually end up calling ucsi_register_displayport, which takes
>>     ucsi_connector->lock
>>
>> BA order:
>>
>> 1. ucsi_handle_connector_change work is started, takes ucsi_connector->lock
>> 2. ucsi_handle_connector_change calls ucsi_send_command which takes ucsi->ppm_lock
>>
>> I think this can be fixed by doing the following:
>>
>> a. Make ucsi_init drop the ucsi->ppm_lock before it starts registering ports; and
>>     replacing any ucsi_run_command calls after this point with ucsi_send_command
>>     (which is a wrapper around run_command taking the lock while handling the command)
>>
>> b. Move the taking of the ucsi_connector->lock from ucsi_register_displayport into
>>     ucsi_register_port() to make sure that nothing can touch the connector/port until
>>     ucsi_register_port() has completed.
>>
>>
>> b. is not stricly necessary but it brings the locking during init more inline
>> with locking done during runtime so this seems like a good idea.
> 
> Makes sense. So b. it is. Can you prepare the patch for that?

b. is just a cleanup / extra step on top of a. we need a. to fix the inversion.

If you are ok with that I can try to make some time for this...

Regards,

Hans


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

* Re: AB BA lock inversion in ucsi driver caused by "usb: typec: ucsi: displayport: Fix a potential race during registration"
  2020-07-27 12:54   ` Hans de Goede
@ 2020-07-28 11:00     ` Heikki Krogerus
  0 siblings, 0 replies; 4+ messages in thread
From: Heikki Krogerus @ 2020-07-28 11:00 UTC (permalink / raw)
  To: Hans de Goede; +Cc: linux-usb

On Mon, Jul 27, 2020 at 02:54:00PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/27/20 2:37 PM, Heikki Krogerus wrote:
> > Hi Hans,
> > 
> > Sorry about the late reply. I just returned from vacation.
> 
> NP. I hope you've enjoyed your vacation.
> 
> > On Fri, Jul 17, 2020 at 12:04:58PM +0200, Hans de Goede wrote:
> > > Hi Heikki,
> > > 
> > > I've been running my personal kernel builds with lockdep enabled
> > > (more people should do that) and it found an AB BA lock inversion in the
> > > ucsi driver. This has been introduced by commit 081da1325d35 ("usb: typec:
> > > ucsi: displayport: Fix a potential race during registration").
> > > 
> > > The problem is as follows:
> > > 
> > > AB order:
> > > 
> > > 1. ucsi_init takes ucsi->ppm_lock (it runs with that locked for the duration of the function)
> > > 2. usci_init eventually end up calling ucsi_register_displayport, which takes
> > >     ucsi_connector->lock
> > > 
> > > BA order:
> > > 
> > > 1. ucsi_handle_connector_change work is started, takes ucsi_connector->lock
> > > 2. ucsi_handle_connector_change calls ucsi_send_command which takes ucsi->ppm_lock
> > > 
> > > I think this can be fixed by doing the following:
> > > 
> > > a. Make ucsi_init drop the ucsi->ppm_lock before it starts registering ports; and
> > >     replacing any ucsi_run_command calls after this point with ucsi_send_command
> > >     (which is a wrapper around run_command taking the lock while handling the command)
> > > 
> > > b. Move the taking of the ucsi_connector->lock from ucsi_register_displayport into
> > >     ucsi_register_port() to make sure that nothing can touch the connector/port until
> > >     ucsi_register_port() has completed.
> > > 
> > > 
> > > b. is not stricly necessary but it brings the locking during init more inline
> > > with locking done during runtime so this seems like a good idea.
> > 
> > Makes sense. So b. it is. Can you prepare the patch for that?
> 
> b. is just a cleanup / extra step on top of a. we need a. to fix the inversion.
> 
> If you are ok with that I can try to make some time for this...

That would be great!

thanks,

-- 
heikki

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

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17 10:04 AB BA lock inversion in ucsi driver caused by "usb: typec: ucsi: displayport: Fix a potential race during registration" Hans de Goede
2020-07-27 12:37 ` Heikki Krogerus
2020-07-27 12:54   ` Hans de Goede
2020-07-28 11:00     ` Heikki Krogerus

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git