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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Thu, Aug 01, 2024 at 10:14:26AM GMT, Oscar Salvador wrote:
> 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

I prefer this at a glance.

>
> 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 = {
>

I hate this to be honest, it's another 'we just have to remember to call an
arbitrary function' situation (why here and not elsewhere?) and
perpetuating the horrible if (hugetlb) { ... } approach to things.

I mean the shm code is _hateful_ anyway, but yeah I really really don't
like this.

I'd quite like us to add a check here for that function being NULL though,
I was mistaken in my previous reply saying we can't do anything here,
actually you can return an error, and so I'd prefer for us to return an
error in that case.

>
> Still unsure about which approach looks more correct though.

I think I've made my point of view clear fwiw at least ;)

>
> --
> Oscar Salvador
> SUSE Labs