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

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2024/12/6 19:53, Lorenzo Stoakes wrote:
> 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.?

Not familiar with ovl, but we convert from file to realfile, adding cred
to prevent potential problems. maybe Amir could help to confirm it  :)

> 
>> +
>> +               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.

Yes, should directly return.

> 
>> +       }
>> +
>> +       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?
> 

Great, I thin Jinjiang could continue to this work.