(CC David Teigland)

After reading up a bit on the referenced BZs, I think the situation is
worse than reported. The "/etc/lvm/devices/system.devices" file seems to
prevent LVM from scanning all block devices (to see if each block device
is a PV or not). But LVM autoscan is used in libguestfs (and by layered
utilities) in several places:

  https://libguestfs.org/guestfs.3.html#guestfs_lvm_scan

and now I'm thinking that, if "/etc/lvm/devices/system.devices" is
present on the filesystem under inspection (regardless of origin -- the
disk image could belong to an otherwise permanent, just offline,
domain), then that file could summarily break libguestfs's invocation of
LVM rescan. Consider:

  https://bugzilla.redhat.com/show_bug.cgi?id=1749513#c0

> The old assumption that any storage attached to a machine is intended
> for that machine to use is no longer true.

This assumption certainly remains true for libguestfs, as far as I can
tell.

  https://bugzilla.redhat.com/show_bug.cgi?id=1749513#c27

> When LVM PVs are attached to a system, lvm will not see and use them
> until the user explicitly adds them to the devices file (with
> lvmdevices --adddev or vgimportdevices).  This is likely to be a
> source of confusion for many users (who are not aware of this
> feature), when they attach new devices to a system and lvm commands to
> not recognize the devices.

Yes, this is exactly what I expected. I think it will break libguestfs.

Note that, in guestfs_lvm_scan() -> do_lvm_scan(), we perform "lvm
pvscan --cache". We pass "--cache" specifically in order to force a
rescan (ignoring and updating the cache kept by lvmetad). Additionally,
dependent on the parameters passed to guestfs_lvm_scan(), we also do
auto-activation ("--activate ay"), which *requires* passing "--cache".

I suspect we'll need LVM to offer a command line switch for disregarding
the new "/etc/lvm/devices/system.devices" file (not deleting it -- just
ignoring it), and then we'll need to pass that flag to "lvm pvscan" in
libguestfs.

... Another spot where we use "lvm pvscan --cache" is the
"appliance/init" file; that location will have to be modified as well, I
believe.

David: can you please tell us what the LVM cmdline option is, for
ignoring the "/etc/lvm/devices/system.devices" file? Thank you.

(NB what I'm saying here is that we'll likely have to clone this BZ for
libguestfs, for the above reasons -- independently, I do agree that we
should add such a removal operation to virt-sysprep. It's likely going
to be similar to the "net-nmconn" operation, which I added recently.)

Rich, any comments? Thanks!

