On Thu, Apr 29, 2021 at 02:33:54PM +0300, Pavel Skripkin wrote: > > There is a chance, that kthread_stop() call will happen before > threadfn call. It means, that kthread_stop() return value must be checked everywhere, > isn't it? Otherwise, there are a lot of potential memory leaks, > because some developers rely on the fact, that data allocated for the thread will > be freed _inside_ thread function. That's not the only potential way that we could leak memory. Earlier in kthread(), if this memory allocation fails, self = kzalloc(sizeof(*self), GFP_KERNEL); we will exit with -ENOMEM. So at the very least all callers of kthread_stop() also need to check for -ENOMEM as well as -EINTR --- or, be somehow sure that the thread function was successfully called and started. In this particular case, the ext4 mount code had just started the kmmpd thread, and then detected that something else had gone wrong, and failed the mount before the kmmpd thread ever had a chance to run. I think if we want to fix this more generally across the whole kernel, we would need to have a variant of kthread_run which supplies two functions --- one which is the thread function, and the other which is a cleanup function. The cleanup function could just be kfree, but there will be other cases where the cleanup function will need to do other work before freeing the data structure (e.g., brelse((struct mmpd_data *)data->bh)). Is it worth it to provide such a cleanup function, which if present would be called any time the thread exits or is killed? I dunno. It's probably simpler to just strongly recommend that the cleanup work should never be done in the thread function, but after kthread_stop() is called, whether it returns an error or not. That's probably the right fix for ext4, I think. (Although note that kthread_stop(sbi->s_mmp_task) is called in multiple places in fs/ext4/super.c, not just in the single location which this patch touches.) - Ted
