Hi Dave, On 27/09/2019 17:15, Dave Martin wrote: > On Fri, Sep 27, 2019 at 11:39:49AM -0400, Masayoshi Mizuma wrote: >> From: Masayoshi Mizuma <m.miz...@jp.fujitsu.com> >> >> The system which has SVE feature crashed because of >> the memory pointed by task->thread.sve_state was destroyed >> by someone. >> >> That is because sve_state is freed while the forking the >> child process. The child process has the pointer of sve_state >> which is same as the parent's because the child's task_struct >> is copied from the parent's one. If the copy_process() >> fails as an error on somewhere, for example, copy_creds(), >> then the sve_state is freed even if the parent is alive. >> The flow is as follows. >> >> copy_process >> p = dup_task_struct >> => arch_dup_task_struct >> *dst = *src; // copy the entire region. >> : >> retval = copy_creds >> if (retval < 0) >> goto bad_fork_free; >> : >> bad_fork_free: >> ... >> delayed_free_task(p); >> => free_task >> => arch_release_task_struct >> => fpsimd_release_task >> => __sve_free >> => kfree(task->thread.sve_state); >> // free the parent's sve_state >> >> Move child's sve_state = NULL and clearing TIF_SVE flag >> to arch_dup_task_struct() so that the child doesn't free the >> parent's one. > > You could also add: > > --8<-- > There is no need to wait until copy_process() to clear TIF_SVE for > dst, becuase the thread flags for dst are initialized already by > copying the src task_struct. > > This change simplifies the code, so get rid of comments that are no > longer needed. > -->8-- > >> >> Cc: sta...@vger.kernel.org > > Since SVE only exists from v4.15, it may be helpful to specify that, > i.e., replace that Cc line with: > > Cc: <sta...@vger.kernel.org> # 4.15.x- > > > Otherwise, I'm happy to see this applied, but I'd like somebody to > confirm that this change definitely fixes the bug.
I am working on a reproducer for this. So I should be able to test it. Cheers, -- Julien Grall
