Bug 1794518 - setfiles fails on file /etc/.ip with ext4 immutable bit set
Summary: setfiles fails on file /etc/.ip with ext4 immutable bit set
Keywords:
Status: NEW
Alias: None
Product: Red Hat Enterprise Linux 8
Classification: Red Hat
Component: libguestfs
Version: 8.4
Hardware: Unspecified
OS: Linux
medium
unspecified
Target Milestone: rc
: 8.0
Assignee: Virtualization Maintenance
QA Contact: Virtualization Bugs
URL:
Whiteboard: V2V
Depends On:
Blocks: TRACKER-bugs-affecting-libguestfs
TreeView+ depends on / blocked
 
Reported: 2020-01-23 18:14 UTC by Mark Zealey
Modified: 2021-07-23 08:07 UTC (History)
13 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-07-23 07:29:26 UTC
Type: Bug
Target Upstream Version:


Attachments (Terms of Use)

Description Mark Zealey 2020-01-23 18:14:26 UTC
Description of problem:

Running virt-v2v on an image in centos8

LIBGUESTFS_BACKEND=direct virt-v2v -v -x  -i disk /mnt/tmp1/img.vhdx -o local -of qcow2 -os file.qcow -on 0 

...
[  49.5] Converting CentOS Linux release 7.7.1908 (Core) to run on KVM
...

Last few lines as it breaks:

libguestfs: trace: v2v: selinux_relabel "/etc/selinux/targeted/contexts/files/file_contexts" "/" "force:true"
guestfsd: => grep (0x97) took 0.00 secs
guestfsd: <= selinux_relabel (0x1d3) request length 108 bytes
commandrvf: stdout=n stderr=y flags=0x0
commandrvf: setfiles -m
usage:  setfiles [-diIDlmnpqvFW] [-e excludedir] [-r alt_root_path] spec_file pathname...
usage:  setfiles [-diIDlmnpqvFW] [-e excludedir] [-r alt_root_path] spec_file -f filename
usage:  setfiles -s [-diIDlmnpqvFW] spec_file
usage:  setfiles -c policyfile spec_file
commandrvf: stdout=n stderr=y flags=0x0
commandrvf: setfiles -F -e /sysroot/dev -e /sysroot/proc -e /sysroot/selinux -e /sysroot/sys -m -r /sysroot -v /sysroot/etc/selinux/targeted/contexts/files/file_contexts /sysroot/
Can't stat exclude path "/sysroot/selinux", No such file or directory - ignoring.
setfiles: Could not set context for /sysroot/quota.user:  Operation not permitted
guestfsd: error: Can't stat exclude path "/sysroot/selinux", No such file or directory - ignoring.
setfiles: Could not set context for /sysroot/quota.user:  Operation not permitted
guestfsd: => selinux_relabel (0x1d3) took 0.31 secs
libguestfs: trace: v2v: selinux_relabel = -1 (error)
virt-v2v: error: libguestfs error: selinux_relabel: Can't stat exclude path 
"/sysroot/selinux", No such file or directory - ignoring.
setfiles: Could not set context for /sysroot/quota.user:  Operation not 
permitted


Version-Release number of selected component (if applicable):

virt-v2v-1.38.4-14.module_el8.1.0+248+298dec18.x86_64

How reproducible:

All the time on this image

Comment 1 Mark Zealey 2020-01-23 18:22:02 UTC
However on centos 7 with libguestfs-1.40.2-5.el7_7.2.x86_64 from rhev the conversion succeeds

Comment 2 Richard W.M. Jones 2020-01-23 18:37:20 UTC
I would really like to see the virt-v2v -v -x full output.  You can attach
it here, or if it contains sensitive data then email it to me directly.

Can you also see if this fixes the problem:

export LIBGUESTFS_MEMSIZE=4096

Comment 3 Richard W.M. Jones 2020-01-24 09:08:23 UTC
Thanks for sending me the full log.  As far as I can tell this really
is a bug in virt-v2v.  The setfiles command that we run when doing selinux
relabelling at the end fails if quotas are enabled, because the /quota.user
file is somehow "special" and setfiles chokes on it.

Comment 4 Richard W.M. Jones 2020-01-24 09:27:55 UTC
This was my attempt to reproduce the problem, but I cannot get it to fail.

-----
I had to use CentOS 6 to reproduce this because it requires an ext4 root filesystem.

$ virt-builder centos-6 --root-password password:123456
$ qemu-system-x86_64 -machine accel=kvm:tcg -cpu host -m 2048 -drive file=centos-6.img,format=raw,if=virtio

Inside the guest, enable quotas as described in this page:
https://wiki.archlinux.org/index.php/Disk_quota
This will involve rebooting the guest at least once.

Ensure after doing this you have a file called /quota.user or /aquota.user
and then power off the guest.

Convert the guest by doing:

$ virt-v2v -i disk centos-6.img -o null

Expected this step to fail at setfiles, but in fact it works fine.

