From 1f546b709d6121d1a3e629e482ba14fb7bf10ce2 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 15 Jun 2021 13:55:23 -0400 Subject: tests: migration-test: Add dirty ring test Add dirty ring test if kernel supports it. Add the dirty ring parameter on source should be mostly enough, but let's change the dest too to make them match always. Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Peter Xu Message-Id: <20210615175523.439830-3-peterx@redhat.com> Reviewed-by: Juan Quintela Signed-off-by: Dr. David Alan Gilbert --- tests/qtest/migration-test.c | 58 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 2b028df..328d6db 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -27,6 +27,10 @@ #include "migration-helpers.h" #include "tests/migration/migration-test.h" +#if defined(__linux__) +#include "linux/kvm.h" +#endif + /* TODO actually test the results and get rid of this */ #define qtest_qmp_discard_response(...) qobject_unref(qtest_qmp(__VA_ARGS__)) @@ -467,6 +471,8 @@ typedef struct { bool use_shmem; /* only launch the target process */ bool only_target; + /* Use dirty ring if true; dirty logging otherwise */ + bool use_dirty_ring; char *opts_source; char *opts_target; } MigrateStart; @@ -573,11 +579,13 @@ static int test_migrate_start(QTestState **from, QTestState **to, shmem_opts = g_strdup(""); } - cmd_source = g_strdup_printf("-accel kvm -accel tcg%s%s " + cmd_source = g_strdup_printf("-accel kvm%s -accel tcg%s%s " "-name source,debug-threads=on " "-m %s " "-serial file:%s/src_serial " "%s %s %s %s", + args->use_dirty_ring ? + ",dirty-ring-size=4096" : "", machine_opts ? " -machine " : "", machine_opts ? machine_opts : "", memory_size, tmpfs, @@ -587,12 +595,14 @@ static int test_migrate_start(QTestState **from, QTestState **to, *from = qtest_init(cmd_source); } - cmd_target = g_strdup_printf("-accel kvm -accel tcg%s%s " + cmd_target = g_strdup_printf("-accel kvm%s -accel tcg%s%s " "-name target,debug-threads=on " "-m %s " "-serial file:%s/dest_serial " "-incoming %s " "%s %s %s %s", + args->use_dirty_ring ? + ",dirty-ring-size=4096" : "", machine_opts ? " -machine " : "", machine_opts ? machine_opts : "", memory_size, tmpfs, uri, @@ -785,12 +795,14 @@ static void test_baddest(void) test_migrate_end(from, to, false); } -static void test_precopy_unix(void) +static void test_precopy_unix_common(bool dirty_ring) { g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); MigrateStart *args = migrate_start_new(); QTestState *from, *to; + args->use_dirty_ring = dirty_ring; + if (test_migrate_start(&from, &to, uri, args)) { return; } @@ -825,6 +837,18 @@ static void test_precopy_unix(void) test_migrate_end(from, to, true); } +static void test_precopy_unix(void) +{ + /* Using default dirty logging */ + test_precopy_unix_common(false); +} + +static void test_precopy_unix_dirty_ring(void) +{ + /* Using dirty ring tracking */ + test_precopy_unix_common(true); +} + #if 0 /* Currently upset on aarch64 TCG */ static void test_ignore_shared(void) @@ -1369,6 +1393,29 @@ static void test_multifd_tcp_cancel(void) test_migrate_end(from, to2, true); } +static bool kvm_dirty_ring_supported(void) +{ +#if defined(__linux__) + int ret, kvm_fd = open("/dev/kvm", O_RDONLY); + + if (kvm_fd < 0) { + return false; + } + + ret = ioctl(kvm_fd, KVM_CHECK_EXTENSION, KVM_CAP_DIRTY_LOG_RING); + close(kvm_fd); + + /* We test with 4096 slots */ + if (ret < 4096) { + return false; + } + + return true; +#else + return false; +#endif +} + int main(int argc, char **argv) { char template[] = "/tmp/migration-test-XXXXXX"; @@ -1439,6 +1486,11 @@ int main(int argc, char **argv) qtest_add_func("/migration/multifd/tcp/zstd", test_multifd_tcp_zstd); #endif + if (kvm_dirty_ring_supported()) { + qtest_add_func("/migration/dirty_ring", + test_precopy_unix_dirty_ring); + } + ret = g_test_run(); g_assert_cmpint(ret, ==, 0); -- cgit v1.1 From c00d434ac6f202b1e81aa52f3385bd9c22ac1599 Mon Sep 17 00:00:00 2001 From: Feng Lin Date: Fri, 25 Jun 2021 14:21:38 +0800 Subject: migration: fix the memory overwriting risk in add_to_iovec When testing migration, a Segmentation fault qemu core is generated. 0 error_free (err=0x1) 1 0x00007f8b862df647 in qemu_fclose (f=f@entry=0x55e06c247640) 2 0x00007f8b8516d59a in migrate_fd_cleanup (s=s@entry=0x55e06c0e1ef0) 3 0x00007f8b8516d66c in migrate_fd_cleanup_bh (opaque=0x55e06c0e1ef0) 4 0x00007f8b8626a47f in aio_bh_poll (ctx=ctx@entry=0x55e06b5a16d0) 5 0x00007f8b8626e71f in aio_dispatch (ctx=0x55e06b5a16d0) 6 0x00007f8b8626a33d in aio_ctx_dispatch (source=, callback=, user_data=) 7 0x00007f8b866bdba4 in g_main_context_dispatch () 8 0x00007f8b8626cde9 in glib_pollfds_poll () 9 0x00007f8b8626ce62 in os_host_main_loop_wait (timeout=) 10 0x00007f8b8626cffd in main_loop_wait (nonblocking=nonblocking@entry=0) 11 0x00007f8b862ef01f in main_loop () Using gdb print the struct QEMUFile f = { ..., iovcnt = 65, last_error = 21984, last_error_obj = 0x1, shutdown = true } Well iovcnt is overflow, because the max size of MAX_IOV_SIZE is 64. struct QEMUFile { ...; struct iovec iov[MAX_IOV_SIZE]; unsigned int iovcnt; int last_error; Error *last_error_obj; bool shutdown; }; iovcnt and last_error is overwrited by add_to_iovec(). Right now, add_to_iovec() increase iovcnt before check the limit. And it seems that add_to_iovec() assumes that iovcnt will set to zero in qemu_fflush(). But qemu_fflush() will directly return when f->shutdown is true. The situation may occur when libvirtd restart during migration, after f->shutdown is set, before calling qemu_file_set_error() in qemu_file_shutdown(). So the safiest way is checking the iovcnt before increasing it. Signed-off-by: Feng Lin Message-Id: <20210625062138.1899-1-linfeng23@huawei.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert Fix typo in 'writeable' which is actually misnamed 'writable' --- migration/qemu-file.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/migration/qemu-file.c b/migration/qemu-file.c index d6e03db..1eacf9e 100644 --- a/migration/qemu-file.c +++ b/migration/qemu-file.c @@ -416,6 +416,11 @@ static int add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size, { f->iov[f->iovcnt - 1].iov_len += size; } else { + if (f->iovcnt >= MAX_IOV_SIZE) { + /* Should only happen if a previous fflush failed */ + assert(f->shutdown || !qemu_file_is_writable(f)); + return 1; + } if (may_free) { set_bit(f->iovcnt, f->may_free); } -- cgit v1.1 From cc48c587d25ff5dd7dddb4e5072de9ca8464c832 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 29 Jun 2021 14:13:55 -0400 Subject: migration: Move yank outside qemu_start_incoming_migration() Starting from commit b5eea99ec2f5c, qmp_migrate_recover() calls unregister before calling qemu_start_incoming_migration(). I believe it wanted to mitigate the next call to yank_register_instance(), but I think that's wrong. Firstly, if during recover, we should keep the yank instance there, not "quickly removing and adding it back". Meanwhile, calling qmp_migrate_recover() twice with b5eea99ec2f5c will directly crash the dest qemu (right now it can't; but it'll start to work right after the next patch) because the 1st call of qmp_migrate_recover() will unregister permanently when the channel failed to establish, then the 2nd call of qmp_migrate_recover() crashes at yank_unregister_instance(). This patch fixes it by moving yank ops out of qemu_start_incoming_migration() into qmp_migrate_incoming. For qmp_migrate_recover(), drop the unregister of yank instance too since we keep it there during the recovery phase. Signed-off-by: Peter Xu Reviewed-by: Dr. David Alan Gilbert Message-Id: <20210629181356.217312-2-peterx@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- migration/migration.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/migration/migration.c b/migration/migration.c index 4228635..1bb03d1 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -456,10 +456,6 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp) { const char *p = NULL; - if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) { - return; - } - qapi_event_send_migration(MIGRATION_STATUS_SETUP); if (strstart(uri, "tcp:", &p) || strstart(uri, "unix:", NULL) || @@ -474,7 +470,6 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp) } else if (strstart(uri, "fd:", &p)) { fd_start_incoming_migration(p, errp); } else { - yank_unregister_instance(MIGRATION_YANK_INSTANCE); error_setg(errp, "unknown migration protocol: %s", uri); } } @@ -2083,9 +2078,14 @@ void qmp_migrate_incoming(const char *uri, Error **errp) return; } + if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) { + return; + } + qemu_start_incoming_migration(uri, &local_err); if (local_err) { + yank_unregister_instance(MIGRATION_YANK_INSTANCE); error_propagate(errp, local_err); return; } @@ -2114,7 +2114,6 @@ void qmp_migrate_recover(const char *uri, Error **errp) * only re-setup the migration stream and poke existing migration * to continue using that newly established channel. */ - yank_unregister_instance(MIGRATION_YANK_INSTANCE); qemu_start_incoming_migration(uri, errp); } -- cgit v1.1 From b7f9afd48e7bc5c341e55348f2c2eed08314be7d Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 29 Jun 2021 14:13:56 -0400 Subject: migration: Allow reset of postcopy_recover_triggered when failed It's possible qemu_start_incoming_migration() failed at any point, when it happens we should reset postcopy_recover_triggered to false so that the user can still retry with a saner incoming port. Signed-off-by: Peter Xu Message-Id: <20210629181356.217312-3-peterx@redhat.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- migration/migration.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/migration/migration.c b/migration/migration.c index 1bb03d1..fcca289 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2097,6 +2097,13 @@ void qmp_migrate_recover(const char *uri, Error **errp) { MigrationIncomingState *mis = migration_incoming_get_current(); + /* + * Don't even bother to use ERRP_GUARD() as it _must_ always be set by + * callers (no one should ignore a recover failure); if there is, it's a + * programming error. + */ + assert(errp); + if (mis->state != MIGRATION_STATUS_POSTCOPY_PAUSED) { error_setg(errp, "Migrate recover can only be run " "when postcopy is paused."); @@ -2115,6 +2122,12 @@ void qmp_migrate_recover(const char *uri, Error **errp) * to continue using that newly established channel. */ qemu_start_incoming_migration(uri, errp); + + /* Safe to dereference with the assert above */ + if (*errp) { + /* Reset the flag so user could still retry */ + qatomic_set(&mis->postcopy_recover_triggered, false); + } } void qmp_migrate_pause(Error **errp) -- cgit v1.1 From fde93d99d9c208c07e2dcc24cd04e824d2b65b35 Mon Sep 17 00:00:00 2001 From: Laurent Vivier Date: Tue, 29 Jun 2021 17:50:06 +0200 Subject: migration: move wait-unplug loop to its own function The loop is used in migration_thread() and bg_migration_thread(), so we can move it to its own function and call it from these both places. Moreover, in migration_thread() we have a wrong state transition from SETUP to ACTIVE while state could be WAIT_UNPLUG. This is correctly managed in bg_migration_thread() so use this code instead. Signed-off-by: Laurent Vivier Message-Id: <20210629155007.629086-2-lvivier@redhat.com> Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela Signed-off-by: Dr. David Alan Gilbert --- migration/migration.c | 54 +++++++++++++++++++++++++-------------------------- 1 file changed, 26 insertions(+), 28 deletions(-) diff --git a/migration/migration.c b/migration/migration.c index fcca289..dbc484c 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -3677,6 +3677,28 @@ bool migration_rate_limit(void) } /* + * if failover devices are present, wait they are completely + * unplugged + */ + +static void qemu_savevm_wait_unplug(MigrationState *s, int old_state, + int new_state) +{ + if (qemu_savevm_state_guest_unplug_pending()) { + migrate_set_state(&s->state, old_state, MIGRATION_STATUS_WAIT_UNPLUG); + + while (s->state == MIGRATION_STATUS_WAIT_UNPLUG && + qemu_savevm_state_guest_unplug_pending()) { + qemu_sem_timedwait(&s->wait_unplug_sem, 250); + } + + migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, new_state); + } else { + migrate_set_state(&s->state, old_state, new_state); + } +} + +/* * Master migration thread on the source VM. * It drives the migration and pumps the data down the outgoing channel. */ @@ -3722,22 +3744,10 @@ static void *migration_thread(void *opaque) qemu_savevm_state_setup(s->to_dst_file); - if (qemu_savevm_state_guest_unplug_pending()) { - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, - MIGRATION_STATUS_WAIT_UNPLUG); - - while (s->state == MIGRATION_STATUS_WAIT_UNPLUG && - qemu_savevm_state_guest_unplug_pending()) { - qemu_sem_timedwait(&s->wait_unplug_sem, 250); - } - - migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, - MIGRATION_STATUS_ACTIVE); - } + qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP, + MIGRATION_STATUS_ACTIVE); s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start; - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, - MIGRATION_STATUS_ACTIVE); trace_migration_thread_setup_complete(); @@ -3845,21 +3855,9 @@ static void *bg_migration_thread(void *opaque) qemu_savevm_state_header(s->to_dst_file); qemu_savevm_state_setup(s->to_dst_file); - if (qemu_savevm_state_guest_unplug_pending()) { - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, - MIGRATION_STATUS_WAIT_UNPLUG); - - while (s->state == MIGRATION_STATUS_WAIT_UNPLUG && - qemu_savevm_state_guest_unplug_pending()) { - qemu_sem_timedwait(&s->wait_unplug_sem, 250); - } + qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP, + MIGRATION_STATUS_ACTIVE); - migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, - MIGRATION_STATUS_ACTIVE); - } else { - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP, - MIGRATION_STATUS_ACTIVE); - } s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start; trace_migration_thread_setup_complete(); -- cgit v1.1 From 944bc528421aa848ca218ee535ea923a4147a525 Mon Sep 17 00:00:00 2001 From: Laurent Vivier Date: Tue, 29 Jun 2021 17:50:07 +0200 Subject: migration: failover: continue to wait card unplug on error If the user cancels the migration in the unplug-wait state, QEMU will try to plug back the card and this fails because the card is partially unplugged. To avoid the problem, continue to wait the card unplug, but to allow the migration to be canceled if the card never finishes to unplug use a timeout. Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1976852 Signed-off-by: Laurent Vivier Reviewed-by: Dr. David Alan Gilbert Message-Id: <20210629155007.629086-3-lvivier@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- migration/migration.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/migration/migration.c b/migration/migration.c index dbc484c..5ff7ba9 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -3691,6 +3691,17 @@ static void qemu_savevm_wait_unplug(MigrationState *s, int old_state, qemu_savevm_state_guest_unplug_pending()) { qemu_sem_timedwait(&s->wait_unplug_sem, 250); } + if (s->state != MIGRATION_STATUS_WAIT_UNPLUG) { + int timeout = 120; /* 30 seconds */ + /* + * migration has been canceled + * but as we have started an unplug we must wait the end + * to be able to plug back the card + */ + while (timeout-- && qemu_savevm_state_guest_unplug_pending()) { + qemu_sem_timedwait(&s->wait_unplug_sem, 250); + } + } migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, new_state); } else { -- cgit v1.1 From d9a801f7e9fd18ce96a0bfff73b785f0a1f8e6a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Fri, 11 Jun 2021 17:43:20 +0100 Subject: virtiofsd: use GDateTime for formatting timestamp for debug messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GDateTime APIs provided by GLib avoid portability pitfalls, such as some platforms where 'struct timeval.tv_sec' field is still 'long' instead of 'time_t'. When combined with automatic cleanup, GDateTime often results in simpler code too. Localtime is changed to UTC to avoid the need to grant extra seccomp permissions for GLib's access of the timezone database. Signed-off-by: Daniel P. Berrangé Reviewed-by: Dr. David Alan Gilbert Message-Id: <20210611164319.67762-1-berrange@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- tools/virtiofsd/passthrough_ll.c | 25 ++++--------------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 49c21fd..9858e96 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -3559,10 +3559,6 @@ static void setup_nofile_rlimit(unsigned long rlimit_nofile) static void log_func(enum fuse_log_level level, const char *fmt, va_list ap) { g_autofree char *localfmt = NULL; - struct timespec ts; - struct tm tm; - char sec_fmt[sizeof "2020-12-07 18:17:54"]; - char zone_fmt[sizeof "+0100"]; if (current_log_level < level) { return; @@ -3574,23 +3570,10 @@ static void log_func(enum fuse_log_level level, const char *fmt, va_list ap) localfmt = g_strdup_printf("[ID: %08ld] %s", syscall(__NR_gettid), fmt); } else { - /* try formatting a broken-down timestamp */ - if (clock_gettime(CLOCK_REALTIME, &ts) != -1 && - localtime_r(&ts.tv_sec, &tm) != NULL && - strftime(sec_fmt, sizeof sec_fmt, "%Y-%m-%d %H:%M:%S", - &tm) != 0 && - strftime(zone_fmt, sizeof zone_fmt, "%z", &tm) != 0) { - localfmt = g_strdup_printf("[%s.%02ld%s] [ID: %08ld] %s", - sec_fmt, - ts.tv_nsec / (10L * 1000 * 1000), - zone_fmt, syscall(__NR_gettid), - fmt); - } else { - /* fall back to a flat timestamp */ - localfmt = g_strdup_printf("[%" PRId64 "] [ID: %08ld] %s", - get_clock(), syscall(__NR_gettid), - fmt); - } + g_autoptr(GDateTime) now = g_date_time_new_now_utc(); + g_autofree char *nowstr = g_date_time_format(now, "%Y-%m-%d %H:%M:%S.%f%z"); + localfmt = g_strdup_printf("[%s] [ID: %08ld] %s", + nowstr, syscall(__NR_gettid), fmt); } fmt = localfmt; } -- cgit v1.1 From 3399bca4514b5c8d513a88fa3e472756468cb4c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Fri, 11 Jun 2021 13:04:27 +0100 Subject: docs: describe the security considerations with virtiofsd xattr mapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Different guest xattr prefixes have distinct access control rules applied by the guest. When remapping a guest xattr care must be taken that the remapping does not allow the a guest user to bypass guest kernel access control rules. For example if 'trusted.*' which requires CAP_SYS_ADMIN is remapped to 'user.virtiofs.trusted.*', an unprivileged guest user which can write to 'user.*' can bypass the CAP_SYS_ADMIN control. Thus the target of any remapping must be explicitly blocked from read/writes by the guest, to prevent access control bypass. The examples shown in the virtiofsd man page already do the right thing and ensure safety, but the security implications of getting this wrong were not made explicit. This could lead to host admins and apps unwittingly creating insecure configurations. Signed-off-by: Daniel P. Berrangé Message-Id: <20210611120427.49736-1-berrange@redhat.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- docs/tools/virtiofsd.rst | 55 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 50 insertions(+), 5 deletions(-) diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst index 4911e79..a6c3502 100644 --- a/docs/tools/virtiofsd.rst +++ b/docs/tools/virtiofsd.rst @@ -127,8 +127,8 @@ Options timeout. ``always`` sets a long cache lifetime at the expense of coherency. The default is ``auto``. -xattr-mapping -------------- +Extended attribute (xattr) mapping +---------------------------------- By default the name of xattr's used by the client are passed through to the server file system. This can be a problem where either those xattr names are used @@ -136,6 +136,9 @@ by something on the server (e.g. selinux client/server confusion) or if the virtiofsd is running in a container with restricted privileges where it cannot access some attributes. +Mapping syntax +~~~~~~~~~~~~~~ + A mapping of xattr names can be made using -o xattrmap=mapping where the ``mapping`` string consists of a series of rules. @@ -232,8 +235,48 @@ Note: When the 'security.capability' xattr is remapped, the daemon has to do extra work to remove it during many operations, which the host kernel normally does itself. -xattr-mapping Examples ----------------------- +Security considerations +~~~~~~~~~~~~~~~~~~~~~~~ + +Operating systems typically partition the xattr namespace using +well defined name prefixes. Each partition may have different +access controls applied. For example, on Linux there are multiple +partitions + + * ``system.*`` - access varies depending on attribute & filesystem + * ``security.*`` - only processes with CAP_SYS_ADMIN + * ``trusted.*`` - only processes with CAP_SYS_ADMIN + * ``user.*`` - any process granted by file permissions / ownership + +While other OS such as FreeBSD have different name prefixes +and access control rules. + +When remapping attributes on the host, it is important to +ensure that the remapping does not allow a guest user to +evade the guest access control rules. + +Consider if ``trusted.*`` from the guest was remapped to +``user.virtiofs.trusted*`` in the host. An unprivileged +user in a Linux guest has the ability to write to xattrs +under ``user.*``. Thus the user can evade the access +control restriction on ``trusted.*`` by instead writing +to ``user.virtiofs.trusted.*``. + +As noted above, the partitions used and access controls +applied, will vary across guest OS, so it is not wise to +try to predict what the guest OS will use. + +The simplest way to avoid an insecure configuration is +to remap all xattrs at once, to a given fixed prefix. +This is shown in example (1) below. + +If selectively mapping only a subset of xattr prefixes, +then rules must be added to explicitly block direct +access to the target of the remapping. This is shown +in example (2) below. + +Mapping examples +~~~~~~~~~~~~~~~~ 1) Prefix all attributes with 'user.virtiofs.' @@ -271,7 +314,9 @@ stripping of 'user.virtiofs.'. The second rule hides unprefixed 'trusted.' attributes on the host. The third rule stops a guest from explicitly setting -the 'user.virtiofs.' path directly. +the 'user.virtiofs.' path directly to prevent access +control bypass on the target of the earlier prefix +remapping. Finally, the fourth rule lets all remaining attributes through. -- cgit v1.1 From 1d03e5660711c40f79917054328e0dc6bcb879a3 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 24 Jun 2021 12:18:09 +0200 Subject: virtiofsd: Don't allow file creation with FUSE_OPEN A well behaved FUSE client uses FUSE_CREATE to create files. It isn't supposed to pass O_CREAT along a FUSE_OPEN request, as documented in the "fuse_lowlevel.h" header : /** * Open a file * * Open flags are available in fi->flags. The following rules * apply. * * - Creation (O_CREAT, O_EXCL, O_NOCTTY) flags will be * filtered out / handled by the kernel. But if the client happens to do it anyway, the server ends up passing this flag to open() without the mandatory mode_t 4th argument. Since open() is a variadic function, glibc will happily pass whatever it finds on the stack to the syscall. If this file is compiled with -D_FORTIFY_SOURCE=2, glibc will even detect that and abort: *** invalid openat64 call: O_CREAT or O_TMPFILE without mode ***: terminated Specifying O_CREAT with FUSE_OPEN is a protocol violation. Check this in do_open(), print out a message and return an error to the client, EINVAL like we already do when fuse_mbuf_iter_advance() fails. The FUSE filesystem doesn't currently support O_TMPFILE, but the very same would happen if O_TMPFILE was passed in a FUSE_OPEN request. Check that as well. Signed-off-by: Greg Kurz Message-Id: <20210624101809.48032-1-groug@kaod.org> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- tools/virtiofsd/fuse_lowlevel.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c index 7fe2cef..3d725bc 100644 --- a/tools/virtiofsd/fuse_lowlevel.c +++ b/tools/virtiofsd/fuse_lowlevel.c @@ -1084,6 +1084,12 @@ static void do_open(fuse_req_t req, fuse_ino_t nodeid, return; } + /* File creation is handled by do_create() or do_mknod() */ + if (arg->flags & (O_CREAT | O_TMPFILE)) { + fuse_reply_err(req, EINVAL); + return; + } + memset(&fi, 0, sizeof(fi)); fi.flags = arg->flags; fi.kill_priv = arg->open_flags & FUSE_OPEN_KILL_SUIDGID; -- cgit v1.1 From 061624455fc3e4a19caeba321b284548127d858e Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Tue, 22 Jun 2021 11:08:46 -0400 Subject: virtiofsd: Fix fuse setxattr() API change issue With kernel header updates fuse_setxattr_in struct has grown in size. But this new struct size only takes affect if user has opted in for fuse feature FUSE_SETXATTR_EXT otherwise fuse continues to send "fuse_setxattr_in" of older size. Older size is determined by FUSE_COMPAT_SETXATTR_IN_SIZE. Fix this. If we have not opted in for FUSE_SETXATTR_EXT, then expect that we will get fuse_setxattr_in of size FUSE_COMPAT_SETXATTR_IN_SIZE and not sizeof(struct fuse_sexattr_in). Fixes: 278f064e4524 ("Update Linux headers to 5.13-rc4") Signed-off-by: Vivek Goyal Message-Id: <20210622150852.1507204-2-vgoyal@redhat.com> Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Greg Kurz Signed-off-by: Dr. David Alan Gilbert --- tools/virtiofsd/fuse_common.h | 5 +++++ tools/virtiofsd/fuse_lowlevel.c | 7 ++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/tools/virtiofsd/fuse_common.h b/tools/virtiofsd/fuse_common.h index fa96718..0c2665b 100644 --- a/tools/virtiofsd/fuse_common.h +++ b/tools/virtiofsd/fuse_common.h @@ -373,6 +373,11 @@ struct fuse_file_info { #define FUSE_CAP_HANDLE_KILLPRIV_V2 (1 << 28) /** + * Indicates that file server supports extended struct fuse_setxattr_in + */ +#define FUSE_CAP_SETXATTR_EXT (1 << 29) + +/** * Ioctl flags * * FUSE_IOCTL_COMPAT: 32bit compat ioctl on 64bit machine diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c index 3d725bc..2028677 100644 --- a/tools/virtiofsd/fuse_lowlevel.c +++ b/tools/virtiofsd/fuse_lowlevel.c @@ -1425,8 +1425,13 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid, struct fuse_setxattr_in *arg; const char *name; const char *value; + bool setxattr_ext = req->se->conn.want & FUSE_CAP_SETXATTR_EXT; - arg = fuse_mbuf_iter_advance(iter, sizeof(*arg)); + if (setxattr_ext) { + arg = fuse_mbuf_iter_advance(iter, sizeof(*arg)); + } else { + arg = fuse_mbuf_iter_advance(iter, FUSE_COMPAT_SETXATTR_IN_SIZE); + } name = fuse_mbuf_iter_advance_str(iter); if (!arg || !name) { fuse_reply_err(req, EINVAL); -- cgit v1.1 From 5290fb625d1bf692306ee958efc67c8620866f67 Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Tue, 22 Jun 2021 11:08:47 -0400 Subject: virtiofsd: Fix xattr operations overwriting errno getxattr/setxattr/removexattr/listxattr operations handle regualar and non-regular files differently. For the case of non-regular files we do fchdir(/proc/self/fd) and the xattr operation and then revert back to original working directory. After this we are saving errno and that's buggy because fchdir() will overwrite the errno. FCHDIR_NOFAIL(lo->proc_self_fd); ret = getxattr(procname, name, value, size); FCHDIR_NOFAIL(lo->root.fd); if (ret == -1) saverr = errno In above example, if getxattr() failed, we will still return 0 to caller as errno must have been written by FCHDIR_NOFAIL(lo->root.fd) call. Fix all such instances and capture "errno" early and save in "saverr" variable. Signed-off-by: Vivek Goyal Message-Id: <20210622150852.1507204-3-vgoyal@redhat.com> Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Greg Kurz Signed-off-by: Dr. David Alan Gilbert --- tools/virtiofsd/passthrough_ll.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 9858e96..ccbda98 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -2791,15 +2791,17 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, goto out_err; } ret = fgetxattr(fd, name, value, size); + saverr = ret == -1 ? errno : 0; } else { /* fchdir should not fail here */ FCHDIR_NOFAIL(lo->proc_self_fd); ret = getxattr(procname, name, value, size); + saverr = ret == -1 ? errno : 0; FCHDIR_NOFAIL(lo->root.fd); } if (ret == -1) { - goto out_err; + goto out; } if (size) { saverr = 0; @@ -2864,15 +2866,17 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size) goto out_err; } ret = flistxattr(fd, value, size); + saverr = ret == -1 ? errno : 0; } else { /* fchdir should not fail here */ FCHDIR_NOFAIL(lo->proc_self_fd); ret = listxattr(procname, value, size); + saverr = ret == -1 ? errno : 0; FCHDIR_NOFAIL(lo->root.fd); } if (ret == -1) { - goto out_err; + goto out; } if (size) { saverr = 0; @@ -2998,15 +3002,15 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, goto out; } ret = fsetxattr(fd, name, value, size, flags); + saverr = ret == -1 ? errno : 0; } else { /* fchdir should not fail here */ FCHDIR_NOFAIL(lo->proc_self_fd); ret = setxattr(procname, name, value, size, flags); + saverr = ret == -1 ? errno : 0; FCHDIR_NOFAIL(lo->root.fd); } - saverr = ret == -1 ? errno : 0; - out: if (fd >= 0) { close(fd); @@ -3064,15 +3068,15 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name) goto out; } ret = fremovexattr(fd, name); + saverr = ret == -1 ? errno : 0; } else { /* fchdir should not fail here */ FCHDIR_NOFAIL(lo->proc_self_fd); ret = removexattr(procname, name); + saverr = ret == -1 ? errno : 0; FCHDIR_NOFAIL(lo->root.fd); } - saverr = ret == -1 ? errno : 0; - out: if (fd >= 0) { close(fd); -- cgit v1.1 From c46ef954fa33f5a409144c5395f8b23d22f42cab Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Tue, 22 Jun 2021 11:08:48 -0400 Subject: virtiofsd: Add support for extended setxattr Add the bits to enable support for setxattr_ext if fuse offers it. Do not enable it by default yet. Let passthrough_ll opt-in. Enabling it by deafult kind of automatically means that you are taking responsibility of clearing SGID if ACL is set. Signed-off-by: Vivek Goyal Message-Id: <20210622150852.1507204-4-vgoyal@redhat.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert Fixed up double def in fuse_common.h --- tools/virtiofsd/fuse_lowlevel.c | 11 ++++++++++- tools/virtiofsd/fuse_lowlevel.h | 3 ++- tools/virtiofsd/passthrough_ll.c | 3 ++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c index 2028677..e4679c7 100644 --- a/tools/virtiofsd/fuse_lowlevel.c +++ b/tools/virtiofsd/fuse_lowlevel.c @@ -1445,7 +1445,9 @@ static void do_setxattr(fuse_req_t req, fuse_ino_t nodeid, } if (req->se->op.setxattr) { - req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags); + uint32_t setxattr_flags = setxattr_ext ? arg->setxattr_flags : 0; + req->se->op.setxattr(req, nodeid, name, value, arg->size, arg->flags, + setxattr_flags); } else { fuse_reply_err(req, ENOSYS); } @@ -1992,6 +1994,9 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid, if (arg->flags & FUSE_HANDLE_KILLPRIV_V2) { se->conn.capable |= FUSE_CAP_HANDLE_KILLPRIV_V2; } + if (arg->flags & FUSE_SETXATTR_EXT) { + se->conn.capable |= FUSE_CAP_SETXATTR_EXT; + } #ifdef HAVE_SPLICE #ifdef HAVE_VMSPLICE se->conn.capable |= FUSE_CAP_SPLICE_WRITE | FUSE_CAP_SPLICE_MOVE; @@ -2127,6 +2132,10 @@ static void do_init(fuse_req_t req, fuse_ino_t nodeid, outarg.flags |= FUSE_HANDLE_KILLPRIV_V2; } + if (se->conn.want & FUSE_CAP_SETXATTR_EXT) { + outarg.flags |= FUSE_SETXATTR_EXT; + } + fuse_log(FUSE_LOG_DEBUG, " INIT: %u.%u\n", outarg.major, outarg.minor); fuse_log(FUSE_LOG_DEBUG, " flags=0x%08x\n", outarg.flags); fuse_log(FUSE_LOG_DEBUG, " max_readahead=0x%08x\n", outarg.max_readahead); diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h index 3bf786b..4b4e8c9 100644 --- a/tools/virtiofsd/fuse_lowlevel.h +++ b/tools/virtiofsd/fuse_lowlevel.h @@ -798,7 +798,8 @@ struct fuse_lowlevel_ops { * fuse_reply_err */ void (*setxattr)(fuse_req_t req, fuse_ino_t ino, const char *name, - const char *value, size_t size, int flags); + const char *value, size_t size, int flags, + uint32_t setxattr_flags); /** * Get an extended attribute diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index ccbda98..4dec087 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -2955,7 +2955,8 @@ out: } static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, - const char *value, size_t size, int flags) + const char *value, size_t size, int flags, + uint32_t extra_flags) { char procname[64]; const char *name; -- cgit v1.1 From 6d0028b94797157477ddc759209f3e523e186ad9 Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Tue, 22 Jun 2021 11:08:49 -0400 Subject: virtiofsd: Add umask to seccom allow list Patches in this series are going to make use of "umask" syscall. So allow it. Signed-off-by: Vivek Goyal Reviewed-by: Stefan Hajnoczi Message-Id: <20210622150852.1507204-5-vgoyal@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- tools/virtiofsd/passthrough_seccomp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virtiofsd/passthrough_seccomp.c b/tools/virtiofsd/passthrough_seccomp.c index 62441cf..f49ed94 100644 --- a/tools/virtiofsd/passthrough_seccomp.c +++ b/tools/virtiofsd/passthrough_seccomp.c @@ -114,6 +114,7 @@ static const int syscall_allowlist[] = { SCMP_SYS(utimensat), SCMP_SYS(write), SCMP_SYS(writev), + SCMP_SYS(umask), }; /* Syscalls used when --syslog is enabled */ -- cgit v1.1 From 227e5d7fd57b94e36fd754099308969b1373c4ef Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Tue, 22 Jun 2021 11:08:50 -0400 Subject: virtiofsd: Add capability to change/restore umask When parent directory has default acl and a file is created in that directory, then umask is ignored and final file permissions are determined using default acl instead. (man 2 umask). Currently, fuse applies the umask and sends modified mode in create request accordingly. fuse server can set FUSE_DONT_MASK and tell fuse client to not apply umask and fuse server will take care of it as needed. With posix acls enabled, requirement will be that we want umask to determine final file mode if parent directory does not have default acl. So if posix acls are enabled, opt in for FUSE_DONT_MASK. virtiofsd will set umask of the thread doing file creation. And host kernel should use that umask if parent directory does not have default acls, otherwise umask does not take affect. Miklos mentioned that we already call unshare(CLONE_FS) for every thread. That means umask has now become property of per thread and it should be ok to manipulate it in file creation path. This patch only adds capability to change umask and restore it. It does not enable it yet. Next few patches will add capability to enable it based on if user enabled posix_acl or not. This should fix fstest generic/099. Reported-by: Luis Henriques Signed-off-by: Vivek Goyal Reviewed-by: Stefan Hajnoczi Reviewed-by: Dr. David Alan Gilbert Message-Id: <20210622150852.1507204-6-vgoyal@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- tools/virtiofsd/passthrough_ll.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 4dec087..65b2c6f 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -122,6 +122,7 @@ struct lo_inode { struct lo_cred { uid_t euid; gid_t egid; + mode_t umask; }; enum { @@ -172,6 +173,8 @@ struct lo_data { /* An O_PATH file descriptor to /proc/self/fd/ */ int proc_self_fd; int user_killpriv_v2, killpriv_v2; + /* If set, virtiofsd is responsible for setting umask during creation */ + bool change_umask; }; static const struct fuse_opt lo_opts[] = { @@ -1134,7 +1137,8 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name) * ownership of caller. * TODO: What about selinux context? */ -static int lo_change_cred(fuse_req_t req, struct lo_cred *old) +static int lo_change_cred(fuse_req_t req, struct lo_cred *old, + bool change_umask) { int res; @@ -1154,11 +1158,14 @@ static int lo_change_cred(fuse_req_t req, struct lo_cred *old) return errno_save; } + if (change_umask) { + old->umask = umask(req->ctx.umask); + } return 0; } /* Regain Privileges */ -static void lo_restore_cred(struct lo_cred *old) +static void lo_restore_cred(struct lo_cred *old, bool restore_umask) { int res; @@ -1173,6 +1180,9 @@ static void lo_restore_cred(struct lo_cred *old) fuse_log(FUSE_LOG_ERR, "setegid(%u): %m\n", old->egid); exit(1); } + + if (restore_umask) + umask(old->umask); } static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, @@ -1202,7 +1212,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, return; } - saverr = lo_change_cred(req, &old); + saverr = lo_change_cred(req, &old, lo->change_umask && !S_ISLNK(mode)); if (saverr) { goto out; } @@ -1211,7 +1221,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, saverr = errno; - lo_restore_cred(&old); + lo_restore_cred(&old, lo->change_umask && !S_ISLNK(mode)); if (res == -1) { goto out; @@ -1917,7 +1927,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, return; } - err = lo_change_cred(req, &old); + err = lo_change_cred(req, &old, lo->change_umask); if (err) { goto out; } @@ -1928,7 +1938,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, fd = openat(parent_inode->fd, name, fi->flags | O_CREAT | O_EXCL, mode); err = fd == -1 ? errno : 0; - lo_restore_cred(&old); + lo_restore_cred(&old, lo->change_umask); /* Ignore the error if file exists and O_EXCL was not given */ if (err && (err != EEXIST || (fi->flags & O_EXCL))) { -- cgit v1.1 From f1aa1774dfb79ce0546d9fbc1ce05cb00550c600 Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Tue, 22 Jun 2021 11:08:51 -0400 Subject: virtiofsd: Switch creds, drop FSETID for system.posix_acl_access xattr When posix access acls are set on a file, it can lead to adjusting file permissions (mode) as well. If caller does not have CAP_FSETID and it also does not have membership of owner group, this will lead to clearing SGID bit in mode. Current fuse code is written in such a way that it expects file server to take care of chaning file mode (permission), if there is a need. Right now, host kernel does not clear SGID bit because virtiofsd is running as root and has CAP_FSETID. For host kernel to clear SGID, virtiofsd need to switch to gid of caller in guest and also drop CAP_FSETID (if caller did not have it to begin with). If SGID needs to be cleared, client will set the flag FUSE_SETXATTR_ACL_KILL_SGID in setxattr request. In that case server should kill sgid. Currently just switch to uid/gid of the caller and drop CAP_FSETID and that should do it. This should fix the xfstest generic/375 test case. We don't have to switch uid for this to work. That could be one optimization that pass a parameter to lo_change_cred() to only switch gid and not uid. Also this will not work whenever (if ever) we support idmapped mounts. In that case it is possible that uid/gid in request are 0/0 but still we need to clear SGID. So we will have to pick a non-root sgid and switch to that instead. That's an TODO item for future when idmapped mount support is introduced. This patch only adds the capability to switch creds and drop FSETID when acl xattr is set. This does not take affect yet. It can take affect when next patch adds the capability to enable posix_acl. Reported-by: Luis Henriques Signed-off-by: Vivek Goyal Message-Id: <20210622150852.1507204-7-vgoyal@redhat.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- tools/virtiofsd/passthrough_ll.c | 75 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 65b2c6f..6e30fd9 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -175,6 +175,7 @@ struct lo_data { int user_killpriv_v2, killpriv_v2; /* If set, virtiofsd is responsible for setting umask during creation */ bool change_umask; + int posix_acl; }; static const struct fuse_opt lo_opts[] = { @@ -1185,6 +1186,51 @@ static void lo_restore_cred(struct lo_cred *old, bool restore_umask) umask(old->umask); } +/* + * A helper to change cred and drop capability. Returns 0 on success and + * errno on error + */ +static int lo_drop_cap_change_cred(fuse_req_t req, struct lo_cred *old, + bool change_umask, const char *cap_name, + bool *cap_dropped) +{ + int ret; + bool __cap_dropped; + + assert(cap_name); + + ret = drop_effective_cap(cap_name, &__cap_dropped); + if (ret) { + return ret; + } + + ret = lo_change_cred(req, old, change_umask); + if (ret) { + if (__cap_dropped) { + if (gain_effective_cap(cap_name)) { + fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name); + } + } + } + + if (cap_dropped) { + *cap_dropped = __cap_dropped; + } + return ret; +} + +static void lo_restore_cred_gain_cap(struct lo_cred *old, bool restore_umask, + const char *cap_name) +{ + assert(cap_name); + + lo_restore_cred(old, restore_umask); + + if (gain_effective_cap(cap_name)) { + fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_%s\n", cap_name); + } +} + static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, const char *name, mode_t mode, dev_t rdev, const char *link) @@ -2976,6 +3022,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, ssize_t ret; int saverr; int fd = -1; + bool switched_creds = false; + bool cap_fsetid_dropped = false; + struct lo_cred old = {}; mapped_name = NULL; name = in_name; @@ -3006,6 +3055,26 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, ", name=%s value=%s size=%zd)\n", ino, name, value, size); sprintf(procname, "%i", inode->fd); + /* + * If we are setting posix access acl and if SGID needs to be + * cleared, then switch to caller's gid and drop CAP_FSETID + * and that should make sure host kernel clears SGID. + * + * This probably will not work when we support idmapped mounts. + * In that case we will need to find a non-root gid and switch + * to it. (Instead of gid in request). Fix it when we support + * idmapped mounts. + */ + if (lo->posix_acl && !strcmp(name, "system.posix_acl_access") + && (extra_flags & FUSE_SETXATTR_ACL_KILL_SGID)) { + ret = lo_drop_cap_change_cred(req, &old, false, "FSETID", + &cap_fsetid_dropped); + if (ret) { + saverr = ret; + goto out; + } + switched_creds = true; + } if (S_ISREG(inode->filetype) || S_ISDIR(inode->filetype)) { fd = openat(lo->proc_self_fd, procname, O_RDONLY); if (fd < 0) { @@ -3021,6 +3090,12 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, saverr = ret == -1 ? errno : 0; FCHDIR_NOFAIL(lo->root.fd); } + if (switched_creds) { + if (cap_fsetid_dropped) + lo_restore_cred_gain_cap(&old, false, "FSETID"); + else + lo_restore_cred(&old, false); + } out: if (fd >= 0) { -- cgit v1.1 From 65a820d292622fd2c3e1e5cd49455adc7f90faf0 Mon Sep 17 00:00:00 2001 From: Vivek Goyal Date: Tue, 22 Jun 2021 11:08:52 -0400 Subject: virtiofsd: Add an option to enable/disable posix acls fuse has an option FUSE_POSIX_ACL which needs to be opted in by fuse server to enable posix acls. As of now we are not opting in for this, so posix acls are disabled on virtiofs by default. Add virtiofsd option "-o posix_acl/no_posix_acl" to let users enable/disable posix acl support. By default it is disabled as of now due to performance concerns with cache=none. Currently even if file server has not opted in for FUSE_POSIX_ACL, user can still query acl and set acl, and system.posix_acl_access and system.posix_acl_default xattrs show up listxattr response. Miklos said this is confusing. So he said lets block and filter system.posix_acl_access and system.posix_acl_default xattrs in getxattr/setxattr/listxattr if user has explicitly disabled posix acls using -o no_posix_acl. As of now continuing to keeping the existing behavior if user did not specify any option to disable acl support due to concerns about backward compatibility. Signed-off-by: Vivek Goyal Message-Id: <20210622150852.1507204-8-vgoyal@redhat.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- docs/tools/virtiofsd.rst | 3 + tools/virtiofsd/helper.c | 1 + tools/virtiofsd/passthrough_ll.c | 115 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 118 insertions(+), 1 deletion(-) diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst index a6c3502..c4ac7fd 100644 --- a/docs/tools/virtiofsd.rst +++ b/docs/tools/virtiofsd.rst @@ -101,6 +101,9 @@ Options Enable/disable extended attributes (xattr) on files and directories. The default is ``no_xattr``. + * posix_acl|no_posix_acl - + Enable/disable posix acl support. Posix ACLs are disabled by default`. + .. option:: --socket-path=PATH Listen on vhost-user UNIX domain socket at PATH. diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c index 5e98ed7..a8295d9 100644 --- a/tools/virtiofsd/helper.c +++ b/tools/virtiofsd/helper.c @@ -186,6 +186,7 @@ void fuse_cmdline_help(void) " to virtiofsd from guest applications.\n" " default: no_allow_direct_io\n" " -o announce_submounts Announce sub-mount points to the guest\n" + " -o posix_acl/no_posix_acl Enable/Disable posix_acl. (default: disabled)\n" ); } diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 6e30fd9..38b2af8 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -175,7 +175,7 @@ struct lo_data { int user_killpriv_v2, killpriv_v2; /* If set, virtiofsd is responsible for setting umask during creation */ bool change_umask; - int posix_acl; + int user_posix_acl, posix_acl; }; static const struct fuse_opt lo_opts[] = { @@ -208,6 +208,8 @@ static const struct fuse_opt lo_opts[] = { { "announce_submounts", offsetof(struct lo_data, announce_submounts), 1 }, { "killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 1 }, { "no_killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 0 }, + { "posix_acl", offsetof(struct lo_data, user_posix_acl), 1 }, + { "no_posix_acl", offsetof(struct lo_data, user_posix_acl), 0 }, FUSE_OPT_END }; static bool use_syslog = false; @@ -706,6 +708,32 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn) conn->want &= ~FUSE_CAP_HANDLE_KILLPRIV_V2; lo->killpriv_v2 = 0; } + + if (lo->user_posix_acl == 1) { + /* + * User explicitly asked for this option. Enable it unconditionally. + * If connection does not have this capability, print error message + * now. It will fail later in fuse_lowlevel.c + */ + if (!(conn->capable & FUSE_CAP_POSIX_ACL) || + !(conn->capable & FUSE_CAP_DONT_MASK) || + !(conn->capable & FUSE_CAP_SETXATTR_EXT)) { + fuse_log(FUSE_LOG_ERR, "lo_init: Can not enable posix acl." + " kernel does not support FUSE_POSIX_ACL, FUSE_DONT_MASK" + " or FUSE_SETXATTR_EXT capability.\n"); + } else { + fuse_log(FUSE_LOG_DEBUG, "lo_init: enabling posix acl\n"); + } + + conn->want |= FUSE_CAP_POSIX_ACL | FUSE_CAP_DONT_MASK | + FUSE_CAP_SETXATTR_EXT; + lo->change_umask = true; + lo->posix_acl = true; + } else { + /* User either did not specify anything or wants it disabled */ + fuse_log(FUSE_LOG_DEBUG, "lo_init: disabling posix_acl\n"); + conn->want &= ~FUSE_CAP_POSIX_ACL; + } } static void lo_getattr(fuse_req_t req, fuse_ino_t ino, @@ -2783,6 +2811,63 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name, assert(fchdir_res == 0); \ } while (0) +static bool block_xattr(struct lo_data *lo, const char *name) +{ + /* + * If user explicitly enabled posix_acl or did not provide any option, + * do not block acl. Otherwise block system.posix_acl_access and + * system.posix_acl_default xattrs. + */ + if (lo->user_posix_acl) { + return false; + } + if (!strcmp(name, "system.posix_acl_access") || + !strcmp(name, "system.posix_acl_default")) + return true; + + return false; +} + +/* + * Returns number of bytes in xattr_list after filtering on success. This + * could be zero as well if nothing is left after filtering. + * + * Returns negative error code on failure. + * xattr_list is modified in place. + */ +static int remove_blocked_xattrs(struct lo_data *lo, char *xattr_list, + unsigned in_size) +{ + size_t out_index, in_index; + + /* + * As of now we only filter out acl xattrs. If acls are enabled or + * they have not been explicitly disabled, there is nothing to + * filter. + */ + if (lo->user_posix_acl) { + return in_size; + } + + out_index = 0; + in_index = 0; + while (in_index < in_size) { + char *in_ptr = xattr_list + in_index; + + /* Length of current attribute name */ + size_t in_len = strlen(xattr_list + in_index) + 1; + + if (!block_xattr(lo, in_ptr)) { + if (in_index != out_index) { + memmove(xattr_list + out_index, xattr_list + in_index, in_len); + } + out_index += in_len; + } + in_index += in_len; + } + return out_index; +} + static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, size_t size) { @@ -2796,6 +2881,11 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, int saverr; int fd = -1; + if (block_xattr(lo, in_name)) { + fuse_reply_err(req, EOPNOTSUPP); + return; + } + mapped_name = NULL; name = in_name; if (lo->xattrmap) { @@ -2986,6 +3076,12 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size) goto out; } } + + ret = remove_blocked_xattrs(lo, value, ret); + if (ret <= 0) { + saverr = -ret; + goto out; + } fuse_reply_buf(req, value, ret); } else { /* @@ -3026,6 +3122,11 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, bool cap_fsetid_dropped = false; struct lo_cred old = {}; + if (block_xattr(lo, in_name)) { + fuse_reply_err(req, EOPNOTSUPP); + return; + } + mapped_name = NULL; name = in_name; if (lo->xattrmap) { @@ -3118,6 +3219,11 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name) int saverr; int fd = -1; + if (block_xattr(lo, in_name)) { + fuse_reply_err(req, EOPNOTSUPP); + return; + } + mapped_name = NULL; name = in_name; if (lo->xattrmap) { @@ -3795,6 +3901,7 @@ int main(int argc, char *argv[]) .allow_direct_io = 0, .proc_self_fd = -1, .user_killpriv_v2 = -1, + .user_posix_acl = -1, }; struct lo_map_elem *root_elem; struct lo_map_elem *reserve_elem; @@ -3923,6 +4030,12 @@ int main(int argc, char *argv[]) exit(1); } + if (lo.user_posix_acl == 1 && !lo.xattr) { + fuse_log(FUSE_LOG_ERR, "Can't enable posix ACLs. xattrs are disabled." + "\n"); + exit(1); + } + lo.use_statx = true; se = fuse_session_new(&args, &lo_oper, sizeof(lo_oper), &lo); -- cgit v1.1 From c99fb3a50db983402e85423139e5d4449cda9111 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hyman=20Huang=28=E9=BB=84=E5=8B=87=29?= Date: Thu, 3 Jun 2021 22:19:53 +0800 Subject: tests/migration: parse the thread-id key of CpuInfoFast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit thread_id in CpuInfoFast is deprecated, parse thread-id instead after execute qmp query-cpus-fast. fix this so that test can go smoothly. Signed-off-by: Hyman Huang(黄勇) Message-Id: <584578c0a0dd781cee45f72ddf517f6e6a41c504.1622729934.git.huangy81@chinatelecom.cn> Reviewed-by: Daniel P. Berrangé Signed-off-by: Dr. David Alan Gilbert --- tests/migration/guestperf/engine.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/migration/guestperf/engine.py b/tests/migration/guestperf/engine.py index 208e095..9e16fa9 100644 --- a/tests/migration/guestperf/engine.py +++ b/tests/migration/guestperf/engine.py @@ -113,7 +113,7 @@ class Engine(object): vcpus = src.command("query-cpus-fast") src_threads = [] for vcpu in vcpus: - src_threads.append(vcpu["thread_id"]) + src_threads.append(vcpu["thread-id"]) # XXX how to get dst timings on remote host ? -- cgit v1.1 From fa264f426642245eb623e9f86cf57022d854dc69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hyman=20Huang=28=E9=BB=84=E5=8B=87=29?= Date: Thu, 3 Jun 2021 22:20:37 +0800 Subject: tests/migration: fix "downtime_limit" type when "migrate-set-parameters" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit migrate-set-parameters parse "downtime_limit" as integer type when execute "migrate-set-parameters" before migration, and, the unit dowtime_limit is milliseconds, fix this two so that test can go smoothly. Signed-off-by: Hyman Huang(黄勇) Message-Id: <31d82df24cc0c468dbe4d2d86730158ebf248071.1622729934.git.huangy81@chinatelecom.cn> Reviewed-by: Daniel P. Berrangé Signed-off-by: Dr. David Alan Gilbert --- tests/migration/guestperf/engine.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/migration/guestperf/engine.py b/tests/migration/guestperf/engine.py index 9e16fa9..7c991c4 100644 --- a/tests/migration/guestperf/engine.py +++ b/tests/migration/guestperf/engine.py @@ -153,7 +153,7 @@ class Engine(object): max_bandwidth=scenario._bandwidth * 1024 * 1024) resp = src.command("migrate-set-parameters", - downtime_limit=scenario._downtime / 1024.0) + downtime_limit=scenario._downtime) if scenario._compression_mt: resp = src.command("migrate-set-capabilities", -- cgit v1.1 From e5f607913cee3f3b486eb024dbc7079b51f6da57 Mon Sep 17 00:00:00 2001 From: Li Zhijian Date: Mon, 28 Jun 2021 15:19:59 +0800 Subject: migration/rdma: Use error_report to suppress errno message Since the prior calls are successful, in this case a errno doesn't indicate a real error which would just make us confused. before: (qemu) migrate -d rdma:192.168.22.23:8888 source_resolve_host RDMA Device opened: kernel name rxe_eth0 uverbs device name uverbs2, infiniband_verbs class device path /sys/class/infiniband_verbs/uverbs2, infiniband class device path /sys/class/infiniband/rxe_eth0, transport: (2) Ethernet rdma_get_cm_event != EVENT_ESTABLISHED after rdma_connect: No space left on device Signed-off-by: Li Zhijian Message-Id: <20210628071959.23455-1-lizhijian@cn.fujitsu.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- migration/rdma.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/migration/rdma.c b/migration/rdma.c index d90b29a..b6cc4be 100644 --- a/migration/rdma.c +++ b/migration/rdma.c @@ -1006,7 +1006,7 @@ route: if (cm_event->event != RDMA_CM_EVENT_ADDR_RESOLVED) { ERROR(errp, "result not equal to event_addr_resolved %s", rdma_event_str(cm_event->event)); - perror("rdma_resolve_addr"); + error_report("rdma_resolve_addr"); rdma_ack_cm_event(cm_event); ret = -EINVAL; goto err_resolve_get_addr; @@ -2544,7 +2544,7 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp, bool return_path) } if (cm_event->event != RDMA_CM_EVENT_ESTABLISHED) { - perror("rdma_get_cm_event != EVENT_ESTABLISHED after rdma_connect"); + error_report("rdma_get_cm_event != EVENT_ESTABLISHED after rdma_connect"); ERROR(errp, "connecting to destination!"); rdma_ack_cm_event(cm_event); goto err_rdma_source_connect; -- cgit v1.1