Re: [PATCH v3 8/9] i2c: Support dynamic address translation

[Romain Gantois <romain.gantois@bootlin.com>]

Hi,

On vendredi 29 novembre 2024 10:54:35 heure normale d’Europe centrale Tomi Valkeinen wrote:
> Hi Romain,
> 
...
> > ATR channel's translation table whenever an I2C transaction with unmapped
> > clients is requested.
> > 
> > Add a mutex to protect alias_list. This prevents
> > i2c_atr_dynamic_attach/detach_addr from racing with the bus notifier
> > handler to modify alias_list.
> > 
> > Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
> > ---
> > 
> >   drivers/i2c/i2c-atr.c         | 244
> >   ++++++++++++++++++++++++++++++++----------
> >   drivers/media/i2c/ds90ub960.c |   2 +-
> >   include/linux/i2c-atr.h       |  13 ++-
> >   3 files changed, 202 insertions(+), 57 deletions(-)
> 
> This fails with:
> 
> WARNING: CPU: 1 PID: 360 at lib/list_debug.c:35
> __list_add_valid_or_report+0xe4/0x100
> 
> as the i2c_atr_create_c2a() calls list_add(), but i2c_atr_attach_addr(),
> which is changed to use i2c_atr_create_c2a(), also calls list_add().
> 
> Also, if you add i2c_atr_create_c2a() which hides the allocation and
> list_add, I think it makes sense to add a i2c_atr_destroy_c2a() to
> revert that.
> 

Sure, I just thought that it was safer to have an explicit "kfree" in the
code, as it would be clear that the c2a pointer shouldn't be used after this.
But setting the pointer to NULL after calling i2c_atr_destroy_c2a() would
essentially achieve the same thing, so I'll be going with your suggestion.

> There's also a memory error "BUG: KASAN: slab-use-after-free in
> __lock_acquire+0xc4/0x375c" (see below) when unloading the ub960 or
> ub953 driver. I haven't looked at that yet.
> 

I don't have the hardware to actually reproduce this but I'll see if I can
find out what the problem is by reading the code.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com