Re: Kernel crash due to memory corruption with v5.4.26-rt17 and PowerPC e500

[Thomas Graziadei]

Hi Sebastian,

any progress on your side?

Do you think the patch could be applied for the next versions?

Regards,
Thomas

On Fri, 2020-07-10 at 10:59 +0000, Thomas Graziadei wrote:
> Hi Sebastian,
> 
> thanks for looking into this.
> 
> We could reproduce the issue with QEMU.
> At runtime you need to set mdev as the kernel's hotplug client
> (/proc/sys/kernel/hotplug) and give it a dummy /etc/mdev.conf like
> (.* 1:1 777). Then just do a loop and insmod/rmmod crc4.ko and
> crc7.ko.
> 
> Swapping the mm assignment did not work -> exception after 1900
> iterations
> Your second suggestion with check.patch (attached to this email for
> completeness, only protecting the exec_mmap function) did not work
> eighter -> exception after 2600 iterations
> 
> Your third suggestion (a modification to the original revert)
> enclosed in this e-mail does seem to work. Still no problems after
> 30000 iterations.
> 
> By the way, as noticed in your kernel config, we would be quite
> interested in a gcc 9 compiler for our platform. Is there a
> mainline/maintained version or fork for this or another possibility
> to get it?
>  
> Regards,
> Thomas
> 
> -----Original Message-----
> From: Sebastian Andrzej Siewior [mailto:bigeasy@xxxxxxxxxxxxx] 
> Sent: Monday, July 06, 2020 6:50 PM
> To: Mark Marshall <markmarshall14@xxxxxxxxx>
> Cc: linux-rt-users <linux-rt-users@xxxxxxxxxxxxxxx>; Mark Marshall <
> mark.marshall@xxxxxxxxxxxxxxxxx>; Thomas Graziadei <
> thomas.graziadei@xxxxxxxxxxxxxxxxx>; Thomas Gleixner <
> tglx@xxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; 
> rostedt@xxxxxxxxxxx
> Subject: Re: Kernel crash due to memory corruption with v5.4.26-rt17
> and PowerPC e500
> 
> On 2020-05-29 18:37:22 [+0200], To Mark Marshall wrote:
> > On 2020-05-29 18:15:18 [+0200], To Mark Marshall wrote:
> > > In order to get it back into the RT queue I need to understand
> > > why 
> > > it is required. What exactly is it fixing. Let me stare at for a 
> > > little…
> > 
> > it used to be local_irq_disable() which then became
> > preempt_disable()
> > local_irq_disable() due to ARM's limitation.
> 
> Any luck on your side?
> 
> I *think* if you swap the mm assignment in exec_mmap() then it should
> be gone. Basically:
> >         tsk->active_mm = mm;
> >         tsk->mm = mm;
> 
> However I think to apply something like this:
> 
> diff --git a/fs/exec.c b/fs/exec.c
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1035,11 +1035,15 @@ static int exec_mmap(struct mm_struct *mm)
>  		}
>  	}
>  	task_lock(tsk);
> +
> +	task_lock_mm();
>  	active_mm = tsk->active_mm;
>  	membarrier_exec_mmap(mm);
>  	tsk->mm = mm;
>  	tsk->active_mm = mm;
>  	activate_mm(active_mm, mm);
> +	task_unlock_mm();
> +
>  	tsk->mm->vmacache_seqnum = 0;
>  	vmacache_flush(tsk);
>  	task_unlock(tsk);
> diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
> --- a/include/linux/sched/task.h
> +++ b/include/linux/sched/task.h
> @@ -176,4 +176,31 @@ static inline void task_unlock(struct
> task_struct *p)
>  	spin_unlock(&p->alloc_lock);
>  }
>  
> +#ifdef CONFIG_PREEMPT_RT
> +/*
> + * Protects ->mm and ->active_mm.
> + * Avoids scheduling so switch_mm() or enter_lazy_tlb() will not
> read 
> +the
> + * members while they are updated.
> + */
> +static inline void task_lock_mm(void)
> +{
> +	preempt_disable();
> +}
> +
> +static inline void task_unlock_mm(void) {
> +	preempt_enable();
> +}
> +
> +#else
> +
> +static inline void task_lock_mm(void)
> +{
> +}
> +
> +static inline void task_unlock_mm(void) { } #endif
> +
>  #endif /* _LINUX_SCHED_TASK_H */
> diff --git a/mm/mmu_context.c b/mm/mmu_context.c
> --- a/mm/mmu_context.c
> +++ b/mm/mmu_context.c
> @@ -25,6 +25,7 @@ void use_mm(struct mm_struct *mm)
>  	struct task_struct *tsk = current;
>  
>  	task_lock(tsk);
> +	task_lock_mm();
>  	active_mm = tsk->active_mm;
>  	if (active_mm != mm) {
>  		mmgrab(mm);
> @@ -32,6 +33,7 @@ void use_mm(struct mm_struct *mm)
>  	}
>  	tsk->mm = mm;
>  	switch_mm(active_mm, mm, tsk);
> +	task_unlock_mm();
>  	task_unlock(tsk);
>  #ifdef finish_arch_post_lock_switch
>  	finish_arch_post_lock_switch();
> @@ -55,10 +57,12 @@ void unuse_mm(struct mm_struct *mm)
>  	struct task_struct *tsk = current;
>  
>  	task_lock(tsk);
> +	task_lock_mm();
>  	sync_mm_rss(mm);
>  	tsk->mm = NULL;
>  	/* active_mm is still 'mm' */
>  	enter_lazy_tlb(mm, tsk);
> +	task_unlock_mm();
>  	task_unlock(tsk);
>  }
>  EXPORT_SYMBOL_GPL(unuse_mm);
> --
> 2.27.0
> 
> > > > Best regards,
> > > > Mark
> 
> Sebastian