Re: [PATCH v2] fixfiles: Unmount temporary bind mounts on SIGINT

["Christian Göttsche" <cgzones@googlemail.com>]

On Fri, 18 Nov 2022 at 11:03, Petr Lautrbach <plautrba@redhat.com> wrote:
>
> Petr Lautrbach <plautrba@redhat.com> writes:
>
> > Petr Lautrbach <plautrba@redhat.com> writes:
> >
> >> `fixfiles -M relabel` temporary bind mounts file systems before
> >> relabeling, but it left the / directory mounted in /tmp/tmp.XXXX when a
> >> user hit CTRL-C. It means that if the user run `fixfiles -M relabel`
> >> again and answered Y to clean out /tmp directory, it would remove all
> >> data from mounted fs.
> >>
> >> This patch changes the location where `fixfiles` mounts fs to /run, uses
> >> private mount namespace via unshare and adds a handler for exit signals
> >> which tries to umount fs mounted by `fixfiles`.
> >>
> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355
> >>
> >> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> >> ---
> >
> > Actually, it's v5:
> >
> > v2:
> >
> > - set trap on EXIT instead of SIGINT
> >
> > v3:
> >
> > - use /run instead of /tmp for mountpoints
> >
> > v4:
> >
> > - use mount namespace as suggested by Christian Göttsche <cgzones@googlemail.com>
> >
> > v5
> >
> > - fixed issues reported by Christian Göttsche <cgzones@googlemail.com>
> >
>
> Any objections?

Works fine for me.
Tested-by: Christian Göttsche <cgzones@googlemail.com>

>
>
> >>
> >>
> >>  policycoreutils/scripts/fixfiles | 36 +++++++++++++++++++++++++-------
> >>  1 file changed, 28 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles
> >> index c72ca0eb9d61..166af6f360a2 100755
> >> --- a/policycoreutils/scripts/fixfiles
> >> +++ b/policycoreutils/scripts/fixfiles
> >> @@ -207,6 +207,25 @@ rpm -q --qf '[%{FILESTATES} %{FILENAMES}\n]' "$1" | grep '^0 ' | cut -f2- -d ' '
> >>  [ ${PIPESTATUS[0]} != 0 ] && echo "$1 not found" >/dev/stderr
> >>  }
> >>
> >> +# unmount tmp bind mount before exit
> >> +umount_TMP_MOUNT() {
> >> +    if [ -n "$TMP_MOUNT" ]; then
> >> +         umount "${TMP_MOUNT}${m}" || exit 130
> >> +         rm -rf "${TMP_MOUNT}" || echo "Error cleaning up."
> >> +    fi
> >> +    exit 130
> >> +}
> >> +
> >> +fix_labels_on_mountpoint() {
> >> +    test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1
> >> +    mkdir -p "${TMP_MOUNT}${m}" || exit 1
> >> +    mount --bind "${m}" "${TMP_MOUNT}${m}" || exit 1
> >> +    ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} ${THREADS} $* -q ${FC} -r "${TMP_MOUNT}" "${TMP_MOUNT}${m}"
> >> +    umount "${TMP_MOUNT}${m}" || exit 1
> >> +    rm -rf "${TMP_MOUNT}" || echo "Error cleaning up."
> >> +}
> >> +export -f fix_labels_on_mountpoint
> >> +
> >>  #
> >>  # restore
> >>  # if called with -n will only check file context
> >> @@ -252,14 +271,15 @@ case "$RESTORE_MODE" in
> >>              # we bind mount so we can fix the labels of files that have already been
> >>              # mounted over
> >>              for m in `echo $FILESYSTEMSRW`; do
> >> -                TMP_MOUNT="$(mktemp -d)"
> >> -                test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1
> >> -
> >> -                mkdir -p "${TMP_MOUNT}${m}" || exit 1
> >> -                mount --bind "${m}" "${TMP_MOUNT}${m}" || exit 1
> >> -                ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} ${THREADS} $* -q ${FC} -r "${TMP_MOUNT}" "${TMP_MOUNT}${m}"
> >> -                umount "${TMP_MOUNT}${m}" || exit 1
> >> -                rm -rf "${TMP_MOUNT}" || echo "Error cleaning up."
> >> +                TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)"
> >> +                export SETFILES VERBOSE EXCLUDEDIRS FORCEFLAG THREADS FC TMP_MOUNT m
> >> +                if type unshare &> /dev/null; then
> >> +                    unshare -m bash -c "fix_labels_on_mountpoint $*" || exit $?
> >> +                else
> >> +                    trap umount_TMP_MOUNT EXIT
> >> +                    fix_labels_on_mountpoint $*
> >> +                    trap EXIT
> >> +                fi
> >>              done;
> >>          fi
> >>      else
> >> --
> >> 2.37.3
>