Re: [PATCH -next] ovl: respect underlying filesystem's get_unmapped_area()

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

On Fri, Dec 06, 2024 at 06:45:11PM +0800, Kefeng Wang wrote:
> So maybe use mm_get_unmapped_area() instead of __get_unmapped_area(),
> something like below,
>
> +static unsigned long ovl_get_unmapped_area(struct file *file,
> +               unsigned long addr, unsigned long len, unsigned long pgoff,
> +               unsigned long flags)
> +{
> +       struct file *realfile;
> +       const struct cred *old_cred;
> +
> +       realfile = ovl_real_file(file);
> +       if (IS_ERR(realfile))
> +               return PTR_ERR(realfile);
> +
> +       if (realfile->f_op->get_unmapped_area) {
> +               unsigned long ret;
> +
> +               old_cred = ovl_override_creds(file_inode(file)->i_sb);
> +               ret = realfile->f_op->get_unmapped_area(realfile, addr, len,
> +                                                       pgoff, flags);
> +               ovl_revert_creds(old_cred);

Credentials stuff necessary now you're not having security_mmap_addr()
called etc.?

> +
> +               if (ret)
> +                       return ret;

Surely you'd unconditionally return in this case? I don't think there's any case
where you'd want to fall through.

> +       }
> +
> +       return mm_get_unmapped_area(current->mm, file, addr, len, pgoff,
> flags);
> +}
>
> Correct me If I'm wrong.
>
> Thanks.
>

I mean this doesn't export anything we don't want exported so this is fine
from that perspective :)

And I guess in principle this is OK in that __get_unmapped_area() would be
invoked on the overlay file, will do the required arch_mmap_check(), then
will invoke your overlay handler.

I did think of suggesting invoking the f_op directly, but it feels icky
vs. just supporting large folios.

But actually... hm I realise I overlooked the fact that underlying _files_
will always provide a large folio-aware handler.

I'm guessing you can't use overlayfs somehow with a MAP_ANON | MAP_SHARED
mapping or similar, thinking of:

	if (file) {
		...
	} else if (flags & MAP_SHARED) {
		/*
		 * mmap_region() will call shmem_zero_setup() to create a file,
		 * so use shmem's get_unmapped_area in case it can be huge.
		 */
		get_area = shmem_get_unmapped_area;
	}

But surely actually any case that works with overlayfs will have a file and
so... yeah.

Hm, I actually think this should work.

Can you make sure to do some pretty thorough testing on this just to make
sure you're not hitting on any weirdness?