On 09/09/2020 21:50, Paul E. McKenney wrote: > On Wed, Sep 09, 2020 at 07:24:11PM +1000, Alexey Kardashevskiy wrote: >> >> >> On 09/09/2020 00:43, Alexey Kardashevskiy wrote: >>> init_srcu_struct_nodes() is called with is_static==true only internally >>> and when this happens, the srcu->sda is not initialized in >>> init_srcu_struct_fields() and we crash on dereferencing @sdp. >>> >>> This fixes the crash by moving "if (is_static)" out of the loop which >>> only does useful work for is_static=false case anyway. >>> >>> Found by syzkaller. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxxxx> >>> --- >>> kernel/rcu/srcutree.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c >>> index c100acf332ed..49b54a50bde8 100644 >>> --- a/kernel/rcu/srcutree.c >>> +++ b/kernel/rcu/srcutree.c >>> @@ -135,6 +135,9 @@ static void init_srcu_struct_nodes(struct srcu_struct *ssp, bool is_static) >>> levelspread[level - 1]; >>> } >>> >>> + if (is_static) >>> + return; >> >> Actually, this is needed here too: >> >> if (!ssp->sda) >> return; >> >> as >> ssp->sda = alloc_percpu(struct srcu_data) >> >> can fail if the process is killed too soon - it is quite easy to get >> this situation with syzkaller (syscalls fuzzer) >> >> Makes sense? > > Just to make sure that I understand, these failures occur when the task > running init_srcu_struct_nodes() is killed, correct? There are multiple user tasks (8) which open /dev/kvm, do 1 ioctl - KVM_CREATE_VM - and terminate, running on 8 vcpus, 8 VMs, crashes every 20min or so, less tasks or vcpus - and the problem does not appear. > > Or has someone managed to invoke (say) synchronize_srcu() on a > dynamically allocated srcu_struct before invoking init_srcu_struct() on > that srcu_struct? Nah, none of that :) init_srcu_struct_nodes() assumes ssp->sda!=NULL but alloc_percpu() fails here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/percpu.c#n1734 === } else if (mutex_lock_killable(&pcpu_alloc_mutex)) { pcpu_memcg_post_alloc_hook(objcg, NULL, 0, size); return NULL; === I am still up to reading that osr-rcuusage.pdf to provide better analysis :) Thanks, > This would be an SRCU usage bug. If you dynamically > allocate your srcu_struct, you are absolutely required to invoke > init_srcu_struct() on it before doing anything else with it. > > Or am I missing something here? > > (The rcutorture test suite does test both static and dynamic allocation > of the srcu_struct, so I am expecting something a bit subtle here.) > > Thanx, Paul > >>> + >>> /* >>> * Initialize the per-CPU srcu_data array, which feeds into the >>> * leaves of the srcu_node tree. >>> @@ -161,8 +164,6 @@ static void init_srcu_struct_nodes(struct srcu_struct *ssp, bool is_static) >>> timer_setup(&sdp->delay_work, srcu_delay_timer, 0); >>> sdp->ssp = ssp; >>> sdp->grpmask = 1 << (cpu - sdp->mynode->grplo); >>> - if (is_static) >>> - continue; >>> >>> /* Dynamically allocated, better be no srcu_read_locks()! */ >>> for (i = 0; i < ARRAY_SIZE(sdp->srcu_lock_count); i++) { >>> >> >> -- >> Alexey -- Alexey