On Fri, 9 Jun 2023 at 10:03, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On Fri, Jun 09, 2023 at 10:38:15AM +0300, Amir Goldstein wrote:
> > On Fri, Jun 9, 2023 at 10:28 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Jun 08, 2023 at 09:29:39PM +0300, Amir Goldstein wrote:
> > > > On Thu, Jun 8, 2023 at 7:07 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> > > > >
> > > > > We recently ported util-linux to the new mount api. Now the mount(8)
> > > > > tool will by default use the new mount api. While trying hard to fall
> > > > > back to the old mount api gracefully there are still cases where we run
> > > > > into issues that are difficult to handle nicely.
> > > > >
> > > > > Now with mount(8) and libmount supporting the new mount api I expect an
> > > > > increase in the number of bug reports and issues we're going to see with
> > > > > filesystems that don't yet support the new mount api. So it's time we
> > > > > rectify this.
> > > > >
> > > > > For overlayfs specifically we ran into issues where mount(8) passed
> > > > > multiple lower layers as one big string through fsconfig(). But the
> > > > > fsconfig() FSCONFIG_SET_STRING option is limited to 256 bytes in
> > > > > strndup_user(). While this would be fixable by extending the fsconfig()
> > > > > buffer I'd rather encourage users to append layers via multiple
> > > > > fsconfig() calls as the interface allows nicely for this. This has also
> > > > > been requested as a feature before.
> > > > >
> > > > > With this port to the new mount api the following will be possible:
> > > > >
> > > > > fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", "/lower1", 0);
> > > > >
> > > > > /* set upper layer */
> > > > > fsconfig(fs_fd, FSCONFIG_SET_STRING, "upperdir", "/upper", 0);
> > > > >
> > > > > /* append "/lower2", "/lower3", and "/lower4" */
> > > > > fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", ":/lower2:/lower3:/lower4", 0);
> > > > >
> > > > > /* turn index feature on */
> > > > > fsconfig(fs_fd, FSCONFIG_SET_STRING, "index", "on", 0);
> > > > >
> > > > > /* append "/lower5" */
> > > > > fsconfig(fs_fd, FSCONFIG_SET_STRING, "lowerdir", ":/lower5", 0);
> > > > >
> > > > > Specifying ':' would have been rejected so this isn't a regression. And
> > > > > we can't simply use "lowerdir=/lower" to append on top of existing
> > > > > layers as "lowerdir=/lower,lowerdir=/other-lower" would make
> > > > > "/other-lower" the only lower layer so we'd break uapi if we changed
> > > > > this. So the ':' prefix seems a good compromise.
> > > > >
> > > > > Users can choose to specify multiple layers at once or individual
> > > > > layers. A layer is appended if it starts with ":". This requires that
> > > > > the user has already added at least one layer before. If lowerdir is
> > > > > specified again without a leading ":" then all previous layers are
> > > > > dropped and replaced with the new layers. If lowerdir is specified and
> > > > > empty than all layers are simply dropped.
> > > > >
> > > > > An additional change is that overlayfs will now parse and resolve layers
> > > > > right when they are specified in fsconfig() instead of deferring until
> > > > > super block creation. This allows users to receive early errors.
> > > > >
> > > > > It also allows users to actually use up to 500 layers something which
> > > > > was theoretically possible but ended up not working due to the mount
> > > > > option string passed via mount(2) being too large.
> > > > >
> > > > > This also allows a more privileged process to set config options for a
> > > > > lesser privileged process as the creds for fsconfig() and the creds for
> > > > > fsopen() can differ. We could restrict that they match by enforcing that
> > > > > the creds of fsopen() and fsconfig() match but I don't see why that
> > > > > needs to be the case and allows for a good delegation mechanism.
> > > > >
> > > > > Plus, in the future it means we're able to extend overlayfs mount
> > > > > options and allow users to specify layers via file descriptors instead
> > > > > of paths:
> > > > >
> > > > > fsconfig(FSCONFIG_SET_PATH{_EMPTY}, "lowerdir", "lower1", dirfd);
> > > > >
> > > > > /* append */
> > > > > fsconfig(FSCONFIG_SET_PATH{_EMPTY}, "lowerdir", "lower2", dirfd);
> > > > >
> > > > > /* append */
> > > > > fsconfig(FSCONFIG_SET_PATH{_EMPTY}, "lowerdir", "lower3", dirfd);
> > > > >
> > > > > /* clear all layers specified until now */
> > > > > fsconfig(FSCONFIG_SET_STRING, "lowerdir", NULL, 0);
> > > > >
> > > > > This would be especially nice if users create an overlayfs mount on top
> > > > > of idmapped layers or just in general private mounts created via
> > > > > open_tree(OPEN_TREE_CLONE). Those mounts would then never have to appear
> > > > > anywhere in the filesystem. But for now just do the minimal thing.
> > > > >
> > > > > We should probably aim to move more validation into ovl_fs_parse_param()
> > > > > so users get errors before fsconfig(FSCONFIG_CMD_CREATE). But that can
> > > > > be done in additional patches later.
> > > > >
> > > > > Link: https://github.com/util-linux/util-linux/issues/2287 [1]
> > > > > Link: https://github.com/util-linux/util-linux/issues/1992 [2]
> > > > > Link: https://bugs.archlinux.org/task/78702 [3]
> > > > > Link: https://lore.kernel.org/linux-unionfs/20230530-klagen-zudem-32c0908c2108@brauner [4]
> > > > > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> > > > > ---
> > > > >
> > > > > ---
> > > > >
> > > > > I'm starting to get the feeling that I stared enough at this and I would
> > > > > need a fresh set of eyes to review it for any bugs. Plus, Amir seems to
> > > > > have conflicting series and I would have to rebase anyway so no point in
> > > > > delaying this any further.
> > > > > ---
> > > > > fs/overlayfs/super.c | 896 ++++++++++++++++++++++++++++++++-------------------
> > > > > 1 file changed, 568 insertions(+), 328 deletions(-)
> > > > >
> > > >
> > > > Very big patch - Not so easy to review.
> > > > It feels like a large refactoring mixed with the api change.
> > > > Can it easily be split to just the refactoring patch
> > > > and the api change patch? or any other split that will be
> > > > easier to review.
> > >
> > > I don't really think so because you can't do a piecemeal conversion
> > > unfortunately. But if you have concrete ideas I'm happy to hear them.
> > >
> >
> > To me it looks like besides using new api you changed the order
> > of config parsing to:
> > - fill ovl_config and sanitize path arguments
> > (replacing string with path in case of upper/workdir)
>
> Afaict this only makes sense if you cane have a sensible split between
> option parsing and superblock creation. While the new mount api does
> have that the old one doesn't. So ovl_fill_super() does option parsing,
> verification, and superblock creation.
>
> So the only thing we could do is something where we move
> ovl_mount_dir_noesc() out of ovl_lower_dir() and ovl_mount_dir() out of
> ovl_get_workdir() and ovl_get_upper(). And resolve all layers first.
>
> But it would still need to remain centralized in ovl_fill_super() and
> then it'd be an equal amount of churn when we implement proper option
> parsing for the new mount api in ovl_parse_param() as the implementation
> of the helpers used in there doesn't make sense before the switch.
>
> So I honestly thing this might end up being churn for churn. But I'll do
> it if you insist.
>
> But it'd be good to first get an indication whether this is even
> acceptable overall and whether I should do rebase and resend asap
> for v6.5.
Looks good to me overall. The only added complexity I see is parsing
the lowerdir option, so it might make sense to split it like this:
1) convert to new API, don't touch lowerdir parsing (technically this
could be a bisect confusing regression, but I'm not really worried)
2) add the new split lowerdir handling
Also would it make sense to move parsing to a separate source file?
Does the split option handling make sense for other fs? Is it
something that could be standardized?
Thanks,
Miklos
