Re: [PATCH] powerpc/mm: add sanity check to avoid null pointer dereference

[Christophe Leroy]

Le 28/12/2020 à 04:10, Defang Bo a écrit :
> Similar to commit<0dc294f717d4>("powerpc/mm: bail out early when flushing TLB page"),
> there should be a check for 'mm' to prevent Null pointer dereference
> in case of 'mm' argument was legitimately passed.

I don't understand what you are trying to do here.

In commit 0dc294f717d4, a call with a NULL mm is expected. The commit 
just reorganises the code in order to bail out earlier and more explicitely.

In your patch, you are adding WARN_ON, which means that being called 
with a NULL mm is unexpected. Why do you think it is necessary to do it 
? Have you encountered any problem ?

Thanks
Christophe

> 
> Signed-off-by: Defang Bo <bodefang@xxxxxxx>
> ---
>   arch/powerpc/mm/nohash/tlb.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/mm/nohash/tlb.c b/arch/powerpc/mm/nohash/tlb.c
> index 5872f69..1d89335 100644
> --- a/arch/powerpc/mm/nohash/tlb.c
> +++ b/arch/powerpc/mm/nohash/tlb.c
> @@ -192,6 +192,9 @@ void local_flush_tlb_mm(struct mm_struct *mm)
>   {
>   	unsigned int pid;
>   
> +	if (WARN_ON(!mm))
> +		return;
> +
>   	preempt_disable();
>   	pid = mm->context.id;
>   	if (pid != MMU_NO_CONTEXT)
> @@ -205,8 +208,11 @@ void __local_flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,
>   {
>   	unsigned int pid;
>   
> +	if (WARN_ON(!mm))
> +		return;
> +
>   	preempt_disable();
> -	pid = mm ? mm->context.id : 0;
> +	pid = mm->context.id;
>   	if (pid != MMU_NO_CONTEXT)
>   		_tlbil_va(vmaddr, pid, tsize, ind);
>   	preempt_enable();
> @@ -268,6 +274,9 @@ void flush_tlb_mm(struct mm_struct *mm)
>   {
>   	unsigned int pid;
>   
> +	if (WARN_ON(!mm))
> +		return;
> +
>   	preempt_disable();
>   	pid = mm->context.id;
>   	if (unlikely(pid == MMU_NO_CONTEXT))