On Fri, Oct 13, 2023 at 6:36 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> On Wed, 11 Oct 2023 at 18:46, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> > Before commit b36a5780cb44 ("ovl: modify layer parameter parsing"),
> > spaces and commas in lowerdir mount option value used to be escaped using
> > seq_show_option().
> >
> > In current upstream, when lowerdir value has a space, it is not escaped
> > in /proc/mounts, e.g.:
> >
> > none /mnt overlay rw,relatime,lowerdir=l l,upperdir=u,workdir=w 0 0
> >
> > which results in broken output of the mount utility:
> >
> > none on /mnt type overlay (rw,relatime,lowerdir=l)
> >
> > Store the original lowerdir mount options before unescaping and show
> > them using the same escaping used for seq_show_option() in addition to
> > escaping the colon separator character.
> >
> > Fixes: b36a5780cb44 ("ovl: modify layer parameter parsing")
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> > fs/overlayfs/params.c | 38 +++++++++++++++++++++++---------------
> > 1 file changed, 23 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> > index 95b751507ac8..1429767a84bc 100644
> > --- a/fs/overlayfs/params.c
> > +++ b/fs/overlayfs/params.c
> > @@ -164,7 +164,8 @@ static ssize_t ovl_parse_param_split_lowerdirs(char *str)
> >
> > for (s = d = str;; s++, d++) {
> > if (*s == '\\') {
> > - s++;
> > + /* keep esc chars in split lowerdir */
> > + *d++ = *s++;
> > } else if (*s == ':') {
> > bool next_colon = (*(s + 1) == ':');
> >
> > @@ -239,7 +240,7 @@ static void ovl_unescape(char *s)
> > }
> > }
> >
> > -static int ovl_mount_dir(const char *name, struct path *path)
> > +static int ovl_mount_dir(const char *name, struct path *path, bool upper)
> > {
> > int err = -ENOMEM;
> > char *tmp = kstrdup(name, GFP_KERNEL);
> > @@ -248,7 +249,7 @@ static int ovl_mount_dir(const char *name, struct path *path)
> > ovl_unescape(tmp);
> > err = ovl_mount_dir_noesc(tmp, path);
> >
> > - if (!err && path->dentry->d_flags & DCACHE_OP_REAL) {
> > + if (!err && upper && path->dentry->d_flags & DCACHE_OP_REAL) {
> > pr_err("filesystem on '%s' not supported as upperdir\n",
> > tmp);
> > path_put_init(path);
> > @@ -269,7 +270,7 @@ static int ovl_parse_param_upperdir(const char *name, struct fs_context *fc,
> > struct path path;
> > char *dup;
> >
> > - err = ovl_mount_dir(name, &path);
> > + err = ovl_mount_dir(name, &path, true);
> > if (err)
> > return err;
> >
> > @@ -472,7 +473,7 @@ static int ovl_parse_param_lowerdir(const char *name, struct fs_context *fc)
> > l = &ctx->lower[nr];
> > memset(l, 0, sizeof(*l));
> >
> > - err = ovl_mount_dir_noesc(dup_iter, &l->path);
> > + err = ovl_mount_dir(dup_iter, &l->path, false);
> > if (err)
> > goto out_put;
> >
> > @@ -950,16 +951,23 @@ int ovl_show_options(struct seq_file *m, struct dentry *dentry)
> > struct super_block *sb = dentry->d_sb;
> > struct ovl_fs *ofs = OVL_FS(sb);
> > size_t nr, nr_merged_lower = ofs->numlayer - ofs->numdatalayer;
> > - char **lowerdatadirs = &ofs->config.lowerdirs[nr_merged_lower];
> > -
> > - /* lowerdirs[] starts from offset 1 */
> > - seq_printf(m, ",lowerdir=%s", ofs->config.lowerdirs[1]);
> > - /* dump regular lower layers */
> > - for (nr = 2; nr < nr_merged_lower; nr++)
> > - seq_printf(m, ":%s", ofs->config.lowerdirs[nr]);
> > - /* dump data lower layers */
> > - for (nr = 0; nr < ofs->numdatalayer; nr++)
> > - seq_printf(m, "::%s", lowerdatadirs[nr]);
> > +
> > + /*
> > + * lowerdirs[] starts from offset 1, then
> > + * >= 0 regular lower layers prefixed with : and
> > + * >= 0 data-only lower layers prefixed with ::
> > + *
> > + * we need to escase comma and space like seq_show_option() does and
> > + * we also need to escape the colon separator from lowerdir paths.
> > + */
> > + seq_puts(m, ",lowerdir=");
> > + for (nr = 1; nr < ofs->numlayer; nr++) {
> > + if (nr > 1)
> > + seq_putc(m, ':');
> > + if (nr >= nr_merged_lower)
> > + seq_putc(m, ':');
> > + seq_escape(m, ofs->config.lowerdirs[nr], ":,= \t\n\\");
>
> This is too eager. Just need to escape what seq_show_option() would
> escape, which is comma and whitespace. The '=' is not need escaped
> in values only in keys (and that likely never triggers).
Right. I will remove =.
> Colons should have stayed escaped as "\:", so no point in adding another
> level of escape.
Yeh, unless the colon are not escaped because they were set via new api.
In this case, I would like to escape them in mountinfo (e.g. "a:b" => "a\072b").
This way, libmount would be able to parse mountinfo correctly in the future
and learn how to iterate lowerdirs, even before fsinfo().
I am not even sure if fsinfo() is going to be able to iterate lowerdirs, so
escaping colons may be needed there as well.
>
> Yes, this two level escape is pretty confusing, considering that
> commas are escaped on both levels if using the old API. When using
> the new API commas need not be escaped, but can be, since the same
> unescaping is done. Not a serious issue as backslash in filenames is
> basically nonexistent, but an inconsistency nonetheless.
In general, I think we should be conservative and if escaping in mountinfo
doesn't hurt we should not change it.
Applications should consume mountinfo via libmount and libmount already
unescapes the seq_show_option() format.
>
> Following choices exist:
>
> 1) should the redundant escaping be left in mountinfo?
Absolutely yes. I don't think it is redundant at all.
It may be redundant in fsinfo(), when lowerdirs can be iterated?
but mountinfo output is a single monolothic string
even if lowerdirs were set individually by new mount api.
>
> 2) should FSCONFIG_SET_STRING accept escaped commas?
>
Well, it already does.
If you are considering to change that retroactively then my argument
is that IMO userspace needs to be able to pass in \: in lowerdirs
and see it in mountinto (and mount command output) as long as
libmount does not know how to split lowerdirs by itself.
Otherwise, replaying the options from mountinfo into libmount will not
work correctly.
> 3) should unescaped commas on FSCONFIG_SET_STRING (and
> FSCONFIG_SET_PATH) be double escaped in mountinfo?
>
Too much escaping in the sentence. I could not parse it ;)
For example, if "a:b" is set via FSCONFIG_SET_{STRING,PATH}
IMO mount info should output "a\072b", for the aforementioned reasons.
If by "double escaped" you meant "a\:b" or "a\134\072b", then I don't think
this is necessary.
> Currently it's yes, yes, no. I'm fine with leaving things as they
> are, but at least the documentation should be clear on what should
> happen.
OK.
How is that:
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -339,6 +339,18 @@ The specified lower directories will be stacked
beginning from the
rightmost one and going left. In the above example lower1 will be the
top, lower2 the middle and lower3 the bottom layer.
+Note: directory names containing colons can be provided as lower layer by
+escaping the colons with a single backslash. For example:
+
+ mount -t overlay overlay -olowerdir=/a\:lower\:\:dir /merged
+
+Since kernel version v6.5, directory names containing colons can also
+be provided as lower layer using the fsconfig syscall from new mount api:
+
+ fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/a:lower::dir", 0);
+
+In the latter case, colons in lower layer directory names will be escaped
+as an octal characters (\072) when displayed in /proc/self/mountinfo.
The wording of the last sentence above is somewhat legalish -
De facto, since v6.5,y, we will escape ":" to "\072" no matter via which api
we got it and regardless of whether it is also escaped with "\:" or not. but
we only need to commit to this rule for unescaped colons via new mount api.
I will add this documentation to the fix patch and retain escaping of ":".
Thanks,
Amir.
