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

[Petr Lautrbach <plautrba@redhat.com>]

Ondrej Mosnacek <omosnace@redhat.com> writes:

> On Thu, Sep 15, 2022 at 6:29 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>> Ondrej Mosnacek <omosnace@redhat.com> writes:
>>
>> > On Thu, Sep 15, 2022 at 2:45 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>> >> `fixfiles -M relabel` temporary bind mounts filestems before relabeling
>> >> but it leaves a directory mounted in /tmp/tmp.XXXX when a user hits
>> >> 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.
>> >>
>> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355
>> >>
>> >> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>> >> ---
>> >>  policycoreutils/scripts/fixfiles | 11 +++++++++++
>> >>  1 file changed, 11 insertions(+)
>> >>
>> >> diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles
>> >> index c72ca0eb9d61..6811921970f2 100755
>> >> --- a/policycoreutils/scripts/fixfiles
>> >> +++ b/policycoreutils/scripts/fixfiles
>> >> @@ -207,6 +207,15 @@ 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
>> >> +}
>> >> +
>> >>  #
>> >>  # restore
>> >>  # if called with -n will only check file context
>> >> @@ -251,6 +260,7 @@ case "$RESTORE_MODE" in
>> >>             else
>> >>                 # we bind mount so we can fix the labels of files that have already been
>> >>                 # mounted over
>> >> +               trap umount_TMP_MOUNT SIGINT
>> >>                 for m in `echo $FILESYSTEMSRW`; do
>> >>                     TMP_MOUNT="$(mktemp -d)"
>> >>                     test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1
>> >> @@ -261,6 +271,7 @@ case "$RESTORE_MODE" in
>> >>                     umount "${TMP_MOUNT}${m}" || exit 1
>> >>                     rm -rf "${TMP_MOUNT}" || echo "Error cleaning up."
>> >>                 done;
>> >> +               trap SIGINT
>> >>             fi
>> >>         else
>> >>             echo >&2 "fixfiles: No suitable file systems found"
>> >> --
>> >> 2.37.3
>> >>
>> >
>> > And what if the fixfiles process is terminated via SIGTERM or even
>> > SIGKILL?
>>
>> Good point.
>>
>> > Or a power failure occurs at the wrong time?
>>
>> After power failure and reboot there's no bind mountpoints from fixfiles
>> left.
>
> Indeed, sorry for the brainfart.
>
>> > What if some
>> > other process leaves behind a bind mount / other mount in /tmp?
>>
>> I don't know. But it's up to users to decide. If they are not sure, they
>> should answer 'No' to /tmp clean up.
>
> I disagree. We shouldn't offer the user an easy way of shooting
> themselves in the foot for no good reason.
>
>>
>>
>> > My suggestion would be to:
>> > 1) Change the trap from SIGINT to EXIT. That will cover both SIGTERM and SIGINT.
>>
>> I'll send updated patch with this.
>>
>> > 2) Additionally modify fullrelabel() to not traverse across mounts
>> > when doing the removing (+ possibly exit with an error whenever a
>> > mountpoint is encountered - OR - try to unmount the mounts instead of
>> > removing their contents).
>>
>> Given that I don't know what was the original motivation for the cleaing
>> code and users are asked whether they want to clean up /tmp and they are
>> warned that they would need to reboot after this operation, I'm not sure
>> how I would defend the change.
>
> The reboot will not fix the deletion of all files caused by a leftover
> mount. If you don't want to harden the removal, then at least please
> add a big warning to the prompt that the operation will also attempt
> to delete files inside mounts mounted under /tmp. (But that's pretty
> silly, when you could instead just add -xdev and avoid the problem
> altogether...)

There's prompt which says:

"this command can remove all files in /tmp"

so users are already warned.


> I would say the motivation is simply to force the re-creation of all
> temporary files in /tmp so that any changes in type transition rules
> are applied. It is illogical to remove also contents of mounts, as
> these will likely use different labeling rules (depending on the
> superblock or in case of a bind mount the original path). There is
> just no way that deleting across mounts would be anything but a
> horrible idea in this case.
>

My problem is that when users run fixfiles twice, it could remove files
from /

Your problem is that when users use /tmp as a place for mountpoints,
running fixfiles (even once) could remove data from their mount points
inside /tmp.


They are just different issues.


Petr