Messages in this thread | | | Subject | Re: [PATCH v2] gpio: Restrict usage of gc irq members before initialization | From | Shreeya Patel <> | Date | Tue, 15 Mar 2022 18:02:05 +0530 |
| |
On 15/03/22 4:30 pm, Andy Shevchenko wrote: > On Tue, Mar 15, 2022 at 12:38 PM Shreeya Patel > <shreeya.patel@collabora.com> wrote: > > Thanks for the update, my comments below. > >> gc irq members are exposed before they could be completely > gc --> GPIO chip > > >> initialized and this leads to race conditions. > Any example here. like ~3-4 lines of the Oops in question? > >> One such issue was observed for the gc->irq.domain variable which >> was accessed through the I2C interface in gpiochip_to_irq() before >> it could be initialized by gpiochip_add_irqchip(). This resulted in >> Kernel NULL pointer dereference. >> >> To avoid such scenarios, restrict usage of gc irq members before > gc --> GPIO chip > >> they are completely initialized. > ... > >> + /* >> + * Using barrier() here to prevent compiler from reordering >> + * gc->irq.gc_irq_initialized before initialization of above >> + * gc irq members. >> + */ >> + barrier(); >> + >> + gc->irq.gc_irq_initialized = true; > There are too many duplications. Why not simply call it 'initialized'? > >> - if (gc->to_irq) { >> + if (gc->to_irq && gc->irq.gc_irq_initialized) { > Why can't this check be added into gpiochip_to_irq() ? > > if (!gc->irq.initialized) > return -ENXIO; > > ...
Because we don't want to return -ENXIO in case of race condition.
It should return -EPROBE_DEFER similar to how we are doing when gc->to_irq is NULL.
So in this case when both gc->to_irq = NULL and gc->irq.initialized = FALSE, we will be returning -EPROBE_DEFER. This will make sure that devices like touchscreen do not become fatal due to returning -ENXIO.
> >> + bool gc_irq_initialized; > Can you move it closer to .init_hw so it will be weakly grouped by > logic similarities? > Also see above.
Thanks for your comments, I'll make the necessary changes and send a v3.
Shreeya Patel
| |