Re: [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep_assert

[Peter Xu]

On Fri, Oct 04, 2024 at 04:17:42PM +0100, Matthew Wilcox wrote:
> On Fri, Oct 04, 2024 at 07:15:48PM +0530, Manas via B4 Relay wrote:
> > +++ b/mm/memory.c
> > @@ -6346,10 +6346,10 @@ static inline void pfnmap_args_setup(struct follow_pfnmap_args *args,
> >  static inline void pfnmap_lockdep_assert(struct vm_area_struct *vma)
> >  {
> >  #ifdef CONFIG_LOCKDEP
> > -	struct address_space *mapping = vma->vm_file->f_mapping;
> > +	struct address_space *mapping = vma->vm_file ? vma->vm_file->f_mapping : NULL;
> 
> Overly long and complex line.  Much simpler to write:
> 
> 	struct address_space *mapping = NULL;
> 
> 	if (vma->vm_file)
> 		mapping = vma->vm_file->f_mapping;
> 
> >  	if (mapping)
> > -		lockdep_assert(lockdep_is_held(&vma->vm_file->f_mapping->i_mmap_rwsem) ||
> > +		lockdep_assert(lockdep_is_held(&mapping->i_mmap_rwsem) ||
> >  			       lockdep_is_held(&vma->vm_mm->mmap_lock));
> >  	else
> >  		lockdep_assert(lockdep_is_held(&vma->vm_mm->mmap_lock));
> 
> This one should have been lockdep_assert_held(&vma->vm_mm->mmap_lock).
> 
> I'm not sure that the previous one is correct.  The
> lockdep_assert_held() macro is pretty careful about checking
> LOCK_STATE_NOT_HELD to avoid the LOCK_STATE_UNKNOWN possibility.
> But I'll leave that for Peter to fix.

Indeed..

Then looks like we could have quite a few other places in Linux that can
have used this wrong.. when the assert wants to check against either of the
two locks (one mutex or rcu read lock, for example) is held.

I'll send a patch after this one lands.

Thanks,

-- 
Peter Xu