Re: [RFC][PATCH 02/13] ovl: redirect dir by file handle on copy up

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Apr 21, 2017 at 6:02 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> On Thu, Apr 20, 2017 at 10:55 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
>> File handles are only unique for a single fs namespace.
>> Right now, the copy up stores the lower handle in overlay.fh
>> without storing lower layer number and/or lower fs device id.
>
> Hmm, would need to be careful with the security implications of
> decoding fh on the wrong fs.
>
>>
>> The overlay.fh format has a header with version and magic:
>> struct ovl_fh {
>>         unsigned char version;  /* 0 */
>>         unsigned char magic;    /* 0xfb */
>>
>> So we can easily extend this format later on to support non
>> same fs redirect_fh.
>>
>> That said, to answer your question, there is no disadvantage to
>> storing the lower handle for non-samefs case, there is only
>> a problem with lookup in the case of several lower layers
>> no on the same fs.
>
> Why?
>

Because decoding an fh takes a vfsmnt as input arg, looking up
by fh is implemented:

    for (i = 0; i < numlower; i++) {
          this = ovl_lookup_fh(lowerstack[i].mnt, d.fh);
          if (this == NULL)
               continue;
    }

and ovl_lookup_fh() even verifies ovl_dentry_under_mnt() for
accepting the decoded dentry.

So unless we are in the "same lower sb" case this lookup
can return a dentry from the wrong layer.

>> So we can actually relax the samefs constrain for redirect_fh
>> to exclude upper layer and specifically, we can always use redirect_fh
>> in the single lower layer case.
>>
>> There is one more advantage to storing overlay.fh unconditionally.
>> Even with multi non-samefs lower layers, the overlay.fh on upper
>> can be used to distinguish between "merge" and "pure" upper
>> for non dirs.
>>
>> I would like to extend the meaning of OVL_TYPE_MERGE
>> to represent an upper non-dir that has been copied up.
>> Patch #5 already takes the first step and sets numlower = 1
>> for non-dirs that have been copied up on samefs.
>>
>> The next steps would be:
>> 1. remove d_is_dir() check from ovl_path_type()
>>     BTW, can you please explain this comment.
>>     I don't understand how this could be:
>>
>>                 /*
>>                  * Non-dir dentry can hold lower dentry from previous
>>                  * location.
>>                  */
>>                 if (oe->numlower && d_is_dir(dentry))
>>                         type |= __OVL_PATH_MERGE;
>
> I guess the comment says, that when a non-dir is copied up it will
> have positive __upperdentry and a positive numlower (actually, always
> one).

Ok, I guess I wasn't sure what "previous location" was referring to
it implied rename.

>
> If you remove the d_is_dir() then it will change the meaning from
> "merge" to "copied up".  The two meanings are distinct: consider the
> ovl_clear_empty() case.  It starts with a merged dir and ends up with
> an opaque one.  But the opaque one is still the same object as the
> original, lower one, so both need to have the same overlay.fh
> attribute.
>

Well, technically, an opaque object should not have overlay.fh
but I see that it can happen with an unhashed just-deleted entry.
so yes, need to be able to make that distinction.

>>
>> 2. check merge_or_lower() in ovl_rename() also for non-dir case
>>     this will cause setting overlay.redirect on non-dir
>>     and then hardlinks could work better when copying layers
>>     even without reverse mapping and recovery
>>
>> 3. set __OVL_PATH_MERGE flag during lookup for non-samefs
>>     In non-samefs case, __OVL_PATH_MERGE may not always be
>>     deduced from numlower, so need to use my patch that add
>>     ovl_update_type() [1]
>
> So lets not confuse "merge" with "copied up".  The two are similar for
> directories, but not the same, and "merge" doesn't make sense for
> regular files.
>

It make sense if you consider that the overlay inode of non-dir is a merge
of the upper attributes and data and the lower ino (and maybe st_dev).

But it is a hack nonetheless, so I'll think up a new flag name for
COPIED_UP. suggestions are welcome.

Thanks,
Amir.



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux