On Sat, Jan 28, 2023 at 8:02 PM Wolfram Sang <wsa@xxxxxxxxxx> wrote: > > Hi Bartosz, > > > > https://lkml.iu.edu/hypermail/linux/kernel/1501.2/01700.html > > > > > > There are still drivers using i2c_del_adapter()+kfree(), so removing the > > > completion could cause use-after-free there, or? > > > > > > > Ugh, what a mess... I was mostly focused on the character device side > > of it but now I realized the true extent of the problem. > > Still, thanks for trying. Really! > > > It's all because the adapter struct really should be allocated by > > i2c_add_adapter() and bus drivers should only really provide some > > structure containing the adapter description for the subsystem the > > lifetime of which would not affect the adapter itself. This way the > > adapter (embedding struct device) would be freed by device type's > > .release() like we do over in the GPIO subsystem. Instead the adapter > > struct is allocated by drivers at .probe() meaning it will get dropped > > at .remove(). > > Or, like SPI does, use controller_alloc() which initializes the parts > needed by the core, returns to the driver which needs to setup the > private data, and finally calls controller_register(). > If we could add a helper like struct *i2c_adapter_get_device(struct i2c_adapter *adap) and convert all users who access adap.dev directly to using it instead in a sweeping change across the subsystem - that would already be a huge improvement as this would allow us to later move struct device memory into the subsystem and free it in .release() and not allow provider drivers to free it at .remove(). Something to consider. Let me know if that's an interesting option. > > I don't have a good solution. I've been thinking about it for an hour > > and every solution requires sweeping changes across the entire > > subsystem. Or else we'd introduce a parallel solution that would do > > the right thing and wait in perpetuity until all drivers convert - > > like with i2e probe_new() which is after all much simpler. > > Thank you for spending time on another solution. "Perpetuity" is a good > word to put it :/ > > > Anyway, that's all I've got. We probably need to drop this change and > > live with what we have now. > > I am curious to see if this finding will make it into your FOSDEM talk. > Looking already forward to it! > Ha! Yeah, definitely. I went down that rabbit hole a while ago and am bothered by the amount of latent bugs I'm finding. I sent some patches for GPIO and SPI (and this one for I2C). See you there, I guess? > Happy hacking, > > Wolfram > Bart
