On Fri, Sep 8, 2023 at 10:43 AM Alexander Larsson <alexl@xxxxxxxxxx> wrote:
>
>
>
> On Thu, Sep 7, 2023 at 1:25 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>>
>> On Thu, Sep 7, 2023 at 11:44 AM Alexander Larsson <alexl@xxxxxxxxxx> wrote:
>> >
>> > There are cases where you want to use an overlayfs mount as a lowerdir
>> > for another overlayfs mount. For example, if the system rootfs is on
>> > overlayfs due to composefs, or to make it volatile (via tmps), then
>> > you cannot currently store a lowerdir on the rootfs. This means you
>> > can't e.g. store on the rootfs a prepared container image for use
>> > using overlayfs.
>> >
>> > To work around this, we introduce an escapment mechanism for overlayfs
>> > xattrs. Whenever the lower/upper dir has a xattr named
>> > `overlay.overlay.XYZ`, we list it as overlay.XYZ in listxattrs, and
>> > when the user calls getxattr or setxattr on `overlay.XYZ`, we apply to
>> > `overlay.overlay.XYZ` in the backing directories.
>>
>> Please use ""
>
>
> I like backquotes, because the github/gitlab UIs show commit messages as markdown, and
> there it renders the quoted part monospace, whereas " is just a regular character with no special rendering.
> But I changed it.
>
Well. It's not critical to me. just thought it would be more consistent with
comment and doc style.
>>
>> >
>> > This allows storing any kind of overlay xattrs in a overlayfs mount
>> > that can be used as a lowerdir in another mount. It is possible to
>> > stack this mechanism multiple times, such that
>> > overlay.overlay.overlay.XYZ will survive two levels of overlay mounts,
>> > however this is not all that useful in practice because of stack depth
>> > limitations of overlayfs mounts.
>> >
>> > Signed-off-by: Alexander Larsson <alexl@xxxxxxxxxx>
>>
>> With comment in ovl_listxattr() below fixed you may add:
>>
>> Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
>>
>> > ---
>> > fs/overlayfs/overlayfs.h | 7 ++++
>> > fs/overlayfs/xattrs.c | 78 +++++++++++++++++++++++++++++++++++++---
>> > 2 files changed, 81 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
>> > index dff7232b7bf5..736d7f952a8e 100644
>> > --- a/fs/overlayfs/overlayfs.h
>> > +++ b/fs/overlayfs/overlayfs.h
>> > @@ -32,6 +32,13 @@ enum ovl_path_type {
>> > #define OVL_XATTR_USER_PREFIX XATTR_USER_PREFIX OVL_XATTR_NAMESPACE
>> > #define OVL_XATTR_USER_PREFIX_LEN (sizeof(OVL_XATTR_USER_PREFIX) - 1)
>> >
>> > +#define OVL_XATTR_ESCAPE_PREFIX OVL_XATTR_NAMESPACE
>> > +#define OVL_XATTR_ESCAPE_PREFIX_LEN (sizeof(OVL_XATTR_ESCAPE_PREFIX) - 1)
>> > +#define OVL_XATTR_ESCAPE_TRUSTED_PREFIX OVL_XATTR_TRUSTED_PREFIX OVL_XATTR_ESCAPE_PREFIX
>> > +#define OVL_XATTR_ESCAPE_TRUSTED_PREFIX_LEN (sizeof(OVL_XATTR_ESCAPE_TRUSTED_PREFIX) - 1)
>> > +#define OVL_XATTR_ESCAPE_USER_PREFIX OVL_XATTR_USER_PREFIX OVL_XATTR_ESCAPE_PREFIX
>> > +#define OVL_XATTR_ESCAPE_USER_PREFIX_LEN (sizeof(OVL_XATTR_ESCAPE_USER_PREFIX) - 1)
>> > +
>> > enum ovl_xattr {
>> > OVL_XATTR_OPAQUE,
>> > OVL_XATTR_REDIRECT,
>> > diff --git a/fs/overlayfs/xattrs.c b/fs/overlayfs/xattrs.c
>> > index b8ea96606ea8..27b31f812eb1 100644
>> > --- a/fs/overlayfs/xattrs.c
>> > +++ b/fs/overlayfs/xattrs.c
>> > @@ -4,6 +4,18 @@
>> > #include <linux/xattr.h>
>> > #include "overlayfs.h"
>> >
>> > +bool ovl_is_escaped_xattr(struct super_block *sb, const char *name)
>> > +{
>> > + struct ovl_fs *ofs = sb->s_fs_info;
>> > +
>> > + if (ofs->config.userxattr)
>> > + return strncmp(name, OVL_XATTR_ESCAPE_USER_PREFIX,
>> > + OVL_XATTR_ESCAPE_USER_PREFIX_LEN) == 0;
>> > + else
>> > + return strncmp(name, OVL_XATTR_ESCAPE_TRUSTED_PREFIX,
>> > + OVL_XATTR_ESCAPE_TRUSTED_PREFIX_LEN - 1) == 0;
>> > +}
>> > +
>> > bool ovl_is_private_xattr(struct super_block *sb, const char *name)
>> > {
>> > struct ovl_fs *ofs = OVL_FS(sb);
>> > @@ -82,8 +94,8 @@ static int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char
>> >
>> > static bool ovl_can_list(struct super_block *sb, const char *s)
>> > {
>> > - /* Never list private (.overlay) */
>> > - if (ovl_is_private_xattr(sb, s))
>> > + /* Never list non-escaped private (.overlay) */
>> > + if (ovl_is_private_xattr(sb, s) && !ovl_is_escaped_xattr(sb, s))
>> > return false;
>> >
>> > /* List all non-trusted xattrs */
>> > @@ -97,10 +109,12 @@ static bool ovl_can_list(struct super_block *sb, const char *s)
>> > ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
>> > {
>> > struct dentry *realdentry = ovl_dentry_real(dentry);
>> > + struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
>> > ssize_t res;
>> > size_t len;
>> > char *s;
>> > const struct cred *old_cred;
>> > + size_t prefix_len;
>> >
>> > old_cred = ovl_override_creds(dentry->d_sb);
>> > res = vfs_listxattr(realdentry, list, size);
>> > @@ -108,6 +122,9 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
>> > if (res <= 0 || size == 0)
>> > return res;
>> >
>> > + prefix_len = ofs->config.userxattr ?
>> > + OVL_XATTR_USER_PREFIX_LEN : OVL_XATTR_TRUSTED_PREFIX_LEN;
>> > +
>> > /* filter out private xattrs */
>> > for (s = list, len = res; len;) {
>> > size_t slen = strnlen(s, len) + 1;
>> > @@ -120,6 +137,12 @@ ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
>> > if (!ovl_can_list(dentry->d_sb, s)) {
>> > res -= slen;
>> > memmove(s, s + slen, len);
>> > + } else if (ovl_is_escaped_xattr(dentry->d_sb, s)) {
>> > + memmove(s + prefix_len,
>> > + s + prefix_len + OVL_XATTR_ESCAPE_PREFIX_LEN,
>> > + slen - (prefix_len + OVL_XATTR_ESCAPE_PREFIX_LEN) + len);
>> > + res -= OVL_XATTR_ESCAPE_PREFIX_LEN;
>> > + s += slen - OVL_XATTR_ESCAPE_PREFIX_LEN;
>>
>> It's a bit hard to follow. how about:
>>
>> res -= OVL_XATTR_ESCAPE_PREFIX_LEN;
>> name_len = slen - prefix_len - OVL_XATTR_ESCAPE_PREFIX_LEN;
>> s += prefix_len;
>> memmove(s, s + OVL_XATTR_ESCAPE_PREFIX_LEN, name_len + len);
>> s += OVL_XATTR_ESCAPE_PREFIX_LEN + name_len;
>>
>
> The last line needs to be just s+= name_len, but yeah, this looks nicer.
>
Cool. no need to post v4 just for this.
You can push to your branch and once we have feedback from Miklos
we can take it from there.
Thanks,
Amir.
