On Thu, Apr 18, 2024 at 09:21:00PM +0200, Heiko Carstens wrote: > Hi Nathan, > > > > > > > > - /* > > > > > > > - * The release function could be called after the > > > > > > > - * module has been unloaded. It's _only_ task is to > > > > > > > - * free the struct. Therefore, we specify kfree() > > > > > > > - * directly here. (Probably a little bit obfuscating > > > > > > > - * but legitime ...). > > > > > > > - */ > > > > > > That doesn't answer my question what prevents the release function > > > from being called after the module has been unloaded. > > > > > > At least back then when the code was added it was a real bug. > > > > I do not know the answer to that question (and I suspect there is > > nothing preventing ->release() from being called after module unload), > > so I'll just bring back the comment (although I'll need to adjust it > > since kfree() is not being used there directly anymore). Andrew, would > > you prefer a diff from what's in -mm or a v2? > > I guess there is some confusion here :) My request was not to keep the Heh, yes, my apologies for being rather dense, I was not interpreting the comment or the thread you linked properly... :( > comment. I'm much rather afraid that the comment is still valid; and if > that is the case then your patch series adds three bugs, exactly what is > described in the comment. > > Right now the release function is kfree which is always within the kernel > image, and therefore always a valid branch target. If however the code is > changed to what you propose, then the release function would be inside of > the module, which potentially does not exist anymore when the release > function is called, since the module was unloaded. > So the branch target would be invalid. That is super subtle :/ I can understand what the comment is warning about with that extra context. I see Arnd's suggestion which may fix this problem and get rid of the warning but if there are other ideas, I am all ears. I guess we could just disable -Wcast-function-type-strict for this code since s390 does not support kCFI right now but since it could, it seems better to resolve it properly. Thanks a lot for the quick review and catching my mistake, cheers! Nathan
