Re: [PATCH] gpio: pca953x: fix pca953x_irq_bus_sync_unlock race

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Fri, Sep 27, 2024 at 1:36 PM Ian Ray <ian.ray@xxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Sep 27, 2024 at 11:49:04AM +0200, Jean Delvare wrote:
> >
> > Hello Ian,
> >
> > On Thu, 2024-06-20 at 07:29 +0300, Ian Ray wrote:
> > > Ensure that `i2c_lock' is held when setting interrupt latch and mask in
> > > pca953x_irq_bus_sync_unlock() in order to avoid races.
> > >
> > > The other (non-probe) call site pca953x_gpio_set_multiple() ensures the
> > > lock is held before calling pca953x_write_regs().
> > >
> > > The problem occurred when a request raced against irq_bus_sync_unlock()
> > > approximately once per thousand reboots on an i.MX8MP based system.
> :
> > > --- a/drivers/gpio/gpio-pca953x.c
> > > +++ b/drivers/gpio/gpio-pca953x.c
> > > @@ -758,6 +758,8 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
> > >         int level;
> > >
> > >         if (chip->driver_data & PCA_PCAL) {
> > > +               guard(mutex)(&chip->i2c_lock);
> > > +
> > >                 /* Enable latch on interrupt-enabled inputs */
> > >                 pca953x_write_regs(chip, PCAL953X_IN_LATCH, chip->irq_mask);
> > >
> >
> > I've been asked to backport this fix to SUSE kernels and I have a
> > concern about it.
> >
> > You take the i2c_lock mutex inside the (chip->driver_data & PCA_PCAL)
> > conditional block, where pca953x_write_regs() is being called, and the
> > commit description implies this is indeed the call you wanted to
> > protect.
> >
> > However, immediately after the conditional block, the common code path
> > includes a call to pca953x_read_regs(). Looking at the rest of the
> > driver code, I see that the i2c_lock mutex is *also* always held
> > (except during device probe) when calling this function. Which isn't
> > really surprising as I seem to understand the device uses a banked
> > register addressing, and this typically affects both reading from and
> > writing to registers.
> >
> > So I suspect the i2c_lock mutex needs to be held for this call to
> > pca953x_read_regs() as well (unless you are familiar with the register
> > map and know for sure that the "direction" register is outside of the
> > banked register range).
>
> Hello Jean,
>
> Direction is indeed banked (see, for example, PCA953x_BANK_CONFIG).
>
> It certainly looks plausible that a race between
> pca953x_gpio_direction_input or pca953x_gpio_direction_output and
> the register read in pca953x_irq_bus_sync_unlock may occur.
>
> In practice, I think that this is unlikely to ever be observed because
> (IMHO) GPIO direction is rarely changed after initialization.
> (Disclaimer: this is true for the embedded systems I work with.)
>
> Hope this clarifies things.
>

I'd argue that this is the case for kernel users but you can never
tell what the user-space will do. I think this may be a valid concern
and worth addressing.

Bart