selinux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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

* [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

* 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 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).