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

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

On 2024/12/11 17:43, Amir Goldstein wrote:
> On Tue, Dec 10, 2024 at 8:19 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>>
>>
>>
>> On 2024/12/6 20:58, Amir Goldstein wrote:
>>> On Fri, Dec 6, 2024 at 11:45 AM Kefeng Wang <wangkefeng.wang@huawei.com> 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);
>>>> +
>>>> +               if (ret)
>>>> +                       return ret;
>>>> +       }
>>>> +
>>>> +       return mm_get_unmapped_area(current->mm, file, addr, len, pgoff,
>>>> flags);
>>>> +}
>>>>
>>>> Correct me If I'm wrong.
>>>>
>>>
>>> You just need to be aware of the fact that between ovl_get_unmapped_area()
>>> and ovl_mmap(), ovl_real_file(file) could change from the lower file, to the
>>> upper file due to another operation that initiated copy-up.
>>
>> Not sure about this part(I have very little knowledge of ovl), do you
>> mean that we could not use ovl_real_file()?  The ovl_mmap() using
>> realfile = file->private_data, we may use similar way in
>> ovl_get_unmapped_area(). but I may have misunderstood.
>>
> 
> First of all, you may add to your patch:
> Acked-by: Amir Goldstein <amir73il@gmail.com>

Thanks,

> 
> I think this patch is fine as is.
> w.r.t. question about ovl_override_creds(), I think it is good practice to
> user mounter credentials when calling into real fs methods, regardless
> of the fact that in most cases known today the ->get_unmapped_area()
> methods do not check credentials.
> 
> My comment was referring to the fact that ovl_real_file(file), when called
> two subsequent times in a row (once from ovl_get_unmapped_area() and
> then again from ovl_mmap()) may not return the same realfile.
> 
> This is because during the lifetime of an overlayfs file/inode, its realinode/
> realfile can change once, in the event known as "copy-up", so you may
> start by calling ovl_get_unmapped_area() on a lower ext4 realfile and then end
> up actually mapping an upper tmpfs realfile, because someone has opened
> the overlayfs file for write in the meanwhile.

Got it, thanks for your detail explanation.

> 
> I guess in this corner case, the alignment may be wrong, or just too strict for
> the actual mapping, but it is not critical, so just FYI.

Yes, not critical, at least not too much worse.

Ovl is always lack of vma THP alignment or some other VMA allocation
requirements.

> There are worse issues with mmap of overlayfs file documented in:
> https://docs.kernel.org/filesystems/overlayfs.html#non-standard-behavior
> "If a file residing on a lower layer is opened for read-only and then
> memory mapped
>   with MAP_SHARED, then subsequent changes to the file are not reflected in the
>   memory mapping."

I think we could ignore above issue in this fixup and if there is a need 
to check vm_flags in ->get_unmapped_area(), we could deal with it later.

And Jinjiang, please send a v2 according to all the discussion.

Thanks.

> 
> Thanks,
> Amir.