On Wed, Oct 25, 2017 at 10:09 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 export metacopy only dentry/inode to vfs using d_real().
>
> 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 int his case. Its already updated.
>
> - For metadata inode, mtime should be same as lower and not change. (data
> can't be modified on metadata inode without copyup).
>
> - 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.
>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
> fs/overlayfs/super.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index e97dccb..dc8909a 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -80,8 +80,18 @@ 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;
bool ovl_dentry_is_metacopy(dentry) {
> + if (!ovl_dentry_check_upperdata(dentry))
> + return false;
> + if (!ovl_test_flag(OVL_UPPERDATA, d_inode(dentry)))
> + return true;
> + /* Pairs with smp_wmb() in ovl_copy_up_meta_inode_data() */
> + smp_rmb();
return false;
}
to be used 2 times in ovl_d_real() and in ovl_getattr()
> + return real;
> + }
>
> if (!d_is_reg(dentry)) {
> if (!inode || inode == d_inode(dentry))
> @@ -113,6 +123,9 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
> smp_rmb();
> }
> }
> +
> + WARN_ON(ovl_dentry_check_upperdata(dentry) &&
> + !ovl_test_flag((OVL_UPPERDATA), d_inode(dentry)));
Instead of checking flag twice, check it once above the if (!inode), e.g:
bool metacopy = ovl_dentry_is_metacopy(dentry);
if (!inode) {
...
if (unlikely(metacopy))
goto lower;
} else if (unlikely(metacopy))
goto bug;
}
> return real;
> }
>
> --
> 2.5.5
>
> --
> 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
--
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
