[Buildroot] [PATCH 2/7] system: add overlayfs option for ro /var

[Norbert Lange]

Am So., 8. Okt. 2023 um 20:37 Uhr schrieb Arnout Vandecappelle <arnout at mind.be>:
>
>   Hi Norbert,
>
> On 15/01/2023 13:52, Norbert Lange wrote:
> > This commit adds an alternative that has the following characteristics:
> >
> > -   Dont depend on anything being available, except the
> >      API File Systems [1].
> >
> >      As /var is meant to be available before normal and even some early
> >      services are running.
> >
> > -   Be a clean drop-in, that can be trivially added / removed.
> >
> > -   Depend on overlayfs being available in the kernel.
> >
> > -   Units are supposed to be reusable for custom solutions.
>
> [snip]
> >
> > Cc: Yann E. MORIN <yann.morin.1998 at free.fr>
> > Cc: Romain Naour <romain.naour at smile.fr>
> > Cc: Jérémy Rosen <jeremy.rosen at smile.fr>
> > Signed-off-by: Norbert Lange <nolange79 at gmail.com>
>
>   In the end, I ended up applying Yann's v4 instead, with a few more
> modifications, as reported in that thread. I did take inspiration from this
> patch - mostly from the commit message.
>
>   For your information, I'll give some feedback on this patch as well.

Happy that there finally is some movement, still its weird that after
a long back-and-forth
a modified version is committed before feedback.

>
> > ---
> >   .../overlayfs/br-bindmount-run at .service       | 26 +++++++++++++++++++
> >   .../overlayfs/br-overlay-prepare at .service     | 26 +++++++++++++++++++
> >   .../overlayfs/overlay.mount.in                | 16 ++++++++++++
> >   .../skeleton-init-systemd.mk                  | 17 ++++++++++++
> >   system/Config.in                              | 13 ++++++++++
> >   5 files changed, 98 insertions(+)
> >   create mode 100644 package/skeleton-init-systemd/overlayfs/br-bindmount-run at .service
> >   create mode 100644 package/skeleton-init-systemd/overlayfs/br-overlay-prepare at .service
> >   create mode 100644 package/skeleton-init-systemd/overlayfs/overlay.mount.in
> >
> > diff --git a/package/skeleton-init-systemd/overlayfs/br-bindmount-run at .service b/package/skeleton-init-systemd/overlayfs/br-bindmount-run at .service
> > new file mode 100644
> > index 0000000000..ce944efd92
> > --- /dev/null
> > +++ b/package/skeleton-init-systemd/overlayfs/br-bindmount-run at .service
> > @@ -0,0 +1,26 @@
> > +[Unit]
> > +Description=Bind-mount rootfs directory (/%I) to /run
> > +Documentation=man:file-hierarchy(7)
> > +ConditionPathIsSymbolicLink=!/%I
> > +DefaultDependencies=no
> > +Conflicts=umount.target
> > +Before=umount.target
> > +
> > +# Needs to run after rootfs is properly mounted
> > +# and before regular mounts might interfere.
> > +After=systemd-remount-fs.service
> > +Before=local-fs-pre.target
> > +
> > +[Service]
> > +Type=oneshot
> > +RemainAfterExit=yes
> > +# dont fail if common dirs already exist
> > +ExecStartPre=/bin/mkdir -m755 -p /run/.br
>
>   There's no reason to make it a hidden directory. Also, there is no reason to
> abbreviate the directory name so much.

should be short imho, to avoid running into path-length problems.
its a technical detail, hidden from users

>
> > +ExecStartPre=/bin/mkdir -m700 -p /run/.br/bnd
>
>   I don't see why it's useful to have a separate base directory for bind mounts.
> When you do a bind mount like this, it's always for a very specific purpose -
> not always for an overlay, but in any case, it makes more sense to mount on a
> mount point that is more related to the sepecific purpose.

Because /run/.br could be for anything else later, and /run/.br/bnd
should mirror
the filesystem root.

