* [PATCH] fixfiles: Unmount temporary bind mounts on SIGINT @ 2022-09-15 12:44 Petr Lautrbach 2022-09-15 13:18 ` Ondrej Mosnacek 0 siblings, 1 reply; 15+ messages in thread From: Petr Lautrbach @ 2022-09-15 12:44 UTC (permalink / raw) To: selinux; +Cc: Petr Lautrbach `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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] fixfiles: Unmount temporary bind mounts on SIGINT 2022-09-15 12:44 [PATCH] fixfiles: Unmount temporary bind mounts on SIGINT Petr Lautrbach @ 2022-09-15 13:18 ` Ondrej Mosnacek 2022-09-15 16:29 ` Petr Lautrbach 2022-09-15 16:37 ` [PATCH v2] " Petr Lautrbach 0 siblings, 2 replies; 15+ messages in thread From: Ondrej Mosnacek @ 2022-09-15 13:18 UTC (permalink / raw) To: Petr Lautrbach; +Cc: SElinux list 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? Or a power failure occurs at the wrong time? What if some other process leaves behind a bind mount / other mount in /tmp? My suggestion would be to: 1) Change the trap from SIGINT to EXIT. That will cover both SIGTERM and SIGINT. 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). -- Ondrej Mosnacek Senior Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] fixfiles: Unmount temporary bind mounts on SIGINT 2022-09-15 13:18 ` Ondrej Mosnacek @ 2022-09-15 16:29 ` Petr Lautrbach 2022-09-16 12:23 ` Ondrej Mosnacek 2022-09-15 16:37 ` [PATCH v2] " Petr Lautrbach 1 sibling, 1 reply; 15+ messages in thread From: Petr Lautrbach @ 2022-09-15 16:29 UTC (permalink / raw) To: SElinux list, Ondrej Mosnacek 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. > 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. > 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. Thanks, Petr > -- > Ondrej Mosnacek > Senior Software Engineer, Linux Security - SELinux kernel > Red Hat, Inc. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] fixfiles: Unmount temporary bind mounts on SIGINT 2022-09-15 16:29 ` Petr Lautrbach @ 2022-09-16 12:23 ` Ondrej Mosnacek 2022-09-16 13:45 ` Petr Lautrbach 0 siblings, 1 reply; 15+ messages in thread From: Ondrej Mosnacek @ 2022-09-16 12:23 UTC (permalink / raw) To: Petr Lautrbach; +Cc: SElinux list 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...) 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. -- Ondrej Mosnacek Senior Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] fixfiles: Unmount temporary bind mounts on SIGINT 2022-09-16 12:23 ` Ondrej Mosnacek @ 2022-09-16 13:45 ` Petr Lautrbach 0 siblings, 0 replies; 15+ messages in thread From: Petr Lautrbach @ 2022-09-16 13:45 UTC (permalink / raw) To: SElinux list, Ondrej Mosnacek 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] fixfiles: Unmount temporary bind mounts on SIGINT 2022-09-15 13:18 ` Ondrej Mosnacek 2022-09-15 16:29 ` Petr Lautrbach @ 2022-09-15 16:37 ` Petr Lautrbach 2022-09-16 14:13 ` [PATCH v3] " Petr Lautrbach 1 sibling, 1 reply; 15+ messages in thread From: Petr Lautrbach @ 2022-09-15 16:37 UTC (permalink / raw) To: selinux; +Cc: Petr Lautrbach `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> --- v2: - set trap on EXIT instead of SIGINT policycoreutils/scripts/fixfiles | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles index c72ca0eb9d61..d763fa83eefd 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 EXIT 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 EXIT fi else echo >&2 "fixfiles: No suitable file systems found" -- 2.37.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3] fixfiles: Unmount temporary bind mounts on SIGINT 2022-09-15 16:37 ` [PATCH v2] " Petr Lautrbach @ 2022-09-16 14:13 ` Petr Lautrbach 2022-09-16 15:36 ` Christian Göttsche 0 siblings, 1 reply; 15+ messages in thread From: Petr Lautrbach @ 2022-09-16 14:13 UTC (permalink / raw) To: selinux; +Cc: Petr Lautrbach `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. This patch changes the location where `fixfiles` mounts fs to /run 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> --- v2: - set trap on EXIT instead of SIGINT v3: - use /run instead of /tmp for mountpoints policycoreutils/scripts/fixfiles | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles index c72ca0eb9d61..acf5f0996c19 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,8 +260,9 @@ 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 EXIT for m in `echo $FILESYSTEMSRW`; do - TMP_MOUNT="$(mktemp -d)" + TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)" test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 mkdir -p "${TMP_MOUNT}${m}" || 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 EXIT fi else echo >&2 "fixfiles: No suitable file systems found" -- 2.37.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3] fixfiles: Unmount temporary bind mounts on SIGINT 2022-09-16 14:13 ` [PATCH v3] " Petr Lautrbach @ 2022-09-16 15:36 ` Christian Göttsche 2022-09-19 8:39 ` Ondrej Mosnacek 2022-10-07 13:46 ` [PATCH v4] " Petr Lautrbach 0 siblings, 2 replies; 15+ messages in thread From: Christian Göttsche @ 2022-09-16 15:36 UTC (permalink / raw) To: Petr Lautrbach; +Cc: selinux On Fri, 16 Sept 2022 at 16:14, 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. > > This patch changes the location where `fixfiles` mounts fs to /run and > adds a handler for exit signals which tries to umount fs mounted by > `fixfiles`. What about additionally using mount namespaces, if available: @@ -256,10 +256,16 @@ 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." + if ! unshare --mount /bin/sh -c "mount --bind \"${m}\" \"${TMP_MOUNT}${m}\" || exit 1 && ${SETFILES} ${VERBOSE} ${EXCLUDEDIRS} ${FORCEFLAG} ${THREADS} $* -q ${FC} -r \"${TMP_MOUNT}\" \"${TMP_MOUNT}${m}\" || true"; then + echo "Creating new mount namespace failed, operating in root namespace" + 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 + fi + if [ "${m}" != '/' ]; then + rmdir "${TMP_MOUNT}${m}" || echo "Error cleaning up ${TMP_MOUNT}${m}." + fi + rmdir "${TMP_MOUNT}" || echo "Error cleaning up ${TMP_MOUNT}." done; fi else ? > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355 > > Signed-off-by: Petr Lautrbach <plautrba@redhat.com> > --- > v2: > > - set trap on EXIT instead of SIGINT > > v3: > > - use /run instead of /tmp for mountpoints > > > policycoreutils/scripts/fixfiles | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles > index c72ca0eb9d61..acf5f0996c19 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,8 +260,9 @@ 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 EXIT > for m in `echo $FILESYSTEMSRW`; do > - TMP_MOUNT="$(mktemp -d)" > + TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)" > test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 > > mkdir -p "${TMP_MOUNT}${m}" || 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 EXIT > fi > else > echo >&2 "fixfiles: No suitable file systems found" > -- > 2.37.3 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] fixfiles: Unmount temporary bind mounts on SIGINT 2022-09-16 15:36 ` Christian Göttsche @ 2022-09-19 8:39 ` Ondrej Mosnacek 2022-09-19 11:17 ` Christian Göttsche 2022-10-07 13:46 ` [PATCH v4] " Petr Lautrbach 1 sibling, 1 reply; 15+ messages in thread From: Ondrej Mosnacek @ 2022-09-19 8:39 UTC (permalink / raw) To: Christian Göttsche; +Cc: Petr Lautrbach, SElinux list (Resending without HTML...) On Fri, Sep 16, 2022 at 5:37 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > On Fri, 16 Sept 2022 at 16:14, 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. > > > > This patch changes the location where `fixfiles` mounts fs to /run and > > adds a handler for exit signals which tries to umount fs mounted by > > `fixfiles`. > > What about additionally using mount namespaces, if available: What benefit would it bring? (Sorry, I'm not very familiar with mount namespaces.) > > @@ -256,10 +256,16 @@ > 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." > + if ! unshare --mount /bin/sh -c "mount --bind > \"${m}\" \"${TMP_MOUNT}${m}\" || exit 1 && ${SETFILES} ${VERBOSE} > ${EXCLUDEDIRS} ${FORCEFLAG} ${THREADS} $* -q ${FC} -r \"${TMP_MOUNT}\" > \"${TMP_MOUNT}${m}\" || true"; then > + echo "Creating new mount namespace failed, > operating in root namespace" > + 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 > + fi > + if [ "${m}" != '/' ]; then > + rmdir "${TMP_MOUNT}${m}" || echo "Error > cleaning up ${TMP_MOUNT}${m}." > + fi > + rmdir "${TMP_MOUNT}" || echo "Error cleaning > up ${TMP_MOUNT}." > done; > fi > else > > ? > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355 > > > > Signed-off-by: Petr Lautrbach <plautrba@redhat.com> > > --- > > v2: > > > > - set trap on EXIT instead of SIGINT > > > > v3: > > > > - use /run instead of /tmp for mountpoints > > > > > > policycoreutils/scripts/fixfiles | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles > > index c72ca0eb9d61..acf5f0996c19 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,8 +260,9 @@ 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 EXIT > > for m in `echo $FILESYSTEMSRW`; do > > - TMP_MOUNT="$(mktemp -d)" > > + TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)" > > test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 > > > > mkdir -p "${TMP_MOUNT}${m}" || 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 EXIT > > fi > > else > > echo >&2 "fixfiles: No suitable file systems found" > > -- > > 2.37.3 > > > -- Ondrej Mosnacek Senior Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] fixfiles: Unmount temporary bind mounts on SIGINT 2022-09-19 8:39 ` Ondrej Mosnacek @ 2022-09-19 11:17 ` Christian Göttsche 0 siblings, 0 replies; 15+ messages in thread From: Christian Göttsche @ 2022-09-19 11:17 UTC (permalink / raw) To: Ondrej Mosnacek; +Cc: Petr Lautrbach, SElinux list On Mon, 19 Sept 2022 at 10:39, Ondrej Mosnacek <omosnace@redhat.com> wrote: > > (Resending without HTML...) > > On Fri, Sep 16, 2022 at 5:37 PM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > On Fri, 16 Sept 2022 at 16:14, 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. > > > > > > This patch changes the location where `fixfiles` mounts fs to /run and > > > adds a handler for exit signals which tries to umount fs mounted by > > > `fixfiles`. > > > > What about additionally using mount namespaces, if available: > > What benefit would it bring? (Sorry, I'm not very familiar with mount > namespaces.) The bind mounts will only be visible to the process and its children used in the unshare command (and processes explicitly joining that mount namespace) and not to other (parallel or subsequent) instances of fixfiles. > > > > > @@ -256,10 +256,16 @@ > > 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." > > + if ! unshare --mount /bin/sh -c "mount --bind > > \"${m}\" \"${TMP_MOUNT}${m}\" || exit 1 && ${SETFILES} ${VERBOSE} > > ${EXCLUDEDIRS} ${FORCEFLAG} ${THREADS} $* -q ${FC} -r \"${TMP_MOUNT}\" > > \"${TMP_MOUNT}${m}\" || true"; then > > + echo "Creating new mount namespace failed, > > operating in root namespace" > > + 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 > > + fi > > + if [ "${m}" != '/' ]; then > > + rmdir "${TMP_MOUNT}${m}" || echo "Error > > cleaning up ${TMP_MOUNT}${m}." > > + fi > > + rmdir "${TMP_MOUNT}" || echo "Error cleaning > > up ${TMP_MOUNT}." > > done; > > fi > > else > > > > ? > > > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2125355 > > > > > > Signed-off-by: Petr Lautrbach <plautrba@redhat.com> > > > --- > > > v2: > > > > > > - set trap on EXIT instead of SIGINT > > > > > > v3: > > > > > > - use /run instead of /tmp for mountpoints > > > > > > > > > policycoreutils/scripts/fixfiles | 13 ++++++++++++- > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles > > > index c72ca0eb9d61..acf5f0996c19 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,8 +260,9 @@ 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 EXIT > > > for m in `echo $FILESYSTEMSRW`; do > > > - TMP_MOUNT="$(mktemp -d)" > > > + TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)" > > > test -z ${TMP_MOUNT+x} && echo "Unable to find temporary directory!" && exit 1 > > > > > > mkdir -p "${TMP_MOUNT}${m}" || 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 EXIT > > > fi > > > else > > > echo >&2 "fixfiles: No suitable file systems found" > > > -- > > > 2.37.3 > > > > > > > -- > Ondrej Mosnacek > Senior Software Engineer, Linux Security - SELinux kernel > Red Hat, Inc. > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4] fixfiles: Unmount temporary bind mounts on SIGINT 2022-09-16 15:36 ` Christian Göttsche 2022-09-19 8:39 ` Ondrej Mosnacek @ 2022-10-07 13:46 ` Petr Lautrbach 2022-11-04 10:41 ` Petr Lautrbach 1 sibling, 1 reply; 15+ messages in thread From: Petr Lautrbach @ 2022-10-07 13:46 UTC (permalink / raw) To: selinux; +Cc: Petr Lautrbach `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. 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> --- 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> (September 16) (inbox) policycoreutils/scripts/fixfiles | 36 +++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles index c72ca0eb9d61..af64a5a567a6 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 -x -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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4] fixfiles: Unmount temporary bind mounts on SIGINT 2022-10-07 13:46 ` [PATCH v4] " Petr Lautrbach @ 2022-11-04 10:41 ` Petr Lautrbach 2022-11-04 11:20 ` Christian Göttsche 0 siblings, 1 reply; 15+ messages in thread From: Petr Lautrbach @ 2022-11-04 10:41 UTC (permalink / raw) To: selinux Petr Lautrbach <plautrba@redhat.com> writes: > `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. > > 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> Is there anyone who objects? Petr > --- > 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> (September 16) (inbox) > > > policycoreutils/scripts/fixfiles | 36 +++++++++++++++++++++++++------- > 1 file changed, 28 insertions(+), 8 deletions(-) > > diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles > index c72ca0eb9d61..af64a5a567a6 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 -x -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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] fixfiles: Unmount temporary bind mounts on SIGINT 2022-11-04 10:41 ` Petr Lautrbach @ 2022-11-04 11:20 ` Christian Göttsche 2022-11-04 11:41 ` Petr Lautrbach 0 siblings, 1 reply; 15+ messages in thread From: Christian Göttsche @ 2022-11-04 11:20 UTC (permalink / raw) To: Petr Lautrbach; +Cc: selinux On Fri, 4 Nov 2022 at 11:42, Petr Lautrbach <plautrba@redhat.com> wrote: > > Petr Lautrbach <plautrba@redhat.com> writes: > > > `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. > > > > 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> > > > Is there anyone who objects? > > Petr > > > > --- > > 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> (September 16) (inbox) > > > > > > policycoreutils/scripts/fixfiles | 36 +++++++++++++++++++++++++------- > > 1 file changed, 28 insertions(+), 8 deletions(-) > > > > diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles > > index c72ca0eb9d61..af64a5a567a6 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)" Whitespace issue: git apply ~/Downloads/v4-fixfiles-Unmount-temporary-bind-mounts-on-SIGINT.patch /home/christian/Downloads/v4-fixfiles-Unmount-temporary-bind-mounts-on-SIGINT.patch:137: space before tab in indent. TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)" warning: 1 line adds whitespace errors. > > + export SETFILES VERBOSE EXCLUDEDIRS FORCEFLAG THREADS FC TMP_MOUNT m > > + if type unshare &> /dev/null; then > > + unshare -m bash -x -c "fix_labels_on_mountpoint" $* || exit $? Two issues: I. The `-x` flag make the output unreadable (especially for check/verify). II. The option (e.g. `-n` for check/verify) is not passed, should be `-c "fix_labels_on_mountpoint $*"`. > > + else > > + trap umount_TMP_MOUNT EXIT > > + fix_labels_on_mountpoint $* > > + trap EXIT > > + fi > > done; > > fi > > else > > -- > > 2.37.3 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] fixfiles: Unmount temporary bind mounts on SIGINT 2022-11-04 11:20 ` Christian Göttsche @ 2022-11-04 11:41 ` Petr Lautrbach 2022-11-05 9:24 ` Dominick Grift 0 siblings, 1 reply; 15+ messages in thread From: Petr Lautrbach @ 2022-11-04 11:41 UTC (permalink / raw) To: Christian Göttsche; +Cc: selinux Christian Göttsche <cgzones@googlemail.com> writes: > On Fri, 4 Nov 2022 at 11:42, Petr Lautrbach <plautrba@redhat.com> wrote: >> >> Petr Lautrbach <plautrba@redhat.com> writes: >> >> > `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. >> > >> > 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> >> >> >> Is there anyone who objects? >> >> Petr >> >> >> > --- >> > 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> (September 16) (inbox) >> > >> > >> > policycoreutils/scripts/fixfiles | 36 +++++++++++++++++++++++++------- >> > 1 file changed, 28 insertions(+), 8 deletions(-) >> > >> > diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles >> > index c72ca0eb9d61..af64a5a567a6 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)" > > Whitespace issue: > > git apply ~/Downloads/v4-fixfiles-Unmount-temporary-bind-mounts-on-SIGINT.patch > /home/christian/Downloads/v4-fixfiles-Unmount-temporary-bind-mounts-on-SIGINT.patch:137: > space before tab in indent. > TMP_MOUNT="$(mktemp -p /run -d fixfiles.XXXXXXXXXX)" > warning: 1 line adds whitespace errors. > >> > + export SETFILES VERBOSE EXCLUDEDIRS FORCEFLAG THREADS FC TMP_MOUNT m >> > + if type unshare &> /dev/null; then >> > + unshare -m bash -x -c "fix_labels_on_mountpoint" $* || exit $? > > Two issues: > > I. > The `-x` flag make the output unreadable (especially for check/verify). This is leftover from my debugging. It is not supposed to be there. > II. > The option (e.g. `-n` for check/verify) is not passed, should be `-c > "fix_labels_on_mountpoint $*"`. I'll fix it. Thanks! >> > + else >> > + trap umount_TMP_MOUNT EXIT >> > + fix_labels_on_mountpoint $* >> > + trap EXIT >> > + fi >> > done; >> > fi >> > else >> > -- >> > 2.37.3 >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] fixfiles: Unmount temporary bind mounts on SIGINT 2022-11-04 11:41 ` Petr Lautrbach @ 2022-11-05 9:24 ` Dominick Grift 0 siblings, 0 replies; 15+ messages in thread From: Dominick Grift @ 2022-11-05 9:24 UTC (permalink / raw) To: Petr Lautrbach; +Cc: Christian Göttsche, selinux On 2022-11-04 12:41, Petr Lautrbach wrote: > Christian Göttsche <cgzones@googlemail.com> writes: > >> On Fri, 4 Nov 2022 at 11:42, Petr Lautrbach <plautrba@redhat.com> >> wrote: >>> >>> Petr Lautrbach <plautrba@redhat.com> writes: >>> >>> > `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. >>> > >>> > 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> >>> >>> >>> Is there anyone who objects? Unrelated but thought i'd mention it here: Since at least Fedora uses btrfs by default on Workstation it might be worth it to look into adding support for btrfs (subvolumes) to fixfiles. >>> >>> Petr >>> >>> >>> > --- >>> > 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> (September 16) (inbox) >>> > >>> > >>> > policycoreutils/scripts/fixfiles | 36 +++++++++++++++++++++++++------- >>> > 1 file changed, 28 insertions(+), 8 deletions(-) >>> > >>> > diff --git a/policycoreutils/scripts/fixfiles b/policycoreutils/scripts/fixfiles >>> > index c72ca0eb9d61..af64a5a567a6 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)" >> >> Whitespace issue: >> >> git apply >> ~/Downloads/v4-fixfiles-Unmount-temporary-bind-mounts-on-SIGINT.patch >> /home/christian/Downloads/v4-fixfiles-Unmount-temporary-bind-mounts-on-SIGINT.patch:137: >> space before tab in indent. >> TMP_MOUNT="$(mktemp -p /run -d >> fixfiles.XXXXXXXXXX)" >> warning: 1 line adds whitespace errors. >> >>> > + export SETFILES VERBOSE EXCLUDEDIRS FORCEFLAG THREADS FC TMP_MOUNT m >>> > + if type unshare &> /dev/null; then >>> > + unshare -m bash -x -c "fix_labels_on_mountpoint" $* || exit $? >> >> Two issues: >> >> I. >> The `-x` flag make the output unreadable (especially for >> check/verify). > > > This is leftover from my debugging. It is not supposed to be there. > > >> II. >> The option (e.g. `-n` for check/verify) is not passed, should be `-c >> "fix_labels_on_mountpoint $*"`. > > I'll fix it. > > > Thanks! > > >>> > + else >>> > + trap umount_TMP_MOUNT EXIT >>> > + fix_labels_on_mountpoint $* >>> > + trap EXIT >>> > + fi >>> > done; >>> > fi >>> > else >>> > -- >>> > 2.37.3 >>> -- gpg --locate-keys dominick.grift@defensec.nl Key fingerprint = FCD2 3660 5D6B 9D27 7FC6 E0FF DA7E 521F 10F6 4098 Dominick Grift ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-11-05 9:30 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-15 12:44 [PATCH] fixfiles: Unmount temporary bind mounts on SIGINT Petr Lautrbach 2022-09-15 13:18 ` Ondrej Mosnacek 2022-09-15 16:29 ` Petr Lautrbach 2022-09-16 12:23 ` Ondrej Mosnacek 2022-09-16 13:45 ` Petr Lautrbach 2022-09-15 16:37 ` [PATCH v2] " Petr Lautrbach 2022-09-16 14:13 ` [PATCH v3] " Petr Lautrbach 2022-09-16 15:36 ` Christian Göttsche 2022-09-19 8:39 ` Ondrej Mosnacek 2022-09-19 11:17 ` Christian Göttsche 2022-10-07 13:46 ` [PATCH v4] " Petr Lautrbach 2022-11-04 10:41 ` Petr Lautrbach 2022-11-04 11:20 ` Christian Göttsche 2022-11-04 11:41 ` Petr Lautrbach 2022-11-05 9:24 ` Dominick Grift
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).