On Sat, 14 Oct 2023 at 08:24, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Fri, Oct 13, 2023 at 6:36 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> > 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'm not sure that will work. libmount will just remove the \072
escaping from the whole lowerdir= option and we'll be left with the
the unescaped sequence. I think it's better to add the old "\:"
escaping in case it was not added originally.
> I am not even sure if fsinfo() is going to be able to iterate lowerdirs, so
> escaping colons may be needed there as well.
Right.
> > 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.
Yes, it's like a network stack with various levels of encoding and
decoding. The escaping on the mountinfo level is there so that fields
and options can properly be separated. The escaping on the old mount
API had a similar purpose but using a different encoding. Shouldn't
we make this consistent, so that only one level of escaping is done
and at the right level?
> > 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.
No. What I mean by redundant is that a layer named "x,y" is encoded
as e.g. upperdir=x\,y when mounting on the old API. Mountinfo it
will look like "upperdir=x\134\054y". The \134 is totally redundant
and not needed to separate the options, just remains there
historically from the input encoding.
> > 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.
If we are not strict about the encoding/decoding rules then something
can break anyway.
>
> > 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.
Agree double escaping being unncessary, not sure I agree about needing
to escape ':' on the mountinfo level.
>
> > 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.
See my above argument against escaping colons on the mountinfo level.
Thanks,
Miklos
