On 12/18/21 23:33, Robert Schlabbach wrote:
Such patch should actually be fixing a use-after-free on disconnect.I wonder if it's the right thing to do, though, because if you look into the em28xx_close_extension() function in em28xx-core.c: if (ops->fini) { if (dev->dev_next) ops->fini(dev->dev_next); ops->fini(dev); } So doing this in em28xx_cards.c: em28xx_close_extension(dev); if (dev->dev_next) { em28xx_close_extension(dev->dev_next); em28xx_release_resources(dev->dev_next); } will end up calling ops->fini() twice on dev->dev_next, no matter in which order you put the calls.
I don't see any problem in calling ->fini() twice. I see only 4 ->fini() variants and of them have guards against double free.
I've checked out if there possible kref unbalance, but ->init() is also called 2 times: em28xx_register_extension() and em28xx_init_extension().
So it looks prone to double-free, but at least em28xx_dvb_fini() in em28xx_dvb.c guards against that by NULLing the dev->dvb pointer after free and checking the pointer at entry. Still, there are redundant calls here. I think a decision should be made whether dev->dev_next is taken care of in em28xx-core.c or in em28xx-cards.c, and the code then be made consistent accordingly.
Agree. Some kind of refactoring should be done :) With regards, Pavel Skripkin
