7:59 a.m.
The first 2 patches are mere refactoring and small fixes.
Initial discussion:
https://listman.redhat.com/archives/libguestfs/2023-April/031367.html
Andrey Drobyshev (3):
daemon/selinux-relabel: don't exclude "/selinux" if it's non-existent
daemon/selinux-relabel: search for "invalid option" in setfiles output
daemon/selinux-relabel: run setfiles with "-T 0", if supported
daemon/selinux-relabel.c | 31 +++++++++++++++++++++++++++++--
1 file changed, 29 insertions(+), 2 deletions(-)
--
2.31.1
7:59 a.m.
New subject: [PATCH 1/3] daemon/selinux-relabel: don't exclude "/selinux" if it's non-existent
Since RHBZ#726528, filesystem.rpm doesn't include /selinux. setfiles
then gives us the warning: "Can't stat exclude path
"/sysroot/selinux",
No such file or directory - ignoring."
Though the warning is harmless, let's get rid of it by checking the
existence of /selinux directory.
Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com>
---
daemon/selinux-relabel.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/daemon/selinux-relabel.c b/daemon/selinux-relabel.c
index 976cffe37..454486c17 100644
--- a/daemon/selinux-relabel.c
+++ b/daemon/selinux-relabel.c
@@ -21,6 +21,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <sys/stat.h>
#include "guestfs_protocol.h"
#include "daemon.h"
@@ -37,6 +38,17 @@ optgroup_selinuxrelabel_available (void)
return prog_exists ("setfiles");
}
+static int
+dir_exists (const char *dir)
+{
+ struct stat statbuf;
+
+ if (stat (dir, &statbuf) == 0 && S_ISDIR (statbuf.st_mode))
+ return 1;
+ else
+ return 0;
+}
+
static int
setfiles_has_option (int *flag, char opt_char)
{
@@ -99,8 +111,10 @@ do_selinux_relabel (const char *specfile, const char *path,
*/
ADD_ARG (argv, i, "-e"); ADD_ARG (argv, i, s_dev);
ADD_ARG (argv, i, "-e"); ADD_ARG (argv, i, s_proc);
- ADD_ARG (argv, i, "-e"); ADD_ARG (argv, i, s_selinux);
ADD_ARG (argv, i, "-e"); ADD_ARG (argv, i, s_sys);
+ if (dir_exists (s_selinux)) {
+ ADD_ARG (argv, i, "-e"); ADD_ARG (argv, i, s_selinux);
+ }
/* You have to use the -m option (where available) otherwise
* setfiles puts all the mountpoints on the excludes list for no
--
2.31.1
7:59 a.m.
New subject: [PATCH 2/3] daemon/selinux-relabel: search for "invalid option" in setfiles output
'X' in the setiles' stderr doesn't necessarily mean that option
'X'
doesn't exist. For instance, when passing '-T' we get: "setfiles:
option requires an argument -- 'T'".
Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com>
---
daemon/selinux-relabel.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/daemon/selinux-relabel.c b/daemon/selinux-relabel.c
index 454486c17..60a6f48a9 100644
--- a/daemon/selinux-relabel.c
+++ b/daemon/selinux-relabel.c
@@ -56,8 +56,9 @@ setfiles_has_option (int *flag, char opt_char)
if (*flag == -1) {
char option[] = { '-', opt_char, '\0' }; /* "-X" */
- char err_opt[] = { '\'', opt_char, '\'', '\0'}; /*
"'X'" */
+ char err_opt[32]; /* "invalid option -- 'X'" */
+ snprintf(err_opt, sizeof(err_opt), "invalid option -- '%c'",
opt_char);
ignore_value (command (NULL, &err, "setfiles", option, NULL));
*flag = err && strstr (err, /* "invalid option -- " */ err_opt) ==
NULL;
}
--
2.31.1
9:37 a.m.
New subject: [PATCH 2/3] daemon/selinux-relabel: search for "invalid option" in setfiles output
On 4/26/23 14:59, Andrey Drobyshev wrote:
'X' in the setiles' stderr doesn't necessarily mean
that option 'X'
doesn't exist. For instance, when passing '-T' we get: "setfiles:
option requires an argument -- 'T'".
Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com>
---
daemon/selinux-relabel.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/daemon/selinux-relabel.c b/daemon/selinux-relabel.c
index 454486c17..60a6f48a9 100644
--- a/daemon/selinux-relabel.c
+++ b/daemon/selinux-relabel.c
@@ -56,8 +56,9 @@ setfiles_has_option (int *flag, char opt_char)
if (*flag == -1) {
char option[] = { '-', opt_char, '\0' }; /* "-X" */
- char err_opt[] = { '\'', opt_char, '\'', '\0'}; /*
"'X'" */
+ char err_opt[32]; /* "invalid option -- 'X'" */
+ snprintf(err_opt, sizeof(err_opt), "invalid option -- '%c'",
opt_char);
ignore_value (command (NULL, &err, "setfiles", option, NULL));
*flag = err && strstr (err, /* "invalid option -- " */ err_opt) ==
NULL;
}
Can you check in the selinux library git history how far back the
invalid option -- '%c'
message can be relied upon?
Other than that, I'd suggest a number of superficial updates, but for a
change, I won't obsess about them.
series
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
(We shouldn't merge this until Rich agrees, too.)
Laszlo
9:47 a.m.
New subject: [PATCH 2/3] daemon/selinux-relabel: search for "invalid option" in setfiles output
On Wed, Apr 26, 2023 at 04:37:21PM +0200, Laszlo Ersek wrote:
On 4/26/23 14:59, Andrey Drobyshev wrote:
> 'X' in the setiles' stderr doesn't necessarily mean that option
'X'
> doesn't exist. For instance, when passing '-T' we get: "setfiles:
> option requires an argument -- 'T'".
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com>
> ---
> daemon/selinux-relabel.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/daemon/selinux-relabel.c b/daemon/selinux-relabel.c
> index 454486c17..60a6f48a9 100644
> --- a/daemon/selinux-relabel.c
> +++ b/daemon/selinux-relabel.c
> @@ -56,8 +56,9 @@ setfiles_has_option (int *flag, char opt_char)
>
> if (*flag == -1) {
> char option[] = { '-', opt_char, '\0' }; /*
"-X" */
> - char err_opt[] = { '\'', opt_char, '\'', '\0'};
/* "'X'" */
> + char err_opt[32]; /* "invalid option -- 'X'" */
>
> + snprintf(err_opt, sizeof(err_opt), "invalid option -- '%c'",
opt_char);
> ignore_value (command (NULL, &err, "setfiles", option, NULL));
> *flag = err && strstr (err, /* "invalid option -- " */
err_opt) == NULL;
> }
Can you check in the selinux library git history how far back the
invalid option -- '%c'
message can be relied upon?
It actually comes from glibc:
https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/getopt.c;h=1e2441c...
It's actually been around "forever" .. It exists in the initial
import (to CVS?) from 1995:
https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/posix/getopt.c;h=7...
and there's even a claim there about POSIX -- but it would only apply
for POSIXLY_CORRECT which we don't use in the daemon.
But yes it looks safe!
Rich.
Other than that, I'd suggest a number of superficial updates, but for a
change, I won't obsess about them.
series
Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
(We shouldn't merge this until Rich agrees, too.)
Laszlo
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
9:51 a.m.
New subject: [PATCH 2/3] daemon/selinux-relabel: search for "invalid option" in setfiles output
On 4/26/23 17:47, Richard W.M. Jones wrote:
On Wed, Apr 26, 2023 at 04:37:21PM +0200, Laszlo Ersek wrote:
> On 4/26/23 14:59, Andrey Drobyshev wrote:
>> 'X' in the setiles' stderr doesn't necessarily mean that option
'X'
>> doesn't exist. For instance, when passing '-T' we get:
"setfiles:
>> option requires an argument -- 'T'".
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com>
>> ---
>> daemon/selinux-relabel.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/daemon/selinux-relabel.c b/daemon/selinux-relabel.c
>> index 454486c17..60a6f48a9 100644
>> --- a/daemon/selinux-relabel.c
>> +++ b/daemon/selinux-relabel.c
>> @@ -56,8 +56,9 @@ setfiles_has_option (int *flag, char opt_char)
>>
>> if (*flag == -1) {
>> char option[] = { '-', opt_char, '\0' }; /*
"-X" */
>> - char err_opt[] = { '\'', opt_char, '\'',
'\0'}; /* "'X'" */
>> + char err_opt[32]; /* "invalid option -- 'X'" */
>>
>> + snprintf(err_opt, sizeof(err_opt), "invalid option --
'%c'", opt_char);
>> ignore_value (command (NULL, &err, "setfiles", option,
NULL));
>> *flag = err && strstr (err, /* "invalid option -- " */
err_opt) == NULL;
>> }
>
> Can you check in the selinux library git history how far back the
>
> invalid option -- '%c'
>
> message can be relied upon?
It actually comes from glibc:
https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/getopt.c;h=1e2441c...
It's actually been around "forever" .. It exists in the initial
import (to CVS?) from 1995:
https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/posix/getopt.c;h=7...
and there's even a claim there about POSIX -- but it would only apply
for POSIXLY_CORRECT which we don't use in the daemon.
In fact, since glibc commit 1e489af957 ("* posix/getopt.c
(_getopt_internal_r): Remove old POSIX-demanded") the alternative
posixly correct form "illegal option" was dropped, so now "invalid
option" is the only message used by getopt().
https://sourceware.org/git/?p=glibc.git;a=commit;h=1e489af957dbf19c5080dc...
But yes it looks safe!
Rich.
>
> Other than that, I'd suggest a number of superficial updates, but for a
> change, I won't obsess about them.
>
> series
> Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
>
> (We shouldn't merge this until Rich agrees, too.)
>
> Laszlo
3 a.m.
New subject: [PATCH 2/3] daemon/selinux-relabel: search for "invalid option" in setfiles output
On 4/26/23 16:47, Richard W.M. Jones wrote:
On Wed, Apr 26, 2023 at 04:37:21PM +0200, Laszlo Ersek wrote:
> On 4/26/23 14:59, Andrey Drobyshev wrote:
>> 'X' in the setiles' stderr doesn't necessarily mean that option
'X'
>> doesn't exist. For instance, when passing '-T' we get:
"setfiles:
>> option requires an argument -- 'T'".
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com>
>> ---
>> daemon/selinux-relabel.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/daemon/selinux-relabel.c b/daemon/selinux-relabel.c
>> index 454486c17..60a6f48a9 100644
>> --- a/daemon/selinux-relabel.c
>> +++ b/daemon/selinux-relabel.c
>> @@ -56,8 +56,9 @@ setfiles_has_option (int *flag, char opt_char)
>>
>> if (*flag == -1) {
>> char option[] = { '-', opt_char, '\0' }; /*
"-X" */
>> - char err_opt[] = { '\'', opt_char, '\'',
'\0'}; /* "'X'" */
>> + char err_opt[32]; /* "invalid option -- 'X'" */
>>
>> + snprintf(err_opt, sizeof(err_opt), "invalid option --
'%c'", opt_char);
>> ignore_value (command (NULL, &err, "setfiles", option,
NULL));
>> *flag = err && strstr (err, /* "invalid option -- " */
err_opt) == NULL;
>> }
>
> Can you check in the selinux library git history how far back the
>
> invalid option -- '%c'
>
> message can be relied upon?
It actually comes from glibc:
Ah, thanks for checking that.
That brings back some (distant) memories.
I've repeatedly used getopt() myself in the past, it's just that I could
never see the point of letting getopt() print an error message on its
own. Also I always wanted invalid options to be reported differently to
me from missing option arguments. For those reasons, I'd always place a
<colon> at the front of optstring. I'd gotten so used to that practice
that now it simply didn't occur to me that any application might rely on
the diagnostic message printed by getopt().
https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/getopt.c;h=1e2441c...
It's actually been around "forever" .. It exists in the initial
import (to CVS?) from 1995:
https://sourceware.org/git/?p=glibc.git;a=blob;f=posix/posix/getopt.c;h=7...
and there's even a claim there about POSIX -- but it would only apply
for POSIXLY_CORRECT which we don't use in the daemon.
But yes it looks safe!
We always depend on the setfiles utility, but with the above
information, we also depend on the host's libc -- after all, the
appliance gets built from the host's packages. From
"appliance/packagelist.in", we try to support REDHAT, DEBIAN, UBUNTU,
ARCHLINUX, SUSE, FRUGALWARE, MAGEIA; do they all default to glibc? (I've
only heard of Alma Linux using musl, but who knows...)
On the other hand... I realize that even *pre-patch*; we already depend
on getopt()'s *internally-printed* "'X'" substring! In other words,
this
patch does not make our dependency on libc internals stronger than it
already is.
So, my R-b stands.
Thanks!
Laszlo
Rich.
>
> Other than that, I'd suggest a number of superficial updates, but for a
> change, I won't obsess about them.
>
> series
> Reviewed-by: Laszlo Ersek <lersek(a)redhat.com>
>
> (We shouldn't merge this until Rich agrees, too.)
>
> Laszlo
7:59 a.m.
New subject: [PATCH 3/3] daemon/selinux-relabel: run setfiles with "-T 0", if supported
Since SELinux userspace v3.4 [1], setfiles command supports "-T nthreads"
option, which allows parallel execution. "-T 0" allows using as many
threads as there're available CPU cores. This might speed up the process
of filesystem relabeling in case the appliance is being run with multiple
vCPUs. The latter is true for at least v2v starting from d2b64ecc67
("v2v: Set the number of vCPUs to same as host number of pCPUs.").
For instance, when running virt-v2v-in-place on my 12-core Xeon host
with SSD, with appliance being run with 8 vCPUs (the upper limit specified
in d2b64ecc67), and on the ~150GiB disk VM (physical size on the host),
I get the following results:
./in-place/virt-v2v-in-place -i libvirt fedora37-vm -v -x
Without this patch:
...
commandrvf: setfiles -F -e /sysroot/dev -e /sysroot/proc -e /sysroot/sys -m -C -r /sysroot
-v /sysroot/etc/selinux/targeted/contexts/files/file_contexts /sysroot/^M
libguestfs: trace: v2v: selinux_relabel = 0
libguestfs: trace: v2v: rm_f "/.autorelabel"
guestfsd: => selinux_relabel (0x1d3) took 17.94 secs
...
With this patch:
...
commandrvf: setfiles -F -e /sysroot/dev -e /sysroot/proc -e /sysroot/sys -m -C -T 0 -r
/sysroot -v /sysroot/etc/selinux/targeted/contexts/files/file_contexts /sysroot/^M
libguestfs: trace: v2v: selinux_relabel = 0
libguestfs: trace: v2v: rm_f "/.autorelabel"
guestfsd: => selinux_relabel (0x1d3) took 5.88 secs
...
So in my scenario it's getting 3 times faster.
[1] https://github.com/SELinuxProject/selinux/releases/tag/3.4
Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com>
---
daemon/selinux-relabel.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/daemon/selinux-relabel.c b/daemon/selinux-relabel.c
index 60a6f48a9..cfc5a31d9 100644
--- a/daemon/selinux-relabel.c
+++ b/daemon/selinux-relabel.c
@@ -73,6 +73,7 @@ do_selinux_relabel (const char *specfile, const char *path,
{
static int flag_m = -1;
static int flag_C = -1;
+ static int flag_T = -1;
const char *argv[MAX_ARGS];
CLEANUP_FREE char *s_dev = NULL, *s_proc = NULL, *s_selinux = NULL,
*s_sys = NULL, *s_specfile = NULL, *s_path = NULL;
@@ -131,6 +132,17 @@ do_selinux_relabel (const char *specfile, const char *path,
if (setfiles_has_option (&flag_C, 'C'))
ADD_ARG (argv, i, "-C");
+ /* If the appliance is being run with multiple vCPUs, running setfiles
+ * in multithreading mode might speeds up the process. Option "-T" was
+ * introduced in SELinux userspace v3.4, and we need to check whether it's
+ * supported. Passing "-T 0" creates as many threads as there're
available
+ * vCPU cores.
+ * https://github.com/SELinuxProject/selinux/releases/tag/3.4
+ */
+ if (setfiles_has_option (&flag_T, 'T')) {
+ ADD_ARG (argv, i, "-T"); ADD_ARG (argv, i, "0");
+ }
+
/* Relabelling in a chroot. */
if (STRNEQ (sysroot, "/")) {
ADD_ARG (argv, i, "-r");
--
2.31.1
9:48 a.m.
New subject: [PATCH 3/3] daemon/selinux-relabel: run setfiles with "-T 0", if supported
Series looks good, so also:
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
I'll get around to this in a bit, let's just wait a few hours in case
there are further comments.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
9:31 a.m.
New subject: [PATCH 3/3] daemon/selinux-relabel: run setfiles with "-T 0", if supported
Thanks, patches upstream in 7674d603e9..d0d8e67384.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
389
days inactive
390
days old
9 comments
3 participants
participants (3)
-
Andrey Drobyshev -
Laszlo Ersek -
Richard W.M. Jones