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

[Ian Ray <ian.ray@xxxxxxxxxxxxxxxx>]

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.

Thanks,
Ian


> 
> I'm not familiar with the gpio-pca953x driver at all so I may be
> missing something and maybe everything is actually fine, but I would
> appreciate if someone could take a look and give a second opinion.
> 
> Thanks,
> --
> Jean Delvare
> SUSE L3 Support
>