Re: [POC/RFC PATCH] overlayfs: fix data inconsistency at copy up

[Amir Goldstein]

On Sat, Oct 22, 2016 at 10:24 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> On Fri, Oct 21, 2016 at 11:13 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>> On Fri, Oct 21, 2016 at 11:53:41AM +0300, Amir Goldstein wrote:
>>> On Thu, Oct 20, 2016 at 11:54 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>>> > On Thu, Oct 20, 2016 at 04:46:30PM -0400, Vivek Goyal wrote:
>>> >
>>> > [..]
>>> >> > +static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *to)
>>> >> > +{
>>> >> > +   struct file *file = iocb->ki_filp;
>>> >> > +   bool isupper = OVL_TYPE_UPPER(ovl_path_type(file->f_path.dentry));
>>> >> > +   ssize_t ret = -EINVAL;
>>> >> > +
>>> >> > +   if (likely(!isupper)) {
>>> >> > +           const struct file_operations *fop = ovl_real_fop(file);
>>> >> > +
>>> >> > +           if (likely(fop->read_iter))
>>> >> > +                   ret = fop->read_iter(iocb, to);
>>> >> > +   } else {
>>> >> > +           struct file *upperfile = filp_clone_open(file);
>>> >> > +
>>> >>
>>> >> IIUC, every read of lower file will call filp_clone_open(). Looking at the
>>> >> code of filp_clone_open(), I am concerned about the overhead of this call.
>>> >> Is it significant? Don't want to be paying too much of penalty for read
>>> >> operation on lower files. That would be a common case for containers.
>>> >>
>>> >
>>> > Looks like I read the code in reverse. So if I open a file read-only,
>>> > and if it has not been copied up, I will simply call read_iter() on
>>> > lower filesystem. But if file has been copied up, then I will call
>>> > filp_clone_open() and pay the cost. And this will continue till this
>>> > file is closed by caller.
>>> >
>>>
>>> I wonder if that cost could be reduced by calling replace_fd() or
>>> some variant of it to install the cloned file onto the rofd after the
>>> first access??
>>
>> Hmm.., Interesting. Will something like following work? This applies on
>> top of Miklos's patch. It seems to work for me. It might be completely
>> broken/racy though. Somebody who understands this code well, will have
>> to have a look.
>>
>
> The idea sounded scary already when I suggested it :)
> See below what I think is scary about this implementation...
>
> Thanks for following through.
>
>
>> ---
>>  fs/file.c            |   41 +++++++++++++++++++++++++++++++++++++++++
>>  fs/overlayfs/inode.c |    1 +
>>  2 files changed, 42 insertions(+)
>>
>> Index: rhvgoyal-linux/fs/overlayfs/inode.c
>> ===================================================================
>> --- rhvgoyal-linux.orig/fs/overlayfs/inode.c    2016-10-21 15:43:05.391488406 -0400
>> +++ rhvgoyal-linux/fs/overlayfs/inode.c 2016-10-21 16:07:57.409420795 -0400
>> @@ -416,6 +416,7 @@ static ssize_t ovl_read_iter(struct kioc
>>                 if (IS_ERR(upperfile)) {
>>                         ret = PTR_ERR(upperfile);
>>                 } else {
>> +                       replace_file(file, upperfile);
>
> When fdtable is not shared (single threaded process), after this call
> I think that file pointer
> may be free (?), because file is not reference counted.
> Although I did not see any code in VFS callers trying to dereference
> the file pointer after
> calling read_iter(), this seems like a dangerous practice, so will
> need to a way to fix that.
>

My bad. file pointer is freed in work_task_run(), so replace_file()
should be just as safe as replace_fd() and do_dup2().