Re: [PATCH v2 6/9] mm: Make hugetlb mappings go through mm_get_unmapped_area_vmflags

[Oscar Salvador]

On Wed, Jul 31, 2024 at 05:15:41PM +0100, Lorenzo Stoakes wrote:
> I've not got the vm debug on in my build, so it's blowing up here for me:
> 
> static unsigned long shm_get_unmapped_area(struct file *file,
> 	unsigned long addr, unsigned long len, unsigned long pgoff,
> 	unsigned long flags)
> {
> 	struct shm_file_data *sfd = shm_file_data(file);
> 
> 	return sfd->file->f_op->get_unmapped_area(sfd->file, addr, len,
> 						pgoff, flags);
> }
> 
> Notice that that doesn't check whether sfd->file->f_op->get_unmapped_area
> is NULL.

I see now, thanks.

> So since you remove this from the f_ops, it causes a NULL pointer deref.
...
> static const struct file_operations shm_file_operations = {
> ..
> 	.get_unmapped_area	= shm_get_unmapped_area,
> ...
> };
> 
> Then this get_area() is invoked, which calls shm_get_unmapped_area(), which
> calls f_op->get_unmapped_area() on your hugetlbfs_file_operations object
> which you just deleted and it's NULL.
> 
> This is why you have to be super careful here, there's clearly stuff out
> there that assumes that this can't happen, which you need to track down.
> 
> A quick grep however _suggests_ this might be the one landmine place. But
> you need to find a smart way to deal with this.

Probably, the most straightforward way to fix this is to instead of
setting .get_unmapped_area to NULL for hugetlbfs_file_operations, would
be to have it re-defined like:

 .get_unmapped_area = mm_get_unmapped_area_vmflags

Which is what we call after this patchset.
So no more things have to tweaked.

On a more correct way, __maybe__ have something like:


 diff --git a/ipc/shm.c b/ipc/shm.c
 index 3e3071252dac..222dca8a3716 100644
 --- a/ipc/shm.c
 +++ b/ipc/shm.c
 @@ -648,8 +648,11 @@ static unsigned long shm_get_unmapped_area(struct file *file,
  {
  	struct shm_file_data *sfd = shm_file_data(file);
  
 -	return sfd->file->f_op->get_unmapped_area(sfd->file, addr, len,
 +	if (sfd->file->f_op->get_unmapped_area)
 +		return sfd->file->f_op->get_unmapped_area(sfd->file, addr, len,
  						pgoff, flags);
 +
 +	return mm_get_unmapped_area_vmflags(sfd->file, addr, len, pgoff, flags);
  }
  
  static const struct file_operations shm_file_operations = {

 
Still unsure about which approach looks more correct though.

-- 
Oscar Salvador
SUSE Labs