AB BA lock inversion in ucsi driver caused by "usb: typec: ucsi: displayport: Fix a potential race during registration"

AB BA lock inversion in ucsi driver caused by "usb: typec: ucsi: displayport: Fix a potential race during registration"

[Hans de Goede]

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

Re: AB BA lock inversion in ucsi driver caused by "usb: typec: ucsi: displayport: Fix a potential race during registration"

[Heikki Krogerus]

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

Re: AB BA lock inversion in ucsi driver caused by "usb: typec: ucsi: displayport: Fix a potential race during registration"

[Hans de Goede]

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

Re: AB BA lock inversion in ucsi driver caused by "usb: typec: ucsi: displayport: Fix a potential race during registration"

[Heikki Krogerus]

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