On Fri, May 19, 2017 at 12:35 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> On Sat, May 13, 2017 at 4:25 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>> On Thu, May 11, 2017 at 4:42 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>>> This patch is based on an earlier POC by Miklos Szeredi.
>>>
>>> When all layers are on the same fs, and iterating a directory which
>>> may contain copy up entries, call vfs_getattr() on the overlay entries
>>> to make sure that d_ino will be consistent with st_ino from stat(2).
>>>
>> [...]
>>>
>>> +/*
>>> + * Set d_ino for upper entries. Non-upper entries should always report
>>> + * the uppermost real inode ino and should not call this function.
>>> + *
>>> + * When not all layer are on same fs, report real ino also for upper.
>>> + *
>>> + * When all layers are on the same fs, and upper has a reference to
>>> + * copy up origin, call vfs_getattr() on the overlay entry to make
>>> + * sure that d_ino will be consistent with st_ino from stat(2).
>>> + */
>>> +static int ovl_cache_update_ino(struct path *path, struct ovl_cache_entry *p)
>>> +
>>> +{
>>> + struct dentry *dir = path->dentry;
>>> + struct dentry *this = NULL;
>>> + enum ovl_path_type type;
>>> + u64 ino = p->real_ino;
>>> + int err = 0;
>>> +
>>> + if (!ovl_same_sb(dir->d_sb) || WARN_ON(p->idx))
>>> + goto out;
>>> +
>>> + if (p->name[0] == '.') {
>>> + if (p->len == 1) {
>>> + this = dget(dir);
>>> + goto get;
>>> + }
>>> + if (p->len == 2 && p->name[1] == '.') {
>>> + /* we shall not be moved */
>>> + this = dget(dir->d_parent);
>>> + goto get;
>>> + }
>>> + }
>>> + this = lookup_one_len(p->name, dir, p->len);
>>
>> Mmm... that reeks of layer violation, but I can't find anything obvious
>> that could go wrong from calling lookup on overlay dir here, is there?
>>
>> It's just so convenient to use overlay lookup here, get the overlay
>> entry from cache and stat it, so I can't think of a better alternative.
>>
>> BTW, couldn't find anything obviously wrong with stat'ing overlay
>> dir and dir->d_parent here either, is there?
>
> One issue is permission checking: lookup needs MAY_EXEC while readdir
> needs MAY_READ. So we can't just call lookup_one_len(), but will have
> to do the thing by hand. Also no need to construct the overlay
> dentry, just pre-loading the underlying dentries will have almost all
> the pros (and cons).
>
OK. Let me see if I get this.
We will need to do by hand:
upperdentry = lookup_one_len(p->name, upperdir, p->len);
ovl_check_origin(dir, upperdentry, &originpath);
And then call some diminished variant of ovl_getattr() to
stat them both and figure out the official ino.
Not the end of the world when lower and upper dentries are
cached as you noted.
BUT, for lookup_one_len(upperdir) we anyway need to
ovl_override_creds().
Doesn't that pull the rug from under your MAY_EXEC argument?
May as well do ovl_override_creds(); lookup_one_len(dir)
and be done with it.
Unless there are other reasons why this is wrong?
Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
