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

[Ian Ray <ian.ray@xxxxxxxxxxxxxxxx>]

On Fri, Oct 18, 2024 at 11:26:54AM +0200, Jean Delvare wrote:
> Hi Ray,
> 
> Sorry for the delay.
> 
> On Tue, 2024-10-08 at 09:02 +0300, Ian Ray wrote:
> > On Mon, Oct 07, 2024 at 11:16:51PM +0200, Jean Delvare wrote:
> > > 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.
> > > >
> > > >  * Normal case
> > > >
> > > >    0-0022: write register AI|3a {03,02,00,00,01} Input latch P0
> > > >    0-0022: write register AI|49 {fc,fd,ff,ff,fe} Interrupt mask P0
> > > >    0-0022: write register AI|08 {ff,00,00,00,00} Output P3
> > > >    0-0022: write register AI|12 {fc,00,00,00,00} Config P3
> > > >
> > > >  * Race case
> > > >
> > > >    0-0022: write register AI|08 {ff,00,00,00,00} Output P3
> > > >    0-0022: write register AI|08 {03,02,00,00,01} *** Wrong register ***
> > > >    0-0022: write register AI|12 {fc,00,00,00,00} Config P3
> > > >    0-0022: write register AI|49 {fc,fd,ff,ff,fe} Interrupt mask P0
> > > >
> > >
> > > I have more questions on this. Where does the above log come from?
> > > Specifically, at which layer (bus driver, regmap, gpio device drier)?
> >
> > Additional debug, with manually added commentary (sorry for not being
> > clearer).  The debug was added to drivers/base/regmap/regmap-i2c.c while
> > investigating the issue.
> 
> FWIW, I think regmap includes a tracing facility which may have served
> you. Specifically, I see calls to trace_regmap_hw_write_start() and
> trace_regmap_hw_write_done() in _regmap_raw_write_impl(). But I must
> confess I couldn't find where these functions are defined nor how to
> enable tracing...

Interesting, thank you!

