On Thu, Oct 26, 2017 at 9:54 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> 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().
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 <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
