On Thu, 29 Apr 2021 13:05:01 -0400
"Theodore Ts'o" <tytso@xxxxxxx> wrote:
> 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.
There is a small problem about -ENOMEM:
static int kmmpd(void *data)
{
...
retval = read_mmp_block(sb, &bh_check, mmp_block);
if (retval) {
ext4_error_err(sb, -retval,
"error reading MMP data: %d",
retval);
goto exit_thread;
}
...
exit_thread:
EXT4_SB(sb)->s_mmp_tsk = NULL;
kfree(data);
brelse(bh);
return retval;
}
read_mmp_block can return -ENOMEM. In this case double free will happen.
I believe, we can change `return retval;` to `return 0;`, because
kthread return value isn't checked anywhere.
What do You think about it?
With regards,
Pavel Skripkin
