From mboxrd@z Thu Jan 1 00:00:00 1970 From: Amir Goldstein Subject: Re: [PATCH 12/13] ovl: Do not export metacopy only upper dentry Date: Thu, 26 Oct 2017 09:54:56 +0300 Message-ID: References: <1508958575-14086-1-git-send-email-vgoyal@redhat.com> <1508958575-14086-13-git-send-email-vgoyal@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: Received: from mail-yw0-f193.google.com ([209.85.161.193]:43883 "EHLO mail-yw0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750957AbdJZGy5 (ORCPT ); Thu, 26 Oct 2017 02:54:57 -0400 Received: by mail-yw0-f193.google.com with SMTP id y75so2098031ywg.0 for ; Wed, 25 Oct 2017 23:54:56 -0700 (PDT) In-Reply-To: Sender: linux-unionfs-owner@vger.kernel.org List-Id: linux-unionfs@vger.kernel.org To: Vivek Goyal Cc: overlayfs , Miklos Szeredi On Thu, Oct 26, 2017 at 9:54 AM, Amir Goldstein wrote: > On Wed, Oct 25, 2017 at 10:09 PM, Vivek Goyal 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(). Also please change subject and this line to "do not expose..." >> >> 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 >> --- >> 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html