On Tue, Oct 10, 2023 at 8:33 PM Sebastian Wick
<sebastian.wick@xxxxxxxxxx> wrote:
>
> On Tue, Oct 10, 2023 at 6:54 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> > On Tue, Oct 10, 2023 at 7:13 PM Sebastian Wick
> > <sebastian.wick@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Oct 10, 2023 at 12:00 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > > >
> > > > On Tue, Oct 10, 2023 at 12:06 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, 6 Oct 2023 at 19:21, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > > > > >
> > > > > > On Fri, Oct 6, 2023 at 7:42 PM Ryan Hendrickson
> > > > >
> > > > > > > And there is the escaping that needs to happen for ':' and '\' when
> > > > > > > parsing the path parameters (':' is only special syntax in lowerdir, but
> > > > > > > the escaping logic seems to apply to upperdir and workdir as well, based
> > > > > > > on my testing). Even using the new API, this is handled in the kernel.
> > > > > > > We'd like to know if this escaping can be considered stable as well, and I
> > > > > > > don't think that's a question for the libmount maintainer.
> > > > > >
> > > > > > Agree.
> > > > > > Unlike the comma separated parameters list,
> > > > > > upperdir,workdir,lowerdir are overlayfs specific format.
> > > > > >
> > > > > > ovl_unescape() (for upperdir/workdir) unescapes '\' characters.
> > > > > > as does ovl_parse_param_split_lowerdirs().
> > > > > > Not sure why this was needed for upperdir/workdir, but it It has
> > > > > > been this way for a long time.
> > > > > > I see no reason for it to change in the future.
> > > > >
> > > > > Unescaping upperdir/workdir was the side effect of using a common
> > > > > helper; it wasn't intentional, I think. The problem is that
> > > > > unescaping breaks code that doesn't expect it, and filenames with
> > > > > backslashes (and especially \\ or \: sequences) are very rare, so this
> > > > > won't show up in testing.
> > > > >
> > > > > At this point I'm not sure which is more likely to cause bugs: getting
> > > > > rid of unescaping or leaving it alone.
> > > >
> > > > Considering the fact that the applications that mount overlayfs has
> > > > always had to do the correct escaping, getting rid of escaping can
> > > > only solve issues in new deployments, so I think we should greatly
> > > > favor leaving it alone.
> > >
> > > Any change here is a regression. I'm seriously confused why this is
> > > even debated. You already managed to have a regression and I'm still
> > > of the opinion that this should be fixed because it literally breaks
> > > user space.
> > >
> >
> > You are right. Literally it does.
> > But if prospect users are ok with upgrading libmount and if that
> > solves the problem, I'd rather not have to carry in the kernel
> > baggage code to support old mount API for a very niche use case.
> >
> > > > >
> > > > > One way out of this mess is to create explicit _unesc versions of these options.
> > > > >
> > > >
> > > > I like that solution, with two reservations:
> > > > 1. IMO, new _unesc versions should only be supported from new mount API
> > > > 2. I only want to do that if real users exists - said users are expected
> > > > to send the patch and explain their use case
> > >
> > > This is confusing me a lot. Why would you not want to provide an API
> > > which is clearly, objectively the better API? As user space, when we
> > > can use the new mount API and we could use this, we absolutely would
> > > use this.
> >
> > I am also confused by this reaction.
> > Who said that I do not want to provide the _unenc API?
> >
> > IIUC, you are requesting a new feature that did not exist before,
> > namely, upperdir_unenc, workdir_unenc, lowerdir_unenc options.
> > Did I understand correctly?
> > If that is the case then please send a patch to support
> > those new options in the new mount API only
> > including documentation and tests.
>
> My entire problem is that you break user space. Either fix the
> regression and *continue* fixing regressions instead of hoping that no
> one complains enough and escalates things, or give us another API
> where you can actually make that guarantee. The current way is simply
> not workable.
Ok. No need for hostility, I am just trying to figure out what would
be the best solution for everyone going forward.
How about an API that takes upperdirfd, workdirfd, lowerdirfd,
leaving string parsing and path resolution completely out of the equation?
I think this is something that Christian had once suggested.
I think that will be a good improvement for many other use cases as well.
Will that work for you?
>
> Even if we'd accept this regression (and thus regress our user space
> to not handle any paths any more), the commitment to keeping the API
> stable in this thread has been "we'll try" instead of a "yes,
> absolutely" and that makes me worry as well.
It may not be me, it may be someone else, so there is a limit to my
commitment, but kernel developers usually abide by Linus' no regressions
rules (which do allow some slack).
Anyway, let's focus on what you would like best.
If you prefer to just fix the regression, it is doable.
If you prefer the upperdirfd, workdirfd, lowerdirfd API, I think we can
find a volunteer to write it up.
Thanks,
Amir.
