| Message ID | cover.1603285620.git.qemu_oss@crudebyte.com |
|---|---|
| Headers | show |
| Series | 9pfs: more local tests | expand |
On Wed, 21 Oct 2020 14:17:01 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > This test case uses a Tunlinkat 9p request with flag AT_REMOVEDIR > (see 'man 2 unlink') to remove a directory from host's test directory. > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > tests/qtest/virtio-9p-test.c | 71 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 71 insertions(+) > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c > index 21807037df..abd7e44648 100644 > --- a/tests/qtest/virtio-9p-test.c > +++ b/tests/qtest/virtio-9p-test.c > @@ -258,6 +258,7 @@ static const char *rmessage_name(uint8_t id) > id == P9_RLOPEN ? "RLOPEN" : > id == P9_RWRITE ? "RWRITE" : > id == P9_RMKDIR ? "RMKDIR" : > + id == P9_RUNLINKAT ? "RUNLINKAT" : > id == P9_RFLUSH ? "RFLUSH" : > id == P9_RREADDIR ? "READDIR" : > "<unknown>"; > @@ -693,6 +694,33 @@ static void v9fs_rmkdir(P9Req *req, v9fs_qid *qid) > v9fs_req_free(req); > } > > +/* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */ > +static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name, > + uint32_t flags, uint16_t tag) > +{ > + P9Req *req; > + > + uint32_t body_size = 4 + 4; > + uint16_t string_size = v9fs_string_size(name); > + > + g_assert_cmpint(body_size, <=, UINT32_MAX - string_size); > + body_size += string_size; > + > + req = v9fs_req_init(v9p, body_size, P9_TUNLINKAT, tag); > + v9fs_uint32_write(req, dirfd); > + v9fs_string_write(req, name); > + v9fs_uint32_write(req, flags); > + v9fs_req_send(req); > + return req; > +} > + > +/* size[4] Runlinkat tag[2] */ > +static void v9fs_runlinkat(P9Req *req) > +{ > + v9fs_req_recv(req, P9_RUNLINKAT); > + v9fs_req_free(req); > +} > + > /* basic readdir test where reply fits into a single response message */ > static void fs_readdir(void *obj, void *data, QGuestAllocator *t_alloc) > { > @@ -1004,6 +1032,22 @@ static void do_mkdir(QVirtio9P *v9p, const char *path, const char *cname) > g_free(name); > } > > +static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath, > + uint32_t flags) > +{ > + char *const name = g_strdup(rpath); > + uint32_t fid; > + P9Req *req; > + > + fid = do_walk(v9p, atpath); > + > + req = v9fs_tunlinkat(v9p, fid, name, flags, 0); > + v9fs_req_wait_for_reply(req, NULL); > + v9fs_runlinkat(req); > + > + g_free(name); > +} > + > static void fs_readdir_split_128(void *obj, void *data, > QGuestAllocator *t_alloc) > { > @@ -1050,6 +1094,32 @@ static void fs_create_dir(void *obj, void *data, QGuestAllocator *t_alloc) > g_free(root_path); > } > > +static void fs_unlinkat_dir(void *obj, void *data, QGuestAllocator *t_alloc) > +{ > + QVirtio9P *v9p = obj; > + alloc = t_alloc; > + struct stat st; > + char *root_path = virtio_9p_test_path(""); > + char *new_dir = virtio_9p_test_path("02"); > + > + g_assert(root_path != NULL); > + > + do_attach(v9p); > + do_mkdir(v9p, "/", "02"); > + > + /* check if created directory really exists now ... */ > + g_assert(stat(new_dir, &st) == 0); > + /* ... and is actually a directory */ > + g_assert((st.st_mode & S_IFMT) == S_IFDIR); > + > + do_unlinkat(v9p, "/", "02", AT_REMOVEDIR); > + /* directory should be gone now */ > + g_assert(stat(new_dir, &st) != 0); > + > + g_free(new_dir); > + g_free(root_path); > +} > + > static void *assign_9p_local_driver(GString *cmd_line, void *arg) > { > virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr"); > @@ -1090,6 +1160,7 @@ static void register_virtio_9p_test(void) > opts.before = assign_9p_local_driver; > qos_add_test("local/config", "virtio-9p", pci_config, &opts); > qos_add_test("local/create_dir", "virtio-9p", fs_create_dir, &opts); > + qos_add_test("local/unlinkat_dir", "virtio-9p", fs_unlinkat_dir, &opts); > } > > libqos_init(register_virtio_9p_test);
On Wed, 21 Oct 2020 14:36:23 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > This test case uses a Tunlinkat request to remove a symlink using > the 9pfs 'local' fs driver. > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > tests/qtest/virtio-9p-test.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c > index 0c11417236..33cba24b18 100644 > --- a/tests/qtest/virtio-9p-test.c > +++ b/tests/qtest/virtio-9p-test.c > @@ -1295,6 +1295,32 @@ static void fs_symlink_file(void *obj, void *data, QGuestAllocator *t_alloc) > g_free(real_file); > } > > +static void fs_unlinkat_symlink(void *obj, void *data, > + QGuestAllocator *t_alloc) > +{ > + QVirtio9P *v9p = obj; > + alloc = t_alloc; > + struct stat st; > + char *real_file = virtio_9p_test_path("06/real_file"); > + char *symlink_file = virtio_9p_test_path("06/symlink_file"); > + > + do_attach(v9p); > + do_mkdir(v9p, "/", "06"); > + do_lcreate(v9p, "06", "real_file"); > + g_assert(stat(real_file, &st) == 0); > + g_assert((st.st_mode & S_IFMT) == S_IFREG); > + > + do_symlink(v9p, "06", "symlink_file", "real_file"); > + g_assert(stat(symlink_file, &st) == 0); > + > + do_unlinkat(v9p, "06", "symlink_file", 0); > + /* symlink should be gone now */ > + g_assert(stat(symlink_file, &st) != 0); > + > + g_free(symlink_file); > + g_free(real_file); > +} > + > static void *assign_9p_local_driver(GString *cmd_line, void *arg) > { > virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr"); > @@ -1339,6 +1365,8 @@ static void register_virtio_9p_test(void) > qos_add_test("local/create_file", "virtio-9p", fs_create_file, &opts); > qos_add_test("local/unlinkat_file", "virtio-9p", fs_unlinkat_file, &opts); > qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, &opts); > + qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink, > + &opts); > } > > libqos_init(register_virtio_9p_test);
On Wed, 21 Oct 2020 14:51:09 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > This test case uses a Tlink request to create a hard link to a regular > file using the 9pfs 'local' fs driver. > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > tests/qtest/virtio-9p-test.c | 71 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 71 insertions(+) > > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c > index 33cba24b18..460fa49fe3 100644 > --- a/tests/qtest/virtio-9p-test.c > +++ b/tests/qtest/virtio-9p-test.c > @@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id) > id == P9_RMKDIR ? "RMKDIR" : > id == P9_RLCREATE ? "RLCREATE" : > id == P9_RSYMLINK ? "RSYMLINK" : > + id == P9_RLINK ? "RLINK" : > id == P9_RUNLINKAT ? "RUNLINKAT" : > id == P9_RFLUSH ? "RFLUSH" : > id == P9_RREADDIR ? "READDIR" : > @@ -767,6 +768,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid) > v9fs_req_free(req); > } > > +/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */ > +static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid, > + const char *name, uint16_t tag) > +{ > + P9Req *req; > + > + uint32_t body_size = 4 + 4; > + uint16_t string_size = v9fs_string_size(name); > + > + g_assert_cmpint(body_size, <=, UINT32_MAX - string_size); > + body_size += string_size; > + > + req = v9fs_req_init(v9p, body_size, P9_TLINK, tag); > + v9fs_uint32_write(req, dfid); > + v9fs_uint32_write(req, fid); > + v9fs_string_write(req, name); > + v9fs_req_send(req); > + return req; > +} > + > +/* size[4] Rlink tag[2] */ > +static void v9fs_rlink(P9Req *req) > +{ > + v9fs_req_recv(req, P9_RLINK); > + v9fs_req_free(req); > +} > + > /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */ > static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name, > uint32_t flags, uint16_t tag) > @@ -1142,6 +1170,21 @@ static void do_symlink(QVirtio9P *v9p, const char *path, const char *clink, > g_free(name); > } > > +/* create a hard link named @a clink in directory @a path pointing to @a to */ > +static void do_hardlink(QVirtio9P *v9p, const char *path, const char *clink, > + const char *to) > +{ > + uint32_t dfid, fid; > + P9Req *req; > + > + dfid = do_walk(v9p, path); > + fid = do_walk(v9p, to); > + > + req = v9fs_tlink(v9p, dfid, fid, clink, 0); > + v9fs_req_wait_for_reply(req, NULL); > + v9fs_rlink(req); > +} > + > static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath, > uint32_t flags) > { > @@ -1321,6 +1364,33 @@ static void fs_unlinkat_symlink(void *obj, void *data, > g_free(real_file); > } > > +static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc) > +{ > + QVirtio9P *v9p = obj; > + alloc = t_alloc; > + struct stat st_real, st_link; > + char *real_file = virtio_9p_test_path("07/real_file"); > + char *hardlink_file = virtio_9p_test_path("07/hardlink_file"); > + > + do_attach(v9p); > + do_mkdir(v9p, "/", "07"); > + do_lcreate(v9p, "07", "real_file"); > + g_assert(stat(real_file, &st_real) == 0); > + g_assert((st_real.st_mode & S_IFMT) == S_IFREG); > + > + do_hardlink(v9p, "07", "hardlink_file", "07/real_file"); > + > + /* check if link exists now ... */ > + g_assert(stat(hardlink_file, &st_link) == 0); > + /* ... and it's a hard link, right? */ > + g_assert((st_link.st_mode & S_IFMT) == S_IFREG); > + g_assert(st_link.st_dev == st_real.st_dev); > + g_assert(st_link.st_ino == st_real.st_ino); > + > + g_free(hardlink_file); > + g_free(real_file); > +} > + > static void *assign_9p_local_driver(GString *cmd_line, void *arg) > { > virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr"); > @@ -1367,6 +1437,7 @@ static void register_virtio_9p_test(void) > qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, &opts); > qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink, > &opts); > + qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, &opts); > } > > libqos_init(register_virtio_9p_test);
On Donnerstag, 22. Oktober 2020 11:07:49 CEST Greg Kurz wrote: > > Ok, I found the problem on the mentioned box that failed to create hard > > links with 9p: it is libvirt auto generating AppArmor policy rules for 9p > > export pathes, which libvirt generates with "rw" AA (AppArmor) > > permissions. Once I manually adjusted the AA rule to "rwl", creating hard > > links worked again. > > > > I guess I'll send a patch for libvirt to change that. At least IMO > > creating > > hard links with 9pfs is a very basic operation and should be enabled for > > the respective 9p export path by default. > > > > Actually I think it should also include "k" which means "file locking", as > > file locking is also a fundamental operation with 9pfs, right? > > Well, I don't know exactly why libvirt is generating a strict AppArmor > policy but I'm not that surprised. If the user can _easily_ change the > policy to fit its needs, it's fine to have a "better safe than sorry" > default. Alone identifying the root cause of this is not that easy I would say. And if it takes me quite some time to find it, then I imagine that other people would struggle with this even more. A large portion of software, even scripts, rely on being able to create hard links and locking files. Right now they typically error out on guest with no helpful error message. So you start looking into the logs, don't find something obvious, then strace on guest side to find the most relevant failing syscall on guest side and see it was probably link(). Then you have to check several security layers: file permissions on guest, file permissions on host, effective UID of qemu process. You try creating hard links directly on host with that UID, works. Next you check is it qemu's seccomp? Is it host's SELinux? Is it AppArmor? Even for an experienced sysadmin I doubt it'll be a matter of minutes to resolve this issue. Now imagine a regular user who just wants to sandbox something on a workstation. Looking at libvirt git log, it seems this security policy exists more or less since day one (SHA-1 29ea8a9b64aac60251d283f74d57690e4edb5a6b, Mar 9 2014). And I don't see an explanation that would suggest a specific reason for exactly "rw". I think something has to be improved here, so I'll challenge by sending a simple libvirt patch, CCing involved authors, and seeing the feedback: diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 12429278fb..ce243e304b 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1142,7 +1142,7 @@ get_files(vahControl * ctl) /* We don't need to add deny rw rules for readonly mounts, * this can only lead to troubles when mounting / readonly. */ - if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rw", true) != 0) + if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rwlk", true) != 0) goto cleanup; } } Even after this change, this is not a global policy. Allowing hard links and file locks would only be lifted for the 9p export path. There would be other options as well of course: e.g. detecting on 9pfs side whether AppArmor and co are enabled, and log a warning to the user a syscall failed for that reason. But that would be much more complicated and I wonder whether it would be worth it. Best regards, Christian Schoenebeck