On Mon, Apr 24, 2017 at 11:14 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> For directory entries, non zero oe->numlower implies OVL_TYPE_MERGE.
> Define a new type flag OVL_TYPE_COPYUP to indicate that an entry is
> a target of a copy up.
>
> For directory entries COPYUP = MERGE && UPPER. For non-dir entries
> non zero oe->numlower implies COPYUP, but COPYUP does not imply
> non zero oe->numlower. COPYUP can also be set on lookup when detecting
> an overlay.fh xattr on a non-dir, even if that fh cannot be followed.
>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
> fs/overlayfs/namei.c | 3 +++
> fs/overlayfs/overlayfs.h | 2 ++
> fs/overlayfs/util.c | 12 ++++++++----
> 3 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 318092a..73a8879 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -386,6 +386,9 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> }
> if (d.opaque)
> type |= __OVL_PATH_OPAQUE;
> + /* overlay.fh xattr implies this is a copy up */
> + if (d.fh)
> + type |= __OVL_PATH_COPYUP;
> }
>
> /*
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 08002ce..d0bb538 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -13,11 +13,13 @@ enum ovl_path_type {
> __OVL_PATH_UPPER = (1 << 0),
> __OVL_PATH_MERGE = (1 << 1),
> __OVL_PATH_OPAQUE = (1 << 2),
> + __OVL_PATH_COPYUP = (1 << 3),
> };
>
> #define OVL_TYPE_UPPER(type) ((type) & __OVL_PATH_UPPER)
> #define OVL_TYPE_MERGE(type) ((type) & __OVL_PATH_MERGE)
> #define OVL_TYPE_OPAQUE(type) ((type) & __OVL_PATH_OPAQUE)
> +#define OVL_TYPE_COPYUP(type) ((type) & __OVL_PATH_COPYUP)
>
> #define OVL_XATTR_PREFIX XATTR_TRUSTED_PREFIX "overlay."
> #define OVL_XATTR_OPAQUE OVL_XATTR_PREFIX "opaque"
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index dba9753..89789bc 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -101,11 +101,15 @@ enum ovl_path_type ovl_update_type(struct dentry *dentry, bool is_dir)
> if (oe->__upperdentry) {
> type |= __OVL_PATH_UPPER;
> /*
> - * Non-dir dentry can hold lower dentry from before
> - * copy-up.
> + * oe->numlower implies a copy up, but copy up does not imply
> + * oe->numlower. It can also be set on lookup when detecting
> + * an overlay.fh xattr on a non-dir that cannot be followed.
The code looks fine, but I don't understand the comment. Why would we
set COPYUP flag when the fh cannot be followed?
The reason I think the COPYUP vs. MERGE distinction is needed is the
ovl_check_empty_and_clear() thing. It starts with a merged directory
with some whiteouts in it and exchanges it with an empty and opaque
directory. Normally the empty directory will be deleted immediately,
but if something fails during the deletion, then it will remain there.
The overlay is left in a consistent state, but the association with
the original inode should still remain, so it will have COPYUP but not
MERGE.
Now the current code is actually broken, because we leave the old,
replaced directory in __upperdentry as well as the rest of the lower
stack. So should the deletion fail after the replacement things won't
work properly.
I think we can fix that by replacing __upperdentry. Luckily we are
under inode lock, so protected against concurrent readdir or creation
inside the directory. Then we have lifetime problems. Until now a
positive __upperdentry was assumed to have a lifetime equal to that of
the overlay dentry. We'd need an old_upperdentry to save it. I think
that's it it, but maybe there are other issues.
Thanks,
Miklos
