On Tue, Dec 10, 2024 at 8:19 AM Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> wrote:
>
>
>
> On 2024/12/6 20:58, Amir Goldstein wrote:
> > On Fri, Dec 6, 2024 at 11:45 AM Kefeng Wang <wangkefeng.wang@xxxxxxxxxx> 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@xxxxxxxxx>
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.
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.
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."
Thanks,
Amir.
