On Thu, Nov 9, 2017 at 10:50 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> If a file is copied up metadata only and later when same file is opened
> for WRITE, then data copy up takes place. We copy up data, remove METACOPY
> xattr and then set the UPPERDATA flag in ovl_entry->flags. While all
> these operations happen with oi->lock held, read side of oi->flags is
> lockless. That is another thread on another cpu can check if UPPERDATA
> flag is set or not.
>
> So this gives us an ordering requirement w.r.t UPPERDATA flag. That is, if
> another cpu sees UPPERDATA flag set, then it should be guaranteed that
> effects of data copy up and remove xattr operations are also visible.
>
> For example.
>
> CPU1 CPU2
> ovl_d_real() acquire(oi->lock)
> ovl_copy_up_flags() ovl_copy_up_data()
> ovl_dentry_needs_data_copy_up() vfs_removexattr()
> ovl_test_flag(OVL_UPPERDATA) ovl_set_flag(OVK_UPPERDATA)
> release(oi->lock)
typo OVK_UPPERDATA
CPU1:
ovl_d_real()
ovl_open_maybe_copy_up()
ovl_open_need_copy_up()
ovl_dentry_needs_data_copy_up()
ovl_test_flag(OVL_UPPERDATA)
That's the execution path I meant you should specify, the execution path
that could expose incomplete upper to vfs from d_real().
>
> Say CPU2 is copying up data and in the end sets UPPERDATA flag. But if
> CPU1 perceives the effects of setting UPPERDATA flag but not effects of
> preceeding operations, that would be a problem.
>
Instead of "that would be a problem", you can say that upper that is
not fully copied
up is exposed to vfs.
> Hence this patch introduces smp_wmb() on setting UPPERDATA flag operation
> and smp_rmb() on UPPERDATA flag test operation.
>
> May be some other lock or barrier is already covering it. But I am not sure
> what that is and is it obvious enough that we will not break it in future.
>
> So hence trying to be safe here and introducing barriers explicitly for
> UPPERDATA flag/bit.
>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
> fs/overlayfs/util.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index f70eea77c5c7..81b307c20a8d 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -267,13 +267,26 @@ bool ovl_has_upperdata(struct dentry *dentry) {
> if (!ovl_should_check_upperdata(dentry))
> return true;
>
> - return ovl_test_flag(OVL_UPPERDATA, d_inode(dentry));
> + if (!ovl_test_flag(OVL_UPPERDATA, d_inode(dentry)))
> + return false;
> + /*
> + * Pairs with smp_wmb() in ovl_copy_up_meta_inode_data(). Make sure
> + * if setting of OVL_UPPERDATA is visible, then effects of writes
> + * before that are visible too.
> + */
> + smp_rmb();
> + return true;
> }
>
I think it is not right to have smp_rmb() is the locked path where it
is not needed
too costly.
We should probably have a different variant for ovl_already_copied_up_locked()
and if OVL_UPPERDATA is set for all non-eligible cases, ovl_has_upperdata()
can have the smp_rmb() and locked variant can just can check the flag directly.
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