Comment 6 Mark Zealey 2020-10-29 19:00:02 UTC
As an update on this (coming back to this project after a while). A centos 7 docker container running virt-v2v 1.40.2rhel=7,release=9.el7_8.1,libvirt performs the conversion fine, a centos 8 container running virt-v2v 1.40.2rhel=8,release=25.module_el8.4.0+525+256c136b,libvirt has these selinux issues. I will try to produce a test image that I can send to you.

Comment 7 Richard W.M. Jones 2021-01-05 09:58:00 UTC
Using the disk image supplied by the reporter I found the error is:

commandrvf: setfiles -F -e /sysroot/dev -e /sysroot/proc -e /sysroot/selinux -e /sysroot/sys -m -r /sysroot -v /sysroot/etc/selinux/targeted/contexts/files/file_contexts /sysroot/
setfiles: Could not set context for /sysroot/etc/.ip:  Operation not permitted
guestfsd: error: setfiles: Could not set context for /sysroot/etc/.ip:  Operation not permitted

Inside the guest:

$ ls -l /etc/.ip 
-rwSr-sr-x 1 root root 4648 Nov 12  2018 /etc/.ip
$ stat /etc/.ip
  File: `/etc/.ip'
  Size: 4648      	Blocks: 16         IO Block: 4096   regular file
Device: fd00h/64768d	Inode: 20031       Links: 1
Access: (6655/-rwSr-sr-x)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2020-11-18 07:46:09.159073077 +0200
Modify: 2018-11-12 03:16:34.316119563 +0200
Change: 2019-06-29 07:41:53.116183079 +0200

(Note that /quota.user and /quota.group also exist in this guest.
They may also be causing problems for setfiles, but not in the
conversion which I did)

Comment 8 Richard W.M. Jones 2021-01-05 10:34:25 UTC
I tried adding -e /etc/.ip to the setfiles command but it didn't
make a difference.

Comment 9 Mark Zealey 2021-01-05 10:36:36 UTC
yes it's affecting a number of different files on different images this was just an example..

Comment 10 Richard W.M. Jones 2021-01-05 10:40:50 UTC
lgetxattr("/sysroot/etc/.ip", "security.selinux", 0x56394efa0ff0, 255) = -1 ENODATA (No data available)
access("/var/run/setrans/.setrans-unix", F_OK) = -1 ENOENT (No such file or directory)
futex(0x7ffa050f15d0, FUTEX_WAKE_PRIVATE, 2147483647) = 0
lsetxattr("/sysroot/etc/.ip", "security.selinux", "system_u:object_r:etc_t:s0", 27, 0) = -1 EPERM (Operation not permitted)
write(2, "setfiles: ", 10)              = 10
write(2, "Could not set context for /sysro"..., 69) = 69

This is really a bug in setfiles.  It should either be able to relabel this
file or provide a way to ignore these errors.

Comment 11 Petr Lautrbach 2021-01-06 18:08:31 UTC
I guess we could add new 'ignore errors' option to setfiles and do not set SELINUX_RESTORECON_ABORT_ON_ERROR flag.

But what is special on /sysroot/etc/.ip so that setfiles can't set the extended  attribute?

    	int rc = lsetxattr(path, XATTR_NAME_SELINUX, context, strlen(context) + 1, 0);

It's not only the file mode, is there any acl in game?

Comment 12 Richard W.M. Jones 2021-01-06 18:31:26 UTC
The bug reporter supplied me with the disk image (I'm afraid much too large
to easily share) so I can run commands on it.  To look for ACLs I used the
getfacl command:

# getfacl /sysroot/etc/.ip 
getfacl: Removing leading '/' from absolute path names
# file: sysroot/etc/.ip
# owner: root
# group: root
# flags: ss-
user::rw-
group::r-x
other::r-x

However you did set me on the right track, because I wondered if the
problem could be an ext4 attr, and indeed:

# lsattr /sysroot/etc/.ip 
----i---------e----- /sysroot/etc/.ip

So the file has the i (immutable) attribute.  ('e' means the file
can use ext4 extents which is not important here).

I wonder if we can ignore (or warn) on files with the immutable bit
set?  It seems wrong to fail setfiles completely.

Comment 13 Petr Lautrbach 2021-01-06 20:27:03 UTC
Apparently there's behaviour change between rhel7 and rhel8. In rhel7 such file would be skipped while the new implementation aborts on it.

At this moment I'd blame https://github.com/SELinuxProject/selinux/commit/602347c7422e971a5674fe2767267a96e3b4f61c#diff-81dd45592a225693b30ab27df56bf9d298238035cf86e8fa930e20d25c8902e7

Originally there were 'goto skip' and 'return SKIP' when lsetfilecon() failed:

	ret = lsetfilecon(ftsent->fts_accpath, newcon);
	if (ret) {
        	fprintf(stderr, "%s set context %s->%s failed:'%s'\n",
			r_opts->progname, my_file, newcon, strerror(errno));
		goto skip;
	}
	ret = 0;


But the new implementation uses selinux_restorecon() with SELINUX_RESTORECON_ABORT_ON_ERROR flag which aborts immediately after lsetfilecon() fails.

We could fix it simply stop using SELINUX_RESTORECON_ABORT_ON_ERROR. But given the change happened 4 years ago, I'd like to discuss it with upstream first.

Comment 17 Richard W.M. Jones 2021-01-15 09:45:54 UTC
Patch looks good here, thanks.

Comment 21 Richard W.M. Jones 2021-02-08 09:53:52 UTC
I tested policycoreutils-2.9-12.el8 and the original disk image supplied
by the reporter.  I see a series of errors like:

virt-v2v: error: libguestfs error: selinux_relabel: setfiles: Could not set context for /sysroot/etc/.ip: Operation not permitted
setfiles: Could not set context for /sysroot/etc/ssh/sshd_config:  Operation not permitted
setfiles: Could not set context for /sysroot/etc/exim.conf:  Operation not permitted
[...]

Previously only the first error was printed.

Unfortunately after that the conversion still fails.

Do we need to change the setfiles command in some way in order to get it to
ignore these errors?

Comment 22 Richard W.M. Jones 2021-02-08 10:03:42 UTC
Here is a simple(r) reproducer for this bug:

$ virt-builder fedora-30
$ guestfish -a fedora-30.img -i removexattr security.selinux /etc/passwd : removexattr security.selinux /etc/group : set-e2attrs /etc/passwd i : set-e2attrs /etc/group i
$ virt-v2v -i disk fedora-30.img -o null

Comment 23 Petr Lautrbach 2021-02-08 13:43:33 UTC
You see multiple errors as there are more files which can't be labelled and setfiles try to relabel them all, setfiles then fails as there were errors during the relabel. I'd consider this correct.

But I guess we could use different error value for different errors. It's -1 for everything now, but we could change it to use 1 as a generic error and 2 as a value for processing/relabeling problem. You would need to check the error value, if it's 2 labelling happened but some files could  be still using wrong label.

Comment 24 Richard W.M. Jones 2021-02-08 13:53:19 UTC
The problem is we're doing this operation on N-thousand VMs at once
(while doing v2v conversions), and we don't really want to stop because
someone set an immutable bit on a file.  There's no good answer here,
but some way to ignore certain classes of errors would be welcome.  How
about some sort of "--ignore class1,class2,..." flag where certain types
of errors would be ignored while still failing on other kinds of errors?

Comment 25 Petr Lautrbach 2021-02-08 17:21:19 UTC
I'm afraid it's not possible with the current code base. setfiles uses selinux_restorecon(3) which returns 0 on succes, -1 on error. In the change attached to this bug we've dropped SELINUX_RESTORECON_ABORT_ON_ERROR flag so that selinux_restorecon() doesn't abort on labeling error but it still returns -1.

I guess you could warn users when setfiles fails. Or you could implement the relabel on your own. Bellow is an example with minimal working code with hardcoded values:

#include <selinux/restorecon.h>
#include <selinux/selinux.h>
#include <selinux/label.h>

int main(int argc, char **argv) {
    struct selinux_opt selinux_opts[] = {
        { SELABEL_OPT_VALIDATE, (char *)1 },
        { SELABEL_OPT_PATH, "/sysroot/etc/selinux/targeted/contexts/files/file_contexts" },
        { SELABEL_OPT_DIGEST, (char *)1 }
    };
    unsigned int flags = SELINUX_RESTORECON_RECURSE |
        SELINUX_RESTORECON_LOG_MATCHES | SELINUX_RESTORECON_VERBOSE;
    struct selabel_handle *hndl;
    char *exclude_list[] = {
      "/sysroot/dev", "/sysroot/proc", "/sysroot/selinux", "/sysroot/sys", 0
    };
    int rc;

    hndl = selabel_open(SELABEL_CTX_FILE, selinux_opts, 3);

    selinux_restorecon_set_sehandle(hndl);
    selinux_restorecon_set_alt_rootpath("/sysroot");
    selinux_restorecon_set_exclude_list((const char **)exclude_list);
    rc = selinux_restorecon("/sysroot", flags);
    return (rc == -1 ? 1 : 0);
}

Comment 26 Richard W.M. Jones 2021-02-08 17:24:21 UTC
I'm going to have to think about this.  The current change is
still an improvement, so it would be nice to have this in RHEL.

If you like however you can move this bug back to component "libguestfs".

Comment 27 Petr Lautrbach 2021-02-08 18:57:17 UTC
policycoreutils bug is cloned at https://bugzilla.redhat.com/show_bug.cgi?id=1926386

Comment 31 RHEL Program Management 2021-07-23 07:29:26 UTC
After evaluating this issue, there are no plans to address it further or fix it in an upcoming release.  Therefore, it is being closed.  If plans change such that this issue will be fixed in an upcoming release, then the bug can be reopened.

Comment 32 Richard W.M. Jones 2021-07-23 08:07:38 UTC
I'm sorry this bug was closed erroneously by an automated process that we
do not have control over.  Reopening.


Note You need to log in before you can comment on or make changes to this bug.