> 
> > > What do these values represent exactly? Which GPIO chip was used on
> > > your system? Which i2c bus driver is being used on that system? What
> > > are the "requests" you mention in the description above?
> >
> > GPIO expander pi4ioe5v6534q at I2C address 0-0022.
> 
> This device model doesn't seem to be explicitly supported by driver
> gpio-pca953x. I see it listed as compatible in
> Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml but not in the
> driver's pca953x_dt_ids. Out of curiosity, did you have to add it
> manually? I admit I'm not familiar with these device tree node
> declarations.
> 
> > # grep . {name,uevent}
> > name:30a20000.i2c
> > uevent:OF_NAME=i2c
> > uevent:OF_FULLNAME=/soc@0/bus@30800000/i2c@30a20000
> > uevent:OF_COMPATIBLE_0=fsl,imx8mp-i2c
> > uevent:OF_COMPATIBLE_1=fsl,imx21-i2c
> > uevent:OF_COMPATIBLE_N=2
> > uevent:OF_ALIAS_0=i2c0
> 
> OK, so the underlying I2C master is capable of writing to multiple
> registers at once. This helped me follow the code flow while trying to
> figure out where the race was.
> 
> > > I'm asking because I do not understand how writing to the wrong
> > > register can happen, even without holding i2c_lock in
> > > pca953x_irq_bus_sync_unlock(). The i2c layer has a per-i2c_adapter lock
> >
> > Given that pca953x_irq_bus_sync_unlock is part of an interrupt handler,
> > IMHO this explains very well why locking is needed (but I did not dig
> > deeper than that).
> 
> I took the time to dig deeper, my conclusions are below.
> 
> > > which is taken before any bus transfer, so it isn't possible that two
> > > transfers collide at the bus level. So the lack of locking at the
> > > device driver level could lead to data corruption (for example read-
> > > modify-write cycles overlapping), but not to data being written to the
> > > wrong register.
> >
> > Based on the observed data, the hypothesis was that pca953x_write_regs
> > (called via pca953x_gpio_set_multiple) and pca953x_irq_bus_sync_unlock
> > can race.
> >
> > The missing guard neatly explained and fixed the issue (disclaimer: on
> > my hardware for my scenario).
> >
> > > As a side note, I dug through the history of the gpio-pca953x driver
> > > and found that i2c_lock was introduced before the driver was converted
> > > to regmap by:
> > >
> > > commit 6e20fb18054c179d7e64c0af43d855b9310a3394
> > > Author: Roland Stigge
> > > Date:   Thu Feb 10 15:01:23 2011 -0800
> > >
> > >     drivers/gpio/pca953x.c: add a mutex to fix race condition
> > >
> > > The fix added locking around read-modify-write cycles (which was indeed
> > > needed) and also around simple register reads (which I don't think was
> > > needed).
> > >
> > > It turns out that regmap has its own protection around read-modify-
> > > write cycles (see regmap_update_bits_base) so I think several uses of
> > > i2c_lock should have been removed from the gpio-pca953x driver when it
> > > was converted to regmap as they became redundant then.
> 
> I have to correct myself here. The regmap layer implements its own,
> configurable and *optional* protection lock. It turns out that the
> gpio-pca953x driver has it disabled:
> 
> static const struct regmap_config pca953x_i2c_regmap = {
>         (...)
>         .disable_locking = true,
>         (...)
> };
> 
> So it is expected and very needed that the gpio-pca953x driver
> implements its own lock to protect against races whenever the hardware
> is accessed.
> 
> > > This driver-side
> > > lock is still needed in a number of functions though, where the read-
> > > modify-write is handled outside of regmap (for example in
> > > pca953x_gpio_set_multiple).
> 
> After reading the regmap code (which took me some time as I wasn't
> familiar at all with it, I didn't know what I was looking for exactly
> and I wanted to make sure I wasn't missing something along the way), I
> think I understand what was racing exactly.
> 
> The gpio-pca953x driver uses regmap_bulk_write() which is implemented
> by _regmap_raw_write_impl(). The register map uses an internal buffer
> to prepare the actual hardware transfers:
> 
> struct regmap *__regmap_init(...) {
>         (...)
>         map->work_buf = kzalloc(map->format.buf_size, GFP_KERNEL);
>         (...)
> }
> 
> This work buffer has space for both the register address and the values
> to be written to or read from the device:
> 
>         map->format.buf_size = DIV_ROUND_UP(config->reg_bits +
>                         config->val_bits + config->pad_bits, 8);
> 
> During a regmap raw write, the register address is written to the first
> byte of the work buffer:
> 
>         map->format.format_reg(map->work_buf, reg, map->reg_shift);
> 
> where map->format.format_reg() is regmap_format_8() for the gpio-
> pca953x driver:
> 
> static void regmap_format_8(void *buf, unsigned int val, unsigned int shift)
> {
>         u8 *b = buf;
> 
>         b[0] = val << shift;
> }
> 
> If _regmap_raw_write_impl() is called concurrently without proper
> locking then the contents of the work buffer may be overwritten by the
> second caller before the first caller had a chance to use it. I think
> this matches your debug log of the race case pretty well.
> 
> I checked the regmap implementation for other use cases of map-
> >format.format_reg(map->work_buf, ...) and found it is being used in
> _regmap_raw_read(), so I had to investigate further, because
> pca953x_irq_bus_sync_unlock() also calls pca953x_read_regs(..., chip-
> >regs->direction, ...) which in turn calls regmap_bulk_read().
> 
> For volatile registers, this function will call regmap_raw_read() which
> reads the values from the hardware most of the time. However, for non-
> volatile registers, _regmap_bulk_read() is being called instead, which
> is implemented by _regmap_read() which reads from the regmap cache. As
> it turns out that the direction registers are not volatile and are read
> first as part of pca953x_irq_setup(), the values will always be
> available from the cache when read from pca953x_irq_bus_sync_unlock(),
> so no hardware access will happen and the internal work buffer won't be
> used.
> 
> Therefore my conclusion is that your fix was needed, is correct and is
> sufficient. My initial concern about the unprotected
> pca953x_read_regs() call in pca953x_irq_bus_sync_unlock() was
> incorrect. Sorry for the noise.

No noise, this was a really interesting study, and a good learning
experience for me.  Thank you for this.

Blue skies,
Ian


> 
> --
> Jean Delvare
> SUSE L3 Support