Running 297 from GitLab CI
Hiya, I was talking this over with Hanna in review to '[PATCH v3 00/16] python/iotests: Run iotest linters during Python CI' [1] and I have some doubt about what you'd personally like to see happen, here. In a nutshell, I split out 'linters.py' from 297 and keep all of the iotest-bits in 297 and all of the generic "run the linters" bits in linters.py, then I run linters.py from the GitLab python CI jobs. I did this so that iotest #297 would continue to work exactly as it had, but trying to serve "two masters" in the form of two test suites means some non-beautiful design decisions. Hanna suggested we just outright drop test 297 to possibly improve the factoring of the tests. I don't want to do that unless you give it the go-ahead, though. I wanted to hear your feelings on if we still want to keep 297 around or not. --js [1] https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05787.html
Re: [PATCH v3] nbd/server: Add --selinux-label option
On Thu, Sep 30, 2021 at 11:54:58AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 9/30/21 11:47, Richard W.M. Jones wrote:
> > Under SELinux, Unix domain sockets have two labels. One is on the
> > disk and can be set with commands such as chcon(1). There is a
> > different label stored in memory (called the process label). This can
> > only be set by the process creating the socket. When using SELinux +
> > SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
> > you must set both labels correctly first.
> >
> > For qemu-nbd the options to set the second label are awkward. You can
> > create the socket in a wrapper program and then exec into qemu-nbd.
> > Or you could try something with LD_PRELOAD.
> >
> > This commit adds the ability to set the label straightforwardly on the
> > command line, via the new --selinux-label flag. (The name of the flag
> > is the same as the equivalent nbdkit option.)
> >
> > A worked example showing how to use the new option can be found in
> > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> >
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> > Signed-off-by: Richard W.M. Jones
> > Reviewed-by: Daniel P. Berrangé
> > Signed-off-by: Eric Blake
>
> this should be Reviewed-by?
In this case, I think S-o-b is actually correct: I did make some
tweaks to Rich's v2 while preparing my pull request, so Rich is
crediting my addition to his work. And at the time of my pull request
that included his v2 (before it got dropped for 32-bit build
problems), I had not actually sent my R-b, because I was already
trusting the R-b present from other reviewers.
Oddly enough, even if Rich had dropped my S-o-b line, it will still
eventually reappear, since I'll be queuing this patch through my NBD
tree which requires me to touch it again. So already having it now
doesn't hurt.
[Many of the patches that go through my tree end up with both my R-b
and S-o-b; but there are patches where I have S-o-b but not R-b,
because I trusted the review of others, and view the act of a careful
review as orthogonal from the responsibility of touching a patch
enough to include it in a pull request]
>
> > ---
> > configure | 8 +++-
> > meson.build | 10 -
> > meson_options.txt | 3 ++
> > qemu-nbd.c| 39 +++
>
> [..]
>
> > }
> > @@ -938,6 +952,19 @@ int main(int argc, char **argv)
> > } else {
> > backlog = MIN(shared, SOMAXCONN);
> > }
> > +if (sockpath && selinux_label) {
>
> 1. Why only for unix sockets?
>
> 2. If [1] is intentional, why silently ignore the new option for ip sockets,
> shouldn't error-out instead?
>
Good point, and I missed it in v2, in spite of my touching that patch
to avoid silent ignoring when selinux support was not compiled in.
So at this point, I'm less certain whether it is smarter to reject
--selinux-label on non-unix sockets, or whether we try to do the
labeling regardless of socket type; and thus consider it premature for
me to give R-b until we have that resolved.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.
On Wed, Sep 29, 2021 at 01:30:50AM -0400, ~farzon wrote: > From: Farzon Lotfi Food for thought: your git/mail configuration used one address for the envelope (name '~farzon' as user 'farzon@') and another as the patch author (name 'Farzon Lotfi' as user 'hi@'). Since you own your domain (with its own perks), you can get away with it, but it looks a bit less professional to need a second From: line to override the mail author (which is more commonly needed to work around overly strict DKIM settings), compared to just sending your mail from the desired full-name author in the first place. But since your Signed-off-by tag is correct, this is not a show-stopper to applying your patch. However, my next comment does require a respin before your patch would be ready. Your Subject: line is too long, as evidenced by your choice of using sentences. It should really be 'category: short description' all within 60 characters or so (when 'git log' displays indentation, a short commit id, and then your subject, you don't want your subject truncated). It feels like some of your subject should instead be part of the commit body, where you currently have only... > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371 ...because that one line as a body is rather sparse. While the URL is nice (it is a lifesaver for tracking whether a particular bug has a known patch), it does not tell me at a glance what is behind that URL, and I don't want to have to fire up my browser to learn about your patch. In general, the subject should be a short "what", and the commit body should be "why" a maintainer should apply it. I'd suggest the following: block: Replace TABs with space Bring the block file in line with the QEMU coding style, with spaces for indentation. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371 > > Signed-off-by: Farzon Lotfi > --- -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.
On Thu, Sep 30, 2021 at 02:55:16PM +0200, Kevin Wolf wrote: > > > When we're changing these lines anyway, let's make sure to have > > > consistent alignment with the surrounding code. So I would prefer > > > something like: > > > > > > +.bdrv_close = parallels_close, > > > .bdrv_child_perm = bdrv_default_perms, > > > > > > Rather than: > > > > > > +.bdrv_close = parallels_close, > > > .bdrv_child_perm = bdrv_default_perms, > > > > > > In most cases, there are already inconsistencies in the BlockDriver > > > definitions, but let's use the chance to make it a little better. > > > > > > Or may be drop alignment around '=' at all, to have > > > >.bdrv_child_perm = bdrv_default_perms, > >.bdrv_co_block_status = parallels_co_block_status, > >.bdrv_has_zero_init = bdrv_has_zero_init_1, > > > > ? > > You're right that this would make it easy to keep things consistent, but > I think it hurts readability a lot, even compared to the current, often > inconsistent state. I agree that the alignment adds a bit of readability, but also understand that it adds a maintenacne burden. Thus, in code I manage, I'm fine with either style (no extra spaces, or '=' lined up); and can live with different styles in different files (which I then will honor when doing a grunt-work patch that touches all of the block drivers). But what I don't like is when a single file cannot be consistent with itself on which of those two styles it is using - a file that uses aligned '=' really needs to put that '=' far enough to the right that an added long-named member doesn't cause frequent reindentation of the rest of the members. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3] nbd/server: Add --selinux-label option
9/30/21 21:37, Richard W.M. Jones wrote: On Thu, Sep 30, 2021 at 02:00:11PM -0300, Willian Rampazzo wrote: On Thu, Sep 30, 2021 at 5:55 AM Vladimir Sementsov-Ogievskiy wrote: 9/30/21 11:47, Richard W.M. Jones wrote: Under SELinux, Unix domain sockets have two labels. One is on the disk and can be set with commands such as chcon(1). There is a different label stored in memory (called the process label). This can only be set by the process creating the socket. When using SELinux + SVirt and wanting qemu to be able to connect to a qemu-nbd instance, you must set both labels correctly first. For qemu-nbd the options to set the second label are awkward. You can create the socket in a wrapper program and then exec into qemu-nbd. Or you could try something with LD_PRELOAD. This commit adds the ability to set the label straightforwardly on the command line, via the new --selinux-label flag. (The name of the flag is the same as the equivalent nbdkit option.) A worked example showing how to use the new option can be found in this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 Signed-off-by: Richard W.M. Jones Reviewed-by: Daniel P. Berrangé Signed-off-by: Eric Blake this should be Reviewed-by? Maybe, because of this: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07081.html I got confused with this v3. Yes, I'd somehow lost the original patch and picked it up from Eric's queue to make v3. Than it's probably correct. Still a bit strange to send own patch with another s-o-b in the end. Having said that I'm not sure what the objection above means. Do you mean Eric's tag should be Reviewed-by instead of S-o-b? (And why?) I thought you just accidentally used s-o-b instead of r-b. -- Best regards, Vladimir
Re: [PATCH v3] nbd/server: Add --selinux-label option
On Thu, Sep 30, 2021 at 02:00:11PM -0300, Willian Rampazzo wrote: > On Thu, Sep 30, 2021 at 5:55 AM Vladimir Sementsov-Ogievskiy > wrote: > > > > 9/30/21 11:47, Richard W.M. Jones wrote: > > > Under SELinux, Unix domain sockets have two labels. One is on the > > > disk and can be set with commands such as chcon(1). There is a > > > different label stored in memory (called the process label). This can > > > only be set by the process creating the socket. When using SELinux + > > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > > > you must set both labels correctly first. > > > > > > For qemu-nbd the options to set the second label are awkward. You can > > > create the socket in a wrapper program and then exec into qemu-nbd. > > > Or you could try something with LD_PRELOAD. > > > > > > This commit adds the ability to set the label straightforwardly on the > > > command line, via the new --selinux-label flag. (The name of the flag > > > is the same as the equivalent nbdkit option.) > > > > > > A worked example showing how to use the new option can be found in > > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > Signed-off-by: Richard W.M. Jones > > > Reviewed-by: Daniel P. Berrangé > > > Signed-off-by: Eric Blake > > > > this should be Reviewed-by? > > Maybe, because of this: > https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07081.html > > I got confused with this v3. Yes, I'd somehow lost the original patch and picked it up from Eric's queue to make v3. Having said that I'm not sure what the objection above means. Do you mean Eric's tag should be Reviewed-by instead of S-o-b? (And why?) Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Re: [PATCH v3] nbd/server: Add --selinux-label option
On Thu, Sep 30, 2021 at 5:55 AM Vladimir Sementsov-Ogievskiy wrote: > > 9/30/21 11:47, Richard W.M. Jones wrote: > > Under SELinux, Unix domain sockets have two labels. One is on the > > disk and can be set with commands such as chcon(1). There is a > > different label stored in memory (called the process label). This can > > only be set by the process creating the socket. When using SELinux + > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > > you must set both labels correctly first. > > > > For qemu-nbd the options to set the second label are awkward. You can > > create the socket in a wrapper program and then exec into qemu-nbd. > > Or you could try something with LD_PRELOAD. > > > > This commit adds the ability to set the label straightforwardly on the > > command line, via the new --selinux-label flag. (The name of the flag > > is the same as the equivalent nbdkit option.) > > > > A worked example showing how to use the new option can be found in > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > Signed-off-by: Richard W.M. Jones > > Reviewed-by: Daniel P. Berrangé > > Signed-off-by: Eric Blake > > this should be Reviewed-by? Maybe, because of this: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07081.html I got confused with this v3.
Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.
Am 30.09.2021 um 14:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 9/30/21 12:46, Kevin Wolf wrote:
> > Am 29.09.2021 um 07:30 hat ~farzon geschrieben:
> > > From: Farzon Lotfi
> > >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371
> > >
> > > Signed-off-by: Farzon Lotfi
> >
> > Just picking one example, but it applies to most hunks of the patch:
> >
> > > diff --git a/block/parallels.c b/block/parallels.c
> > > index 6ebad2a2bb..629d8aae2b 100644
> > > --- a/block/parallels.c
> > > +++ b/block/parallels.c
> > > @@ -916,11 +916,11 @@ static void parallels_close(BlockDriverState *bs)
> > > }
> > > static BlockDriver bdrv_parallels = {
> > > -.format_name = "parallels",
> > > -.instance_size = sizeof(BDRVParallelsState),
> > > -.bdrv_probe = parallels_probe,
> > > -.bdrv_open = parallels_open,
> > > -.bdrv_close = parallels_close,
> > > +.format_name= "parallels",
> > > +.instance_size = sizeof(BDRVParallelsState),
> > > +.bdrv_probe = parallels_probe,
> > > +.bdrv_open = parallels_open,
> > > +.bdrv_close = parallels_close,
> > > .bdrv_child_perm = bdrv_default_perms,
> > > .bdrv_co_block_status = parallels_co_block_status,
> > > .bdrv_has_zero_init = bdrv_has_zero_init_1,
> >
> > When we're changing these lines anyway, let's make sure to have
> > consistent alignment with the surrounding code. So I would prefer
> > something like:
> >
> > +.bdrv_close = parallels_close,
> > .bdrv_child_perm = bdrv_default_perms,
> >
> > Rather than:
> >
> > +.bdrv_close = parallels_close,
> > .bdrv_child_perm = bdrv_default_perms,
> >
> > In most cases, there are already inconsistencies in the BlockDriver
> > definitions, but let's use the chance to make it a little better.
>
>
> Or may be drop alignment around '=' at all, to have
>
>.bdrv_child_perm = bdrv_default_perms,
>.bdrv_co_block_status = parallels_co_block_status,
>.bdrv_has_zero_init = bdrv_has_zero_init_1,
>
> ?
You're right that this would make it easy to keep things consistent, but
I think it hurts readability a lot, even compared to the current, often
inconsistent state.
Kevin
Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.
9/30/21 12:46, Kevin Wolf wrote:
Am 29.09.2021 um 07:30 hat ~farzon geschrieben:
From: Farzon Lotfi
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371
Signed-off-by: Farzon Lotfi
Just picking one example, but it applies to most hunks of the patch:
diff --git a/block/parallels.c b/block/parallels.c
index 6ebad2a2bb..629d8aae2b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -916,11 +916,11 @@ static void parallels_close(BlockDriverState *bs)
}
static BlockDriver bdrv_parallels = {
-.format_name = "parallels",
-.instance_size = sizeof(BDRVParallelsState),
-.bdrv_probe= parallels_probe,
-.bdrv_open = parallels_open,
-.bdrv_close= parallels_close,
+.format_name= "parallels",
+.instance_size = sizeof(BDRVParallelsState),
+.bdrv_probe = parallels_probe,
+.bdrv_open = parallels_open,
+.bdrv_close = parallels_close,
.bdrv_child_perm = bdrv_default_perms,
.bdrv_co_block_status = parallels_co_block_status,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
When we're changing these lines anyway, let's make sure to have
consistent alignment with the surrounding code. So I would prefer
something like:
+.bdrv_close = parallels_close,
.bdrv_child_perm = bdrv_default_perms,
Rather than:
+.bdrv_close = parallels_close,
.bdrv_child_perm = bdrv_default_perms,
In most cases, there are already inconsistencies in the BlockDriver
definitions, but let's use the chance to make it a little better.
Or may be drop alignment around '=' at all, to have
.bdrv_child_perm = bdrv_default_perms,
.bdrv_co_block_status = parallels_co_block_status,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
?
This alignment in definitions is
1. source for alignment inconsistencies
2. infecting logic-changing patches by fixing surround alignment (or having to
add separate patches to adjust old alignments, which is a real waste of time)
3. [1] and [2] will never be helpful for rebase conflicts resolution, when need
to backport/forwardport commits.
and for that all we have a bit more readable definition, which is rarely read as is.
(I think, it's more often used to navigate by tags, like bdrv_open -> jump to
invocation in qcow2.c -> jump to qcow2_open)
--
Best regards,
Vladimir
Re: [RFC PATCH 00/10] security: Introduce qemu_security_policy_taint() API
On Tue, Sep 14, 2021 at 01:30:27PM +, P J P wrote: > Hello Philippe, all > > >On Thursday, 9 September, 2021, 03:58:40 pm IST, Daniel P. Berrangé > > wrote: > >On Thu, Sep 09, 2021 at 01:20:14AM +0200, Philippe Mathieu-Daudé wrote: > >> This series is experimental! The goal is to better limit the > >> boundary of what code is considerated security critical, and > >> what is less critical (but still important!). > >> > >> This approach was quickly discussed few months ago with Markus > >> then Daniel. Instead of classifying the code on a file path > >> basis (see [1]), we insert (runtime) hints into the code > >> (which survive code movement). Offending unsafe code can > >> taint the global security policy. By default this policy > >> is 'none': the current behavior. It can be changed on the > >> command line to 'warn' to display warnings, and to 'strict' > >> to prohibit QEMU running with a tainted policy. > > > > * Thanks so much for restarting this thread. I've been at it intermittently > last few > months, thinking about how could we annotate the source/module objects. > > -> [*] https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04642.html > > * Last time we discussed about having both compile and run time options for > users > to be able to select the qualified objects/backends/devices as desired. Right, we have multiple different use cases here - People building QEMU who want to cut down what they ship to minimize the stuff they support, which is outside the security guarantee. This can be OS distros, but also any other consumer of QEMU eg. RHEL wants to cut out almost all non-virtualization stuff. There is a quirk here though, that RHEL still includes TCG which is considered outside the security guarantee. So a simple build time "--secure on|off" doesn't do the job on its own. We need something to let people understand the consequences of the build options they are enabling. NB, when I talk of build options, I include both configure/meson args, and also the CONFIG_* options set in configs/**/*.mak - Application developers want to check that they're not using stuff outside the security guarantee, even if the distro has enable it. These need to be able to query whether the VM they've launched has undesirable configuration choices. Some people fall into both groups, some people fall into neither group. > * To confirm: How/Where is the security policy defined? Is it > device/module specific OR QEMU project wide? Currently our only definition is in the docs https://qemu-project.gitlab.io/qemu/system/security.html#security-requirements Philippe's patch is proposing tagging against internal QEMU objects of various types. I further proposed that we expose this in QMP so it is introspectable. I think there's scope for doing stuff at build time with configure args and *mak CONFIG_* options, but haven't thought what that might look like. > > IOW, the reporting via QAPI introspetion is much more important > > for libvirt and mgmt apps, than any custom cli arg / printfs > > at the QEMU level. > > > > * True, while it makes sense to have a solution that is conversant with > the management/libvirt layers, It'll be great if we could address qemu/cli > other use cases too. > > >it feels like we need > > 'secure': 'bool' > > * Though we started the (above [*]) discussion with '--security' option in > mind, > I wonder if 'secure' annotation is much specific. And if we could widen its > scope. > --- x --- > > > Source annotations: I've been thinking over following approaches > === > > 1) Segregate the QEMU sources under > > ../staging/ <= devel/experimental, not for production usage > ../src/ <= good for production usage, hence security relevant > ../deprecated/ <= Bad for production usage, not security relevant > > - This is similar to Linux staging drivers' tree. > - Staging drivers are not considered for production usage and hence CVE > allocation. > - At build time by default we only build sources under ../src/ tree. > - But we can still have options to build /staging/ and /deprecated/ trees. > > - It's readily understandable to end users. I don't think we should be working in terms of source files at all. Some files contain multiple distinct bits of functionality that are not easily separated, and will have distinct security levels. Also IMHO it is unpleasant to be moving files around in git to when code switches between levels. Also there are distinct criteria here, both security levels, and support levels - there can be stuff which is fully supported but considered insecure, and stuff that is deprecated but considered secure. > 2) pkgconfig(1) way: > - If we could define per device/backend a configuration (.pc) file which > is then used > at build/run time to decide which sources are suitable for the build or > usage.
Re: [PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.
Am 29.09.2021 um 07:30 hat ~farzon geschrieben:
> From: Farzon Lotfi
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371
>
> Signed-off-by: Farzon Lotfi
Just picking one example, but it applies to most hunks of the patch:
> diff --git a/block/parallels.c b/block/parallels.c
> index 6ebad2a2bb..629d8aae2b 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -916,11 +916,11 @@ static void parallels_close(BlockDriverState *bs)
> }
>
> static BlockDriver bdrv_parallels = {
> -.format_name = "parallels",
> -.instance_size = sizeof(BDRVParallelsState),
> -.bdrv_probe = parallels_probe,
> -.bdrv_open = parallels_open,
> -.bdrv_close = parallels_close,
> +.format_name= "parallels",
> +.instance_size = sizeof(BDRVParallelsState),
> +.bdrv_probe = parallels_probe,
> +.bdrv_open = parallels_open,
> +.bdrv_close = parallels_close,
> .bdrv_child_perm = bdrv_default_perms,
> .bdrv_co_block_status = parallels_co_block_status,
> .bdrv_has_zero_init = bdrv_has_zero_init_1,
When we're changing these lines anyway, let's make sure to have
consistent alignment with the surrounding code. So I would prefer
something like:
+.bdrv_close = parallels_close,
.bdrv_child_perm = bdrv_default_perms,
Rather than:
+.bdrv_close = parallels_close,
.bdrv_child_perm = bdrv_default_perms,
In most cases, there are already inconsistencies in the BlockDriver
definitions, but let's use the chance to make it a little better.
Kevin
Re: [PATCH v3] nbd/server: Add --selinux-label option
9/30/21 11:47, Richard W.M. Jones wrote:
Under SELinux, Unix domain sockets have two labels. One is on the
disk and can be set with commands such as chcon(1). There is a
different label stored in memory (called the process label). This can
only be set by the process creating the socket. When using SELinux +
SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
you must set both labels correctly first.
For qemu-nbd the options to set the second label are awkward. You can
create the socket in a wrapper program and then exec into qemu-nbd.
Or you could try something with LD_PRELOAD.
This commit adds the ability to set the label straightforwardly on the
command line, via the new --selinux-label flag. (The name of the flag
is the same as the equivalent nbdkit option.)
A worked example showing how to use the new option can be found in
this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
Signed-off-by: Richard W.M. Jones
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Eric Blake
this should be Reviewed-by?
---
configure | 8 +++-
meson.build | 10 -
meson_options.txt | 3 ++
qemu-nbd.c| 39 +++
[..]
}
@@ -938,6 +952,19 @@ int main(int argc, char **argv)
} else {
backlog = MIN(shared, SOMAXCONN);
}
+if (sockpath && selinux_label) {
1. Why only for unix sockets?
2. If [1] is intentional, why silently ignore the new option for ip sockets,
shouldn't error-out instead?
+#ifdef CONFIG_SELINUX
+if (setsockcreatecon_raw(selinux_label) == -1) {
+error_report("Cannot set SELinux socket create context "
+ "to %s: %s",
+ selinux_label, strerror(errno));
+exit(EXIT_FAILURE);
+}
+#else
+error_report("SELinux support not enabled in this binary");
+exit(EXIT_FAILURE);
+#endif
+}
saddr = nbd_build_socket_address(sockpath, bindto, port);
if (qio_net_listener_open_sync(server, saddr, backlog,
_err) < 0) {
@@ -945,6 +972,18 @@ int main(int argc, char **argv)
error_report_err(local_err);
exit(EXIT_FAILURE);
}
+if (sockpath && selinux_label) {
+#ifdef CONFIG_SELINUX
+if (setsockcreatecon_raw(NULL) == -1) {
+error_report("Cannot clear SELinux socket create context: %s",
+ strerror(errno));
+exit(EXIT_FAILURE);
+}
+#else
+error_report("SELinux support not enabled in this binary");
+exit(EXIT_FAILURE);
+#endif
+}
} else {
size_t i;
/* See comment in check_socket_activation above. */
[..]
--
Best regards,
Vladimir
[PATCH v3] nbd/server: Add --selinux-label option
Under SELinux, Unix domain sockets have two labels. One is on the
disk and can be set with commands such as chcon(1). There is a
different label stored in memory (called the process label). This can
only be set by the process creating the socket. When using SELinux +
SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
you must set both labels correctly first.
For qemu-nbd the options to set the second label are awkward. You can
create the socket in a wrapper program and then exec into qemu-nbd.
Or you could try something with LD_PRELOAD.
This commit adds the ability to set the label straightforwardly on the
command line, via the new --selinux-label flag. (The name of the flag
is the same as the equivalent nbdkit option.)
A worked example showing how to use the new option can be found in
this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
Signed-off-by: Richard W.M. Jones
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Eric Blake
---
configure | 8 +++-
meson.build | 10 -
meson_options.txt | 3 ++
qemu-nbd.c| 39 +++
tests/docker/dockerfiles/centos8.docker | 1 +
.../dockerfiles/fedora-i386-cross.docker | 1 +
tests/docker/dockerfiles/fedora.docker| 1 +
tests/docker/dockerfiles/opensuse-leap.docker | 1 +
tests/docker/dockerfiles/ubuntu1804.docker| 1 +
tests/docker/dockerfiles/ubuntu2004.docker| 1 +
10 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/configure b/configure
index 1043ccce4f..b3211a66ee 100755
--- a/configure
+++ b/configure
@@ -445,6 +445,7 @@ fuse="auto"
fuse_lseek="auto"
multiprocess="auto"
slirp_smbd="$default_feature"
+selinux="auto"
malloc_trim="auto"
gio="$default_feature"
@@ -1576,6 +1577,10 @@ for opt do
;;
--disable-slirp-smbd) slirp_smbd=no
;;
+ --enable-selinux) selinux="enabled"
+ ;;
+ --disable-selinux) selinux="disabled"
+ ;;
*)
echo "ERROR: unknown option $opt"
echo "Try '$0 --help' for more information"
@@ -1963,6 +1968,7 @@ disabled with --disable-FEATURE, default is enabled if
available
multiprocessOut of process device emulation support
gio libgio support
slirp-smbd use smbd (at path --smbd=*) in slirp networking
+ selinux SELinux support in qemu-nbd
NOTE: The object files are built at the place where configure is launched
EOF
@@ -5207,7 +5213,7 @@ if test "$skip_meson" = no; then
-Dattr=$attr -Ddefault_devices=$default_devices
-Dvirglrenderer=$virglrenderer \
-Ddocs=$docs -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \
-Dvhost_user_blk_server=$vhost_user_blk_server
-Dmultiprocess=$multiprocess \
--Dfuse=$fuse -Dfuse_lseek=$fuse_lseek
-Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf\
+-Dselinux=$selinux \
$(if test "$default_feature" = no; then echo
"-Dauto_features=disabled"; fi) \
-Dtcg_interpreter=$tcg_interpreter \
$cross_arg \
diff --git a/meson.build b/meson.build
index 15ef4d3c41..0ded2ac5eb 100644
--- a/meson.build
+++ b/meson.build
@@ -1072,6 +1072,11 @@ keyutils = dependency('libkeyutils', required: false,
has_gettid = cc.has_function('gettid')
+# libselinux
+selinux = dependency('libselinux',
+ required: get_option('selinux'),
+ method: 'pkg-config', kwargs: static_kwargs)
+
# Malloc tests
malloc = []
@@ -1300,6 +1305,7 @@ config_host_data.set('CONFIG_FUSE', fuse.found())
config_host_data.set('CONFIG_FUSE_LSEEK', fuse_lseek.found())
config_host_data.set('CONFIG_X11', x11.found())
config_host_data.set('CONFIG_CFI', get_option('cfi'))
+config_host_data.set('CONFIG_SELINUX', selinux.found())
config_host_data.set('QEMU_VERSION', '"@0@"'.format(meson.project_version()))
config_host_data.set('QEMU_VERSION_MAJOR',
meson.project_version().split('.')[0])
config_host_data.set('QEMU_VERSION_MINOR',
meson.project_version().split('.')[1])
@@ -2759,7 +2765,8 @@ if have_tools
qemu_io = executable('qemu-io', files('qemu-io.c'),
dependencies: [block, qemuutil], install: true)
qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
- dependencies: [blockdev, qemuutil, gnutls], install: true)
+ dependencies: [blockdev, qemuutil, gnutls, selinux],
+ install: true)
subdir('storage-daemon')
subdir('contrib/rdmacm-mux')
@@ -3124,6 +3131,7 @@ summary_info += {'libpmem support': libpmem.found()}
summary_info += {'libdaxctl support': libdaxctl.found()}
summary_info += {'libudev': libudev.found()}
summary_info += {'FUSE lseek':fuse_lseek.found()}
+summary_info += {'selinux': selinux.found()}
summary(summary_info, bool_yn: true, section: 'Dependencies')
if not
