On Sat, Oct 14, 2023 at 10:10 AM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> 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.
>
It is better to add "\:" if we want to be super nice and abide by the unspoken
rule that the mount options parsed by libmount can be fed into mount(2) syscall
or back into libmount to get the same overlayfs config.
But I don't think it is a must because this rule is for overlayfs created with
mount(2) syscall or libmount, which knows how to split commas and use new
mount api, but it doesn't know how to split lowerdir colons.
If in the future, libmount will learn how to split lowerdir colons and use new
mount api to configure them one by one, then in the same future, libmount
will be able to distinguish between single escaped colons (\072) and
unescaped colons, and we do not need to add "\:" for making this possible.
> > 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.
>
It is not only historical.
In the present, even with most recent libmount, the mount(8) tool
e.g. mount -t overlay, displays the options (... upperdir=x\,y) and
then the user can replay those options.
It is true that libmount could have displayed "upperdir=x\054y" as
(... upperdir="x,y"), which is the escaping for commas that libmount
supports, but:
1. libmount does not do that
2. those options could be fed back into libmount but not directly to
mount(2)
> > > 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.
>
See counter arguments.
My goal is to get as much of the parsing/escaping logic done by libmount
going forward, so I'd rather not add new escaping logic to new use cases
if we can avoid it.
At the moment, I would like to get some minimal fix to the 6.5 regression
merged while 6.5.y is still getting fixes.
If we cannot reach consensus within this short timeframe, then I think
that the smallest and least controversial fix is to escape colons once
as \072 as the current patch does.
We can always remove this escaping later if we find that it is not needed
no harm done. If you like, I can remove the commitment to mountinfo
format from documentation for the fix patch?
Being extra nice and displaying "a\,b" and "a\:b" for new users that
used new mount api explicitly to pass "a,b" and "a:b" is possible, but
it is not a 6.5 regression fix, so we can take the time to decide if we
want to do that or not.
OK?
Thanks,
Amir.
