Re: [Letux-kernel] BUG: drivers/pinctrl/core: races in pinctrl_groups and deferred probing

[Tony Lindgren]

* H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> [180619 04:54]:
> >> I had seen the call sequence
> >> 
> >> create_pinctrl()-> pinctrl_dt_to_map() -> pcs_dt_node_to_map() -> pinctrl_generic_add_group()
> >> 
> >> w/o any lock inside.

So the sequence above has mutex added around adding the pin
controller specific functions and groups by the patch series
I posted for both pcs_parse_bits_in_pinctrl_entry() and
pcs_parse_one_pinctrl_entry(). So I think the above should
be fixed now. But please confirm to make sure I'm not mistaken.

> >> There is a mutex_lock(&pinctrl_maps_mutex); in create_pinctrl(), but locked after that.
> >> 
> >> Or is there a lock outside of create_pinctrl()?
> >> 
> >> If I look into the stack dumps, call nesting is
> >> 
> >> driver_probe_device() -> pinctrl_bind_pins() -> devm_pinctrl_get() -> create_pinctrl()
> >> 
> >> They all do no locking.
> >> 
> >> Maybe I am missing something.
> > 
> > Can you please post a patch for that as you already have it
> > debugged? That's easier to understand than reading a verbal
> > patch :)
> 
> I have no patch for it and all tests were without, but I can suggest a change which IMHO
> could solve it:
> 
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index eb2b217f5e1e..7d125f9a7804 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -1037,15 +1037,16 @@ static struct pinctrl *create_pinctrl(struct device *dev,
>         INIT_LIST_HEAD(&p->states);
>         INIT_LIST_HEAD(&p->dt_maps);
>  
> +       mutex_lock(&pinctrl_maps_mutex);
>         ret = pinctrl_dt_to_map(p, pctldev);
>         if (ret < 0) {
>                 kfree(p);
> +               mutex_unlock(&pinctrl_maps_mutex);
>                 return ERR_PTR(ret);
>         }
>  
>         devname = dev_name(dev);
>  
> -       mutex_lock(&pinctrl_maps_mutex);
>         /* Iterate over the pin control maps to locate the right ones */
>         for_each_maps(maps_node, i, map) {
>                 /* Map must be for this device */
> 
> Description: we should also protect pinctrl_dt_to_map(), which calls pinctrl_generic_add_group()
> and the calls inside pinctrl_generic_add_group() to pinctrl_generic_group_name_to_selector()
> and radix_tree_insert() with a mutex.
> 
> But I haven't tested this case (because it is unlikely to happen and be testable).

Hmm not sure about this one. The groups and functions are
specific to the pin controller driver and must be locked
by the pin controller driver like this series does. So for
pinctrl_generic_add_group() moving the pinctrl_maps_mutex
earlier should not help. We already do the locking for
generic functions and groups like I described above.

For pinctrl maps moving the mutex earlier might make sense
though :) But aren't we doing that already where the list
gets modified in pinctrl_register_map() and when searching
through the list with for_each_maps? Linus got any comments
on this one?

Regards,

Tony