>
>   For the overlay case, it's very convenient to have everything that the overlay
> builds on next to each other, i.e. lower, upper, work.

Yeah, there was a reason for not doing that, had to do
with being able to re-use that system of generic mount templates,
ie.  /run/.br/bnd would always mirror a part of the fs root, without
changed hierarchies.

ie. you could either use a single mount / to /run/.br/bnd, or say /etc
to /run/.br/bnd/etc and
/var to /run/.br/bnd/var. It would not effect the overlay units.

>
> > +
> > +ExecStartPre=/bin/mkdir -m700 -p /run/.br/bnd/%I
> > +ExecStart=/bin/mount --make-private -n --bind -o ro /%I /run/.br/bnd/%I
> > +
> > +# lazy unmount, dont block shutdown under any circumstances
> > +ExecStop=/bin/umount -n -l /run/.br/bnd/%I
> > +ExecStopPost=-/bin/rmdir /run/.br/bnd/%I

This got left out aswell:
ExecStopPost=-/bin/rmdir /run/.br/bnd/%I

Removing it, would allow new accesses to the filesystem
thats in progress of being lazily unmounted.

> > diff --git a/package/skeleton-init-systemd/overlayfs/br-overlay-prepare at .service b/package/skeleton-init-systemd/overlayfs/br-overlay-prepare at .service
> > new file mode 100644
> > index 0000000000..86b32900dd
> > --- /dev/null
> > +++ b/package/skeleton-init-systemd/overlayfs/br-overlay-prepare at .service
> > @@ -0,0 +1,26 @@
> > +[Unit]
> > +Description=Mount and prepare tmpfs for overlay (/%I)
> > +Documentation=man:file-hierarchy(7)
> > +ConditionPathIsSymbolicLink=!/%I
> > +DefaultDependencies=no
> > +Conflicts=umount.target
> > +Before=local-fs.target umount.target
> > +# prepare for systemd mount units aswell
> > +RequiresMountsFor=/run/.br/ovl/%I
> > +
> > +[Service]
> > +Environment="OVERLAY_DIR=/run/.br/ovl/%I"
> > +Type=oneshot
> > +RemainAfterExit=yes
> > +# dont fail if common dirs already exist
> > +ExecStartPre=/bin/mkdir -m755 -p /run/.br
> > +ExecStartPre=/bin/mkdir -m700 -p /run/.br/ovl
>
>   I'm not sure if there's a good reason to make this mode 700 instead of the
> default 755. If there is a good reason, I'd accept a patch that modifies the
> prepare-var-overlay.service that I ended up applying.

No one should access this directories directly, its being defensive

>
> > +ExecStartPre=/bin/mkdir -m700 -p ${OVERLAY_DIR}
> > +# Create an override and edit this line for customization
> > +ExecStart=/bin/mount --make-private -n -t tmpfs tmpfs_br_ovl ${OVERLAY_DIR}
>
>   There is in fact no reason to mount anything here - /run is already a tmpfs
> with all the required properties. We can just reuse it.

