On Wed, Mar 7, 2018 at 3:29 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> On Wed, Mar 07, 2018 at 09:15:40AM +0200, Amir Goldstein wrote:
>> On Tue, Mar 6, 2018 at 10:54 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>> > d_real() can make a upper metacopy dentry/inode visible to the vfs layer.
>> > This is something new and vfs layer does not know that this inode contains
>> > only metadata and not data. And this could break things.
>> >
>> > So to be safe, do not expose metacopy only dentry/inode to vfs using
>> > d_real().
>> >
>> > IOW, d_real() will not reuturn metacopy dentry. Instead, it will return
>> > dentry corresponding lower dentry/inode which has file data.
>> >
>> > For regular d_real() call (inode == NULL, D_REAL_UPPER not set), if upper
>> > dentry inode is metacopy only and does not have data, return lower dentry.
>> >
>> > If d_real() is called with flag D_REAL_UPPER, return upper dentry only if
>> > it has data (flag OVL_UPPERDATA is set).
>> >
>> > Similiarly, if d_real(inode=X) is called, a warning is emitted if returned
>> > dentry/inode does not have OVL_UPPERDATA set. This should not happen as
>> > we never made this metacopy inode visible to vfs so nobody should be
>> > calling overlayfs back with inode=metacopy_inode.
>> >
>> > I scanned the code and I don't think it breaks any of the existing code.
>> > There are two users of D_REAL_UPPER. may_write_real() and
>> > update_ovl_inode_times().
>> >
>> > may_write_real(), will get an NULL dentry if upper inode is metacopy only
>> > and it will return -EPERM. Effectively, we are disallowing modifications
>> > to metacopy only inode from this interface. Though there is opportunity
>> > to improve it. (Allow chattr on metacopy inodes).
>> >
>> > update_ovl_inode_times() gets inode mtime and ctime from real inode. It
>> > should not be broken for metacopy inode as well for following reasons.
>> >
>> > - For any metadata operations (setattr, acl etc), overlay always calls
>> > ovl_copyattr() and updates ovl inode mtime and ctime. So there is no
>> > need to update mtime and ctime in this case. Its already updated, hence
>> > even if d_real(D_REAL_UPPER) returns nil, it should be fine.
>> >
>> > - For metadata inode, mtime should be same as lower and not change. (data
>> > can't be modified on metadata inode without copyup). IOW, mtime of
>> > ovl dentry should be same as mtime of underlying metadata inode on upper
>> > always. So there is no need to update it.
>> >
>> > - For file writes, ctime and mtime will be updated. But in that case
>> > first data will be copied up and this will not be a metadata inode
>> > anymore. And furthr call to d_real(D_REAL_UPPER) will return upper
>> > inode and new mtime and ctime will be obtainable.
>> >
>> > So atime updates should work just fine for metacopy inodes. I think only
>> > corner case is if somehow underlying filesystem changes ctime of upper
>> > metadata inode without overlay knowing about it. Not sure how that
>> > can happen. If somehow is affected by that, then we probably can implement
>> > another flag which will allow caller to get metacopy inode as well.
>> > Something like d_real(D_REAL_UPPER | D_METACOPY). And that should solve
>> > this issue.
>> >
>> > Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
>> > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
>> > ---
>> > fs/overlayfs/overlayfs.h | 1 +
>> > fs/overlayfs/super.c | 21 +++++++++++++++++----
>> > fs/overlayfs/util.c | 8 ++++++++
>> > 3 files changed, 26 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>> > index 2d682923252e..24725b6668b9 100644
>> > --- a/fs/overlayfs/overlayfs.h
>> > +++ b/fs/overlayfs/overlayfs.h
>> > @@ -225,6 +225,7 @@ void ovl_path_lowerdata(struct dentry *dentry, struct path *path);
>> > enum ovl_path_type ovl_path_real(struct dentry *dentry, struct path *path);
>> > struct dentry *ovl_dentry_upper(struct dentry *dentry);
>> > struct dentry *ovl_dentry_lower(struct dentry *dentry);
>> > +struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
>> > struct dentry *ovl_dentry_real(struct dentry *dentry);
>> > struct dentry *ovl_i_dentry_upper(struct inode *inode);
>> > struct inode *ovl_inode_upper(struct inode *inode);
>> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> > index d3dbdd695722..4be4e47cbf57 100644
>> > --- a/fs/overlayfs/super.c
>> > +++ b/fs/overlayfs/super.c
>> > @@ -96,8 +96,14 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>> > struct dentry *real;
>> > int err;
>> >
>> > - if (flags & D_REAL_UPPER)
>> > - return ovl_dentry_upper(dentry);
>> > + if (flags & D_REAL_UPPER) {
>> > + real = ovl_dentry_upper(dentry);
>> > + if (!real)
>> > + return NULL;
>> > + if (!ovl_has_upperdata(dentry))
>> > + return NULL;
>> > + return real;
>> > + }
>> >
>> > if (!d_is_reg(dentry)) {
>> > if (!inode || inode == d_inode(dentry))
>> > @@ -113,15 +119,22 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>> >
>> > real = ovl_dentry_upper(dentry);
>> > if (real && (!inode || inode == d_inode(real))) {
>> > + bool metacopy = !ovl_has_upperdata(dentry);
>> > if (!inode) {
>> > err = ovl_check_append_only(d_inode(real), open_flags);
>> > if (err)
>> > return ERR_PTR(err);
>> > - }
>> > +
>> > + if (unlikely(metacopy))
>> > + goto lower;
>> > + } else if (unlikely(metacopy))
>> > + goto bug;
>> > +
>> > return real;
>> > }
>> >
>> > - real = ovl_dentry_lower(dentry);
>> > +lower:
>> > + real = ovl_dentry_lowerdata(dentry);
>> > if (!real)
>> > goto bug;
>> >
>> > diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
>> > index 274bbfc855e0..36d41f7001e3 100644
>> > --- a/fs/overlayfs/util.c
>> > +++ b/fs/overlayfs/util.c
>> > @@ -186,6 +186,14 @@ struct dentry *ovl_dentry_lower(struct dentry *dentry)
>> > return oe->numlower ? oe->lowerstack[0].dentry : NULL;
>> > }
>> >
>> > +struct dentry *ovl_dentry_lowerdata(struct dentry *dentry)
>> > +{
>> > + struct ovl_entry *oe = dentry->d_fsdata;
>> > + int idx = oe->numlower - 1;
>> > +
>> > + return oe->lowerstack[idx].dentry;
>> > +}
>> > +
>>
>> This new change is not in line with the subject line.
>> Either change the commit message to fit or better split this
>> small change to a new patch because the commit message is long
>> enough as it is.
>
> Ok, I will move this helper in a separate patch before this patch.
>
It's not just the helper. The subject says "Don't expose metacopy upper"
but this helper is used to "not expose metacopy lower", so either amend
the commit message or fix exposing metacopy lower in a separate patch.
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
