On Wed, Sep 20, 2023 at 10:56 AM Bartosz Golaszewski <[email protected]> wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> There are two places in the code where we retrieve a lookup table using
> gpiod_find_lookup_table() (which protects the table list with the lookup
> table lock) and then use it after the lock is released.
>
> We need to keep the lookup table mutex locked the entire time we're using
> the tables. Remove the locking from gpiod_find_lookup_table() and use
> guards to protect the code actually using the table objects.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
I convinced myself that this change is correct, good find!
> @@ -3822,8 +3822,6 @@ static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev)
Can we rename this function gpiod_find_lookup_table_locked()
as per precedents in the kernel, to indicate that it needs to be
called with a lock held?
Either way:
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, 20 Sep 2023 11:12:58 +0200, Linus Walleij
<[email protected]> said:
> On Wed, Sep 20, 2023 at 10:56 AM Bartosz Golaszewski <[email protected]> wrote:
>
>> From: Bartosz Golaszewski <[email protected]>
>>
>> There are two places in the code where we retrieve a lookup table using
>> gpiod_find_lookup_table() (which protects the table list with the lookup
>> table lock) and then use it after the lock is released.
>>
>> We need to keep the lookup table mutex locked the entire time we're using
>> the tables. Remove the locking from gpiod_find_lookup_table() and use
>> guards to protect the code actually using the table objects.
>>
>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>
> I convinced myself that this change is correct, good find!
>
>> @@ -3822,8 +3822,6 @@ static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev)
>
> Can we rename this function gpiod_find_lookup_table_locked()
> as per precedents in the kernel, to indicate that it needs to be
> called with a lock held?
>
I think you mean gpiod_find_lookup_table_unlocked() as with this change it
will no longer take the lock?
Bart
> Either way:
> Reviewed-by: Linus Walleij <[email protected]>
>
> Yours,
> Linus Walleij
>
On Wed, Sep 20, 2023 at 11:33 AM Bartosz Golaszewski <[email protected]> wrote:
> On Wed, 20 Sep 2023 11:12:58 +0200, Linus Walleij
> <[email protected]> said:
> > On Wed, Sep 20, 2023 at 10:56 AM Bartosz Golaszewski <[email protected]> wrote:
> > Can we rename this function gpiod_find_lookup_table_locked()
> > as per precedents in the kernel, to indicate that it needs to be
> > called with a lock held?
> >
>
> I think you mean gpiod_find_lookup_table_unlocked() as with this change it
> will no longer take the lock?
I think the pattern is the one I indicated: *_locked() means the function
is to be called with the appropriate lock held, cf
arch/arm64/kvm/hyp/nvhe/mm.c
pkvm_create_mappings() takes a lock and then calls
pkvm_create_mappings_locked() which even asserts that
the lock is held.
Yours,
Linus Walleij