Yes, problem is that the /run mount is limited in size, not configurable
(parameters are in systemd's sources) and
filling /var with stuff should never exhaust one of the most critical
filesystems.

>
> > +ExecStartPost=/bin/mkdir -p ${OVERLAY_DIR}/up ${OVERLAY_DIR}/wd
> > +
> > +# lazy unmount, dont block shutdown under any circumstances
> > +ExecStop=/bin/umount -n -l ${OVERLAY_DIR}
> > +ExecStopPost=/bin/rmdir ${OVERLAY_DIR}
> > diff --git a/package/skeleton-init-systemd/overlayfs/overlay.mount.in b/package/skeleton-init-systemd/overlayfs/overlay.mount.in
> > new file mode 100644
> > index 0000000000..84f4d9ee47
> > --- /dev/null
> > +++ b/package/skeleton-init-systemd/overlayfs/overlay.mount.in
> > @@ -0,0 +1,16 @@
> > +[Unit]
> > +Description=Variable storage (/@PATH@)
> > +Documentation=man:file-hierarchy(7)
> > +ConditionPathIsSymbolicLink=!/@PATH@
>
>   There's too much @PATH@ here. I actually think we can completely avoid it by
> using %p or %P as appropriate everywhere.
>
>   I think that that way, we can indeed reuse both the overlay.mount.in and
> prepare-var-overlay at .service for other overlays. I unfortunately didn't have
> time to implement and test that, so instead I committed it without the %p/%P and
> without a template service.
>
>   If you end up implementing it as a template, please make sure to also include
> a runtime test for it, e.g. with /etc as the overlay. Or even better something
> deeper - for the test it could be something silly, like /usr/share.

Yeah, gonna rebase my stuff and pickup where I stopped long ago,
gonna take a while.

>
> > +
> > +After=br-bindmount-run@@PATH at .service
> > +Requires=br-bindmount-run@@PATH at .service
> > +
> > +After=br-overlay-prepare@@PATH at .service
> > +BindsTo=br-overlay-prepare@@PATH at .service
> > +
> > +[Mount]
> > +Type=overlay
> > +What=br_ovl_ at PATH@
> > +Where=/@PATH@
> > +Options=redirect_dir=on,index=on,xino=on,lowerdir=/run/.br/bnd/@PATH@,upperdir=/run/.br/ovl/@PATH@/up,workdir=/run/.br/ovl/@PATH@/wd
> > diff --git a/package/skeleton-init-systemd/skeleton-init-systemd.mk b/package/skeleton-init-systemd/skeleton-init-systemd.mk
> > index fb15552f99..ad529cddf6 100644
> > --- a/package/skeleton-init-systemd/skeleton-init-systemd.mk
> > +++ b/package/skeleton-init-systemd/skeleton-init-systemd.mk
> > @@ -57,6 +57,23 @@ define SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
> >   endef
> >   SKELETON_INIT_SYSTEMD_ROOTFS_PRE_CMD_HOOKS += SKELETON_INIT_SYSTEMD_PRE_ROOTFS_VAR
> >   endif  # BR2_INIT_SYSTEMD_VAR_FACTORY
> > +
> > +ifeq ($(BR2_INIT_SYSTEMD_VAR_OVERLAYFS),y)
> > +define SKELETON_INIT_SYSTEMD_LINUX_CONFIG_FIXUPS
> > +     $(call KCONFIG_ENABLE_OPT,CONFIG_OVERLAY_FS)
> > +endef
> > +define SKELETON_INIT_SYSTEMD_POST_INSTALL_VAR_OVERLAYFS
> > +     sed 's, at PATH@,var,g' $(SKELETON_INIT_SYSTEMD_PKGDIR)/overlayfs/overlay.mount.in >$(@D)/var.mount
> > +     $(INSTALL) -D -m 0644 $(@D)/var.mount $(TARGET_DIR)/usr/lib/systemd/system/var.mount
> > +     $(INSTALL) -D -m 0644 $(SKELETON_INIT_SYSTEMD_PKGDIR)/overlayfs/br-bindmount-run at .service \
> > +             $(TARGET_DIR)/usr/lib/systemd/system/br-bindmount-run at .service
> > +     $(INSTALL) -D -m 0644 $(SKELETON_INIT_SYSTEMD_PKGDIR)/overlayfs/br-overlay-prepare at .service \
> > +             $(TARGET_DIR)/usr/lib/systemd/system/br-overlay-prepare at .service
>
>   If it's templates, they can actually be installed independently of the
> BR2_INIT_SYSTEMD_VAR_OVERLAYFS option - maybe you want to use a factory for /var
> but an overlay for /etc...

Sure, but not sure if it should be installed unconditionally. Easy to
just copy those files into a
rootfs-overlay.

>
>
>   I've marked this patch as Superseded (because I took Yann's v4). The rest of
> the series I'll look at later.

Ok

>
>   Regards,
>   Arnout
>

Regards,
Norbert