(In reply to Laszlo Ersek from comment #2)

> David: can you please tell us what the LVM cmdline option is, for
> ignoring the "/etc/lvm/devices/system.devices" file? Thank you.

Forgot to set needinfo for this; doing it now.

(In reply to Laszlo Ersek from comment #2)
> Note that, in guestfs_lvm_scan() -> do_lvm_scan(), we perform "lvm
> pvscan --cache". We pass "--cache" specifically in order to force a
> rescan (ignoring and updating the cache kept by lvmetad). Additionally,
> dependent on the parameters passed to guestfs_lvm_scan(), we also do
> auto-activation ("--activate ay"), which *requires* passing "--cache".
> 
> I suspect we'll need LVM to offer a command line switch for disregarding
> the new "/etc/lvm/devices/system.devices" file (not deleting it -- just
> ignoring it), and then we'll need to pass that flag to "lvm pvscan" in
> libguestfs.
> 
> ... Another spot where we use "lvm pvscan --cache" is the
> "appliance/init" file; that location will have to be modified as well, I
> believe.

Regarding these lvm commands run by the library calls, I'm having some trouble understanding how that works and what might need to change, e.g. which instance of lvm is running using which lvm config files.

> David: can you please tell us what the LVM cmdline option is, for
> ignoring the "/etc/lvm/devices/system.devices" file? Thank you.

Specify an empty devices file on the command line: --devicesfile "".  That's equivalent to disabling the devices file entirely by doing rm /etc/lvm/devices/system.devices.  Either of those allows lvm to look at every device on the system.

> (NB what I'm saying here is that we'll likely have to clone this BZ for
> libguestfs, for the above reasons -- independently, I do agree that we
> should add such a removal operation to virt-sysprep. It's likely going
> to be similar to the "net-nmconn" operation, which I added recently.)

Thanks for looking at this.  Do you know of other programs that are used to do similar things that we should also look at?

Thanks, David!

Unfortunately, I don't know what other programs might behave like libguestfs.

The relevant upstream lvm2 commit seems to be 83fe6e720f42 ("device usage based on devices file", 2021-02-23). First released as part of v2_03_12. Does not seem to be available in Fedora yet, but it's available in RHEL-8.5 and RHEL-9 dev builds, I think.

Now, it seems that multiple LVM commands can take --devicesfile. In fact it seems to be a common option, parsed (but perhaps not used) by *all* LVM subcommands. I think we can detect whether --devicesfile is supported (or put differently: whether we *must* pass it on the command line) by first invoking:

lvm help --help

and if the output contains "--devicesfile", then we need to append '--devicesfile ""' to every lvm command libguestfs issues.

I've tried to round up the "lvm" invocations in libguestfs like this:

$ git grep -E -l '"lvm\>'

Four files are affected:

daemon/lvm-filter.c
daemon/lvm.c
daemon/lvm.ml
generator/daemon.ml

Right now I can't think of a common spot for *all* of these to detect "--devicesfile" availability, from "lvm help --help". Perhaps add a C-language implementation to "lvm.c", expose it via "daemon.h", and call it from all the C-language source files, including the one(s) generated by "generator/daemon.ml". Regarding "daemon/lvm.ml", just mimic "lvs_has_S_opt" (as a simplification: --devicesfile will never be available is -S is absent; see the related LVM release numbers).

... Then I think we'll have to refactor all "lvm" invocations first, from the stdarg-style command() function, to the array-style commandv(). Because that's how we can ADD_ARG() "--devicesfile" and "\"\"" everywhere. :/

I think I've seriously confused myself here. I mixed up the *guest* filesystem with the appliance *root* filesystem. How lvm behaves is dictated by the latter!

However, I don't understand how the appliance root is "hidden" (so that, for example, one can mount a block device on "/" in guestfish later on). Notably, with virt-rescue, the appliance root does remain exposed as /, and the guest filesystem is available under /sysroot (IIUC).

Rich, can you please explain this to me? How is the appliance root hidden in normal libguestfs operation?

I suspect there's a tricky chroot() somewhere (possibly involved in every libguestfs API that results in a utility being executed from the appliance root, but then operating on the guest filesystems).

Thanks.

Anyway, my main point here is that we need not modify all "lvm" invocations in libguestfs. We just need to delete, in the "appliance/init" script, the "/etc/lvm/devices/system.devices" file (see comment 4). (This file We already have similar tricks in "appliance/init": see e.g. commit 807433bc2309 ("appliance: init: generate /etc/machine-id", 2016-03-30).

In other words, there are now two examples where virt-sysprep *and* "appliance/init" are supposed to perform similar cleanups. For machine-id, refer to the above commit, and to the corresponding virt-sysprep BZ 1554546. For the "lvm2 devices file", the present BZ is for virt-sysprep, and we need to reflect the same logic to "appliance/init".

... Good news (I think) for libguestfs: we have the following snippet in
"appliance/init" already, from commit dd162d2cd56a ("daemon: Move
lvmetad to early in the appliance boot process.", 2018-05-29):

> # Set up a clean LVM environment.
> # Empty LVM configuration file means "all defaults".
> mkdir -p /tmp/lvm
> touch /tmp/lvm/lvm.conf
> LVM_SYSTEM_DIR=/tmp/lvm
> export LVM_SYSTEM_DIR
> lvmetad

Now, "all defaults" will very soon mean (see comment#0)
"use_devicesfile=1".

However, per comment 4, if we make sure that no
"$LVM_SYSTEM_DIR/devices/system.devices" exists, we're good (in
libguestfs anyway).

And the above snippet already covers that -- we need not delete the
file, as the LVM system dir is brand new, and we never create the file
in the first place. So I think no patch or BZ is needed for libguestfs.

[guestfs-tools PATCH] sysprep: remove lvm2's default "system.devices" file
Message-Id: <20220410113834.6258-1-lersek>
https://listman.redhat.com/archives/libguestfs/2022-April/028633.html

There seem to be two separate bugs described here.  The first one is a request
for virt-sysprep to remove everything(?) under /etc/lvm/devices/ in the guest
filesystem.  Actually it's not clear if we should remove everything or just
/etc/lvm/devices/system.devices.  (For me on every system I've looked at the
directory is completely empty).

The second bug is a question about whether libguestfs will be affected if
either the host or guest filesystem contains /etc/lvm/devices/system.devices,
and I think the answer to that is simply no.  Supermin copies files from the
host filesystem to construct the appliance, but it always substitutes
pristine configuration files:

$ zcat /usr/lib64/guestfs/supermin.d/base.tar.gz | tar tf - | grep /etc/lvm
./etc/lvm/lvm.conf
./etc/lvm/lvmlocal.conf

This is because obviously you wouldn't want libguestfs's internal operations
to be affected by configuration changes made on the host.

Furthermore libguestfs itself constructs a new LVM configuration at
runtime (inside the appliance at /tmp/lvm) so that it can modify various
LVM settings.  It doesn't seem as if we need to use --devicesfile either.

> However, I don't understand how the appliance root is "hidden" (so that, for
> example, one can mount a block device on "/" in guestfish later on). Notably,
> with virt-rescue, the appliance root does remain exposed as /, and the guest
> filesystem is available under /sysroot (IIUC).

If you mount something under / in guestfish it's actually mounted under
/sysroot in the appliance.

(In reply to Richard W.M. Jones from comment #10)
> There seem to be two separate bugs described here.  The first one is a
> request
> for virt-sysprep to remove everything(?) under /etc/lvm/devices/ in the guest
> filesystem.  Actually it's not clear if we should remove everything or just
> /etc/lvm/devices/system.devices.  (For me on every system I've looked at the
> directory is completely empty).

It's about the specific file; both upstream lvm2 commit 83fe6e720f42 [1], and comment 4 here [2] make that clear.

[1] "If the file does not exist [...] then lvm will not use a devices file"
[2] "disabling the devices file entirely by doing rm /etc/lvm/devices/system.devices"

> The second bug is a question about whether libguestfs will be affected if
> either the host or guest filesystem contains /etc/lvm/devices/system.devices,
> and I think the answer to that is simply no.  Supermin copies files from the
> host filesystem to construct the appliance, but it always substitutes
> pristine configuration files:
> 
> $ zcat /usr/lib64/guestfs/supermin.d/base.tar.gz | tar tf - | grep /etc/lvm
> ./etc/lvm/lvm.conf
> ./etc/lvm/lvmlocal.conf
> 
> This is because obviously you wouldn't want libguestfs's internal operations
> to be affected by configuration changes made on the host.
> 
> Furthermore libguestfs itself constructs a new LVM configuration at
> runtime (inside the appliance at /tmp/lvm) so that it can modify various
> LVM settings.  It doesn't seem as if we need to use --devicesfile either.

Yep, I saw that in "appliance/init".

> > However, I don't understand how the appliance root is "hidden" (so that, for
> > example, one can mount a block device on "/" in guestfish later on). Notably,
> > with virt-rescue, the appliance root does remain exposed as /, and the guest
> > filesystem is available under /sysroot (IIUC).
> 
> If you mount something under / in guestfish it's actually mounted under
> /sysroot in the appliance.

Thank you.

(In reply to Laszlo Ersek from comment #9)
> [guestfs-tools PATCH] sysprep: remove lvm2's default "system.devices" file
> Message-Id: <20220410113834.6258-1-lersek>
> https://listman.redhat.com/archives/libguestfs/2022-April/028633.html

Upstream commit 4fe8a03cd2d3.

Tested with guestfs-tools-1.48.1-1.el9.x86_64:

Steps:

1. In rhel9.0 guest:
# vgimportdevices -a
  Added 1 devices to devices file.

# cat /etc/lvm/devices/system.devices
# LVM uses devices listed in this file.
# Created by LVM command vgimportdevices pid 1836 at Mon May 16 12:29:15 2022
VERSION=1.1.1
IDTYPE=sys_wwid IDNAME=t10.ATA     QEMU HARDDISK                           QM00001              DEVNAME=/dev/sda2 PVID=uz7LWPzbXurkj0rkWy2cquoGRFRx4Lkw PART=2

2. On rhel9.1 host:
# virt-sysprep -a rhel9.0.qcow2
[   0.0] Examining the guest ...
[   3.5] Performing "abrt-data" ...
[   3.5] Performing "backup-files" ...
[   3.9] Performing "bash-history" ...
[   3.9] Performing "blkid-tab" ...
[   4.0] Performing "crash-data" ...
[   4.0] Performing "cron-spool" ...
[   4.0] Performing "dhcp-client-state" ...
[   4.0] Performing "dhcp-server-state" ...
[   4.0] Performing "dovecot-data" ...
[   4.0] Performing "ipa-client" ...
[   4.0] Performing "kerberos-hostkeytab" ...
[   4.0] Performing "logfiles" ...
[   4.2] Performing "lvm-system-devices" ...
...
[   4.6] SELinux relabelling
[  18.1] Performing "lvm-uuids" ...

3.
# virt-cat -a rhel9.0.qcow2 /etc/lvm/devices/system.devices
libguestfs: error: download: /etc/lvm/devices/system.devices: No such file or directory

The /etc/lvm/devices/system.device file has been deleted by virt-sysprep.

Verified this bug since the test case for this bug has been automated and passed in the latest nightly compose test.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (Low: guestfs-tools security, bug fix, and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2022:7959