> > Hi, >> >> I think both 3 and 4 parts of your patches look good. But I guess it >> would not hurt if Nadav could scrutinize at them from C++ perspective as >> well. >> >> However, I am having a bit of trouble testing those. I do not think >> anything is wrong with your patches but possibly something has changed on >> QEMU side virtiofs implementation or something else. >> >> I am not Ubuntu 20.04 and using your QEMU branch - fix-dax-fd, >> >> Whenever I run virtiofsd and QEMU and I get the error: >> >> ./virtiofsd --socket-path=/tmp/vhostqemu -o >> source=/home/wkozaczuk/projects/osv/apps/native-example -o cache=always -d >> [2240367089675] [ID: 00018845] virtio_session_mount: Waiting for >> vhost-user socket connection... >> [2249948371337] [ID: 00018845] virtio_session_mount: Received vhost-user >> socket connection >> [2249949209337] [ID: 00000001] tmpdir(virtiofsd-nuZoui): Permission denied >> >> /home/wkozaczuk/projects/qemu/build/x86_64-softmmu/qemu-system-x86_64 \ >> -m 2G \ >> -smp 4 \ >> -vnc :1 \ >> -gdb tcp::1234,server,nowait \ >> -kernel /home/wkozaczuk/projects/osv/build/last/kernel.elf \ >> -append "--mount-fs=virtiofs,/dev/virtiofs1,/tmp/virtiofs >> /tmp/virtiofs/hello" \ >> -device virtio-blk-pci,id=blk0,drive=hd0,scsi=off \ >> -drive >> file=/home/wkozaczuk/projects/osv/build/last/usr.img,if=none,id=hd0,cache=none,aio=native >> >> \ >> -chardev socket,id=char0,path=/tmp/vhostqemu \ >> -device vhost-user-fs-pci,queue-size=1024,chardev=char0,tag=myfs \ >> -object memory-backend-file,id=mem,size=2G,mem-path=/dev/shm,share=on \ >> -numa node,memdev=mem \ >> -netdev user,id=un0,net=192.168.122.0/24,host=192.168.122.1 \ >> -device virtio-net-pci,netdev=un0 \ >> -device virtio-rng-pci \ >> -enable-kvm \ >> -cpu host,+x2apic \ >> -chardev stdio,mux=on,id=stdio,signal=off \ >> -mon chardev=stdio,mode=readline \ >> -device isa-serial,chardev=stdio >> >> qemu-system-x86_64: -device >> vhost-user-fs-pci,queue-size=1024,chardev=char0,tag=myfs: Failed to read >> msg header. Read 0 instead of 12. Original request 1. >> qemu-system-x86_64: -device >> vhost-user-fs-pci,queue-size=1024,chardev=char0,tag=myfs: vhost_dev_init >> failed: Operation not permitted >> >> I know virtiofs is a moving target so I must be missing some settings, it >> looks like a permission error. Any ideas? >> > Truth is I am getting the exact same errors and I haven't yet wrapped my > head around the security / sandboxing functionality of virtiofsd, so > unfortunately no ideas on this (apart from that it *seems* to originate > in virtiofsd, and what fails is the mkdtemp call here > <https://gitlab.com/virtio-fs/qemu/-/blob/21336c0f3d05a97f5c409bbc894c19d87259655c/tools/virtiofsd/passthrough_ll.c#L2905> > ). > Not porud to say this, but I have been running with elevated privileges > locally. I will reach out to the virtio-fs devs to ask though and hopefully > come back with a proper answer! > Thank you for pointing this out > Update: the answer from the devs is that it's preferred to run virtiofsd with elevated privileges indeed, as it's supposed to drop all excess privileges at runtime.
>> Regards, >> Waldek >> >> On Sunday, May 17, 2020 at 5:52:01 AM UTC-4, Fotis Xenakis wrote: >>> >>> Since in virtio-fs the filesystem is very tightly coupled with the >>> driver, this tries to make clear the dependence of the first on the >>> second, as well as simplify. >>> >>> This includes: >>> - The definition of fuse_request is moved from the fs to the driver, >>> since it is part of the interface it provides. Also, it is enhanced >>> with methods, somewhat promoting it to a "proper" class. >>> - fuse_strategy, as a redirection to the driver is removed and instead >>> the dependence on the driver is made explicit. >>> - Last, virtio::fs::fs_req is removed and fuse_request is used in its >>> place, since it offered no value with fuse_request now defined in the >>> driver. >>> >>> Signed-off-by: Fotis Xenakis <[email protected]> >>> --- >>> drivers/virtio-fs.cc | 42 +++++++++++++--------------------- >>> drivers/virtio-fs.hh | 27 +++++++++++++++------- >>> fs/virtiofs/virtiofs_i.hh | 24 ++----------------- >>> fs/virtiofs/virtiofs_vfsops.cc | 17 +++++++------- >>> fs/virtiofs/virtiofs_vnops.cc | 39 +++++++++++++++---------------- >>> 5 files changed, 64 insertions(+), 85 deletions(-) >>> >>> diff --git a/drivers/virtio-fs.cc b/drivers/virtio-fs.cc >>> index ca9b00fc..e0e090bc 100644 >>> --- a/drivers/virtio-fs.cc >>> +++ b/drivers/virtio-fs.cc >>> @@ -28,25 +28,23 @@ >>> >>> using namespace memory; >>> >>> -void fuse_req_wait(fuse_request* req) >>> -{ >>> - WITH_LOCK(req->req_mutex) { >>> - req->req_wait.wait(req->req_mutex); >>> - } >>> -} >>> +using fuse_request = virtio::fs::fuse_request; >>> >>> namespace virtio { >>> >>> -static int fuse_make_request(void* driver, fuse_request* req) >>> +// Wait for the request to be marked as completed. >>> +void fs::fuse_request::wait() >>> { >>> - auto fs_driver = static_cast<fs*>(driver); >>> - return fs_driver->make_request(req); >>> + WITH_LOCK(req_mutex) { >>> + req_wait.wait(req_mutex); >>> + } >>> } >>> >>> -static void fuse_req_done(fuse_request* req) >>> +// Mark the request as completed. >>> +void fs::fuse_request::done() >>> { >>> - WITH_LOCK(req->req_mutex) { >>> - req->req_wait.wake_one(req->req_mutex); >>> + WITH_LOCK(req_mutex) { >>> + req_wait.wake_one(req_mutex); >>> } >>> } >>> >>> @@ -87,7 +85,7 @@ static struct devops fs_devops { >>> struct driver fs_driver = { >>> "virtio_fs", >>> &fs_devops, >>> - sizeof(struct fuse_strategy), >>> + sizeof(fs*), >>> }; >>> >>> bool fs::ack_irq() >>> @@ -161,10 +159,7 @@ fs::fs(virtio_device& virtio_dev) >>> dev_name += std::to_string(_disk_idx++); >>> >>> struct device* dev = device_create(&fs_driver, dev_name.c_str(), >>> D_BLK); // TODO Should it be really D_BLK? >>> - auto* strategy = static_cast<fuse_strategy*>(dev->private_data); >>> - strategy->drv = this; >>> - strategy->make_request = fuse_make_request; >>> - >>> + dev->private_data = this; >>> debugf("virtio-fs: Add device instance %d as [%s]\n", _id, >>> dev_name.c_str()); >>> } >>> @@ -201,13 +196,12 @@ void fs::req_done() >>> while (true) { >>> virtio_driver::wait_for_queue(queue, >>> &vring::used_ring_not_empty); >>> >>> - fs_req* req; >>> + fuse_request* req; >>> u32 len; >>> - while ((req = static_cast<fs_req*>(queue->get_buf_elem(&len))) >>> != >>> + while ((req = >>> static_cast<fuse_request*>(queue->get_buf_elem(&len))) != >>> nullptr) { >>> >>> - fuse_req_done(req->fuse_req); >>> - delete req; >>> + req->done(); >>> queue->get_buf_finalize(); >>> } >>> >>> @@ -231,11 +225,7 @@ int fs::make_request(fuse_request* req) >>> fuse_req_enqueue_input(queue, req); >>> fuse_req_enqueue_output(queue, req); >>> >>> - auto* fs_request = new (std::nothrow) fs_req(req); >>> - if (!fs_request) { >>> - return ENOMEM; >>> - } >>> - queue->add_buf_wait(fs_request); >>> + queue->add_buf_wait(req); >>> queue->kick(); >>> >>> return 0; >>> diff --git a/drivers/virtio-fs.hh b/drivers/virtio-fs.hh >>> index fdbab5d0..d78d7962 100644 >>> --- a/drivers/virtio-fs.hh >>> +++ b/drivers/virtio-fs.hh >>> @@ -12,7 +12,7 @@ >>> #include <osv/waitqueue.hh> >>> #include "drivers/virtio.hh" >>> #include "drivers/virtio-device.hh" >>> -#include "fs/virtiofs/virtiofs_i.hh" >>> +#include "fs/virtiofs/fuse_kernel.h" >>> >>> namespace virtio { >>> >>> @@ -23,6 +23,24 @@ enum { >>> >>> class fs : public virtio_driver { >>> public: >>> + struct fuse_request { >>> + struct fuse_in_header in_header; >>> + struct fuse_out_header out_header; >>> + >>> + void* input_args_data; >>> + size_t input_args_size; >>> + >>> + void* output_args_data; >>> + size_t output_args_size; >>> + >>> + void wait(); >>> + void done(); >>> + >>> + private: >>> + mutex_t req_mutex; >>> + waitqueue req_wait; >>> + }; >>> + >>> struct fs_config { >>> char tag[36]; >>> u32 num_queues; >>> @@ -59,13 +77,6 @@ public: >>> static hw_driver* probe(hw_device* dev); >>> >>> private: >>> - struct fs_req { >>> - fs_req(fuse_request* f) : fuse_req(f) {}; >>> - ~fs_req() {}; >>> - >>> - fuse_request* fuse_req; >>> - }; >>> - >>> std::string _driver_name; >>> fs_config _config; >>> dax_window _dax; >>> diff --git a/fs/virtiofs/virtiofs_i.hh b/fs/virtiofs/virtiofs_i.hh >>> index 17fbcd36..76533d74 100644 >>> --- a/fs/virtiofs/virtiofs_i.hh >>> +++ b/fs/virtiofs/virtiofs_i.hh >>> @@ -11,30 +11,10 @@ >>> #include "fuse_kernel.h" >>> #include <osv/mutex.h> >>> #include <osv/waitqueue.hh> >>> +#include "drivers/virtio-fs.hh" >>> >>> -struct fuse_request { >>> - struct fuse_in_header in_header; >>> - struct fuse_out_header out_header; >>> - >>> - void* input_args_data; >>> - size_t input_args_size; >>> - >>> - void* output_args_data; >>> - size_t output_args_size; >>> - >>> - mutex_t req_mutex; >>> - waitqueue req_wait; >>> -}; >>> - >>> -struct fuse_strategy { >>> - void* drv; >>> - int (*make_request)(void*, fuse_request*); >>> -}; >>> - >>> -int fuse_req_send_and_receive_reply(fuse_strategy* strategy, uint32_t >>> opcode, >>> +int fuse_req_send_and_receive_reply(virtio::fs* drv, uint32_t opcode, >>> uint64_t nodeid, void* input_args_data, size_t input_args_size, >>> void* output_args_data, size_t output_args_size); >>> >>> -void fuse_req_wait(fuse_request* req); >>> - >>> #endif >>> diff --git a/fs/virtiofs/virtiofs_vfsops.cc >>> b/fs/virtiofs/virtiofs_vfsops.cc >>> index 85cbf868..30daa42b 100644 >>> --- a/fs/virtiofs/virtiofs_vfsops.cc >>> +++ b/fs/virtiofs/virtiofs_vfsops.cc >>> @@ -14,9 +14,11 @@ >>> #include "virtiofs_i.hh" >>> #include "drivers/virtio-fs.hh" >>> >>> +using fuse_request = virtio::fs::fuse_request; >>> + >>> static std::atomic<uint64_t> fuse_unique_id(1); >>> >>> -int fuse_req_send_and_receive_reply(fuse_strategy* strategy, uint32_t >>> opcode, >>> +int fuse_req_send_and_receive_reply(virtio::fs* drv, uint32_t opcode, >>> uint64_t nodeid, void* input_args_data, size_t input_args_size, >>> void* output_args_data, size_t output_args_size) >>> { >>> @@ -36,9 +38,9 @@ int fuse_req_send_and_receive_reply(fuse_strategy* >>> strategy, uint32_t opcode, >>> req->output_args_data = output_args_data; >>> req->output_args_size = output_args_size; >>> >>> - assert(strategy->drv); >>> - strategy->make_request(strategy->drv, req.get()); >>> - fuse_req_wait(req.get()); >>> + assert(drv); >>> + drv->make_request(req.get()); >>> + req->wait(); >>> >>> int error = -req->out_header.error; >>> >>> @@ -88,8 +90,8 @@ static int virtiofs_mount(struct mount* mp, const >>> char* dev, int flags, >>> in_args->max_readahead = PAGE_SIZE; >>> in_args->flags |= FUSE_MAP_ALIGNMENT; >>> >>> - auto* strategy = static_cast<fuse_strategy*>(device->private_data); >>> - error = fuse_req_send_and_receive_reply(strategy, FUSE_INIT, >>> FUSE_ROOT_ID, >>> + auto* drv = static_cast<virtio::fs*>(device->private_data); >>> + error = fuse_req_send_and_receive_reply(drv, FUSE_INIT, >>> FUSE_ROOT_ID, >>> in_args.get(), sizeof(*in_args), out_args.get(), >>> sizeof(*out_args)); >>> if (error) { >>> kprintf("[virtiofs] Failed to initialize fuse filesystem!\n"); >>> @@ -101,7 +103,6 @@ static int virtiofs_mount(struct mount* mp, const >>> char* dev, int flags, >>> "minor: %d\n", out_args->major, out_args->minor); >>> >>> if (out_args->flags & FUSE_MAP_ALIGNMENT) { >>> - auto* drv = static_cast<virtio::fs*>(strategy->drv); >>> drv->set_map_alignment(out_args->map_alignment); >>> } >>> >>> @@ -114,7 +115,7 @@ static int virtiofs_mount(struct mount* mp, const >>> char* dev, int flags, >>> >>> virtiofs_set_vnode(mp->m_root->d_vnode, root_node); >>> >>> - mp->m_data = strategy; >>> + mp->m_data = drv; >>> mp->m_dev = device; >>> >>> return 0; >>> diff --git a/fs/virtiofs/virtiofs_vnops.cc >>> b/fs/virtiofs/virtiofs_vnops.cc >>> index dc75410f..38c000ec 100644 >>> --- a/fs/virtiofs/virtiofs_vnops.cc >>> +++ b/fs/virtiofs/virtiofs_vnops.cc >>> @@ -28,7 +28,6 @@ >>> >>> #include "virtiofs.hh" >>> #include "virtiofs_i.hh" >>> -#include "drivers/virtio-fs.hh" >>> >>> static constexpr uint32_t OPEN_FLAGS = O_RDONLY; >>> >>> @@ -60,8 +59,8 @@ static int virtiofs_lookup(struct vnode* vnode, char* >>> name, struct vnode** vpp) >>> } >>> strcpy(in_args.get(), name); >>> >>> - auto* strategy = >>> static_cast<fuse_strategy*>(vnode->v_mount->m_data); >>> - auto error = fuse_req_send_and_receive_reply(strategy, FUSE_LOOKUP, >>> + auto* drv = static_cast<virtio::fs*>(vnode->v_mount->m_data); >>> + auto error = fuse_req_send_and_receive_reply(drv, FUSE_LOOKUP, >>> inode->nodeid, in_args.get(), in_args_len, out_args.get(), >>> sizeof(*out_args)); >>> if (error) { >>> @@ -111,8 +110,8 @@ static int virtiofs_open(struct file* fp) >>> } >>> in_args->flags = OPEN_FLAGS; >>> >>> - auto* strategy = >>> static_cast<fuse_strategy*>(vnode->v_mount->m_data); >>> - auto error = fuse_req_send_and_receive_reply(strategy, FUSE_OPEN, >>> + auto* drv = static_cast<virtio::fs*>(vnode->v_mount->m_data); >>> + auto error = fuse_req_send_and_receive_reply(drv, FUSE_OPEN, >>> inode->nodeid, in_args.get(), sizeof(*in_args), out_args.get(), >>> sizeof(*out_args)); >>> if (error) { >>> @@ -146,8 +145,8 @@ static int virtiofs_close(struct vnode* vnode, >>> struct file* fp) >>> in_args->fh = f_data->file_handle; >>> in_args->flags = OPEN_FLAGS; // need to be same as in FUSE_OPEN >>> >>> - auto* strategy = >>> static_cast<fuse_strategy*>(vnode->v_mount->m_data); >>> - auto error = fuse_req_send_and_receive_reply(strategy, >>> FUSE_RELEASE, >>> + auto* drv = static_cast<virtio::fs*>(vnode->v_mount->m_data); >>> + auto error = fuse_req_send_and_receive_reply(drv, FUSE_RELEASE, >>> inode->nodeid, in_args.get(), sizeof(*in_args), nullptr, 0); >>> if (error) { >>> kprintf("[virtiofs] inode %lld, close failed\n", >>> inode->nodeid); >>> @@ -173,8 +172,8 @@ static int virtiofs_readlink(struct vnode* vnode, >>> struct uio* uio) >>> return ENOMEM; >>> } >>> >>> - auto* strategy = >>> static_cast<fuse_strategy*>(vnode->v_mount->m_data); >>> - auto error = fuse_req_send_and_receive_reply(strategy, >>> FUSE_READLINK, >>> + auto* drv = static_cast<virtio::fs*>(vnode->v_mount->m_data); >>> + auto error = fuse_req_send_and_receive_reply(drv, FUSE_READLINK, >>> inode->nodeid, nullptr, 0, link_path.get(), PATH_MAX); >>> if (error) { >>> kprintf("[virtiofs] inode %lld, readlink failed\n", >>> inode->nodeid); >>> @@ -188,10 +187,9 @@ static int virtiofs_readlink(struct vnode* vnode, >>> struct uio* uio) >>> >>> // Read @read_amt bytes from @inode, using the DAX window. >>> static int virtiofs_read_direct(virtiofs_inode& inode, u64 file_handle, >>> - u64 read_amt, fuse_strategy& strategy, struct uio& uio) >>> + u64 read_amt, virtio::fs& drv, struct uio& uio) >>> { >>> - auto* drv = static_cast<virtio::fs*>(strategy.drv); >>> - auto* dax = drv->get_dax(); >>> + auto* dax = drv.get_dax(); >>> // Enter the critical path: setup mapping -> read -> remove mapping >>> std::lock_guard<mutex> guard {dax->lock}; >>> >>> @@ -215,7 +213,7 @@ static int virtiofs_read_direct(virtiofs_inode& >>> inode, u64 file_handle, >>> uint64_t moffset = 0; >>> in_args->moffset = moffset; >>> >>> - auto map_align = drv->get_map_alignment(); >>> + auto map_align = drv.get_map_alignment(); >>> if (map_align < 0) { >>> kprintf("[virtiofs] inode %lld, map alignment not set\n", >>> inode.nodeid); >>> return ENOTSUP; >>> @@ -239,7 +237,7 @@ static int virtiofs_read_direct(virtiofs_inode& >>> inode, u64 file_handle, >>> virtiofs_debug("inode %lld, setting up mapping (foffset=%lld, >>> len=%lld, " >>> "moffset=%lld)\n", inode.nodeid, in_args->foffset, >>> in_args->len, in_args->moffset); >>> - auto error = fuse_req_send_and_receive_reply(&strategy, >>> FUSE_SETUPMAPPING, >>> + auto error = fuse_req_send_and_receive_reply(&drv, >>> FUSE_SETUPMAPPING, >>> inode.nodeid, in_args.get(), sizeof(*in_args), nullptr, 0); >>> if (error) { >>> kprintf("[virtiofs] inode %lld, mapping setup failed\n", >>> inode.nodeid); >>> @@ -277,7 +275,7 @@ static int virtiofs_read_direct(virtiofs_inode& >>> inode, u64 file_handle, >>> >>> virtiofs_debug("inode %lld, removing mapping (moffset=%lld, >>> len=%lld)\n", >>> inode.nodeid, r_one->moffset, r_one->len); >>> - error = fuse_req_send_and_receive_reply(&strategy, >>> FUSE_REMOVEMAPPING, >>> + error = fuse_req_send_and_receive_reply(&drv, FUSE_REMOVEMAPPING, >>> inode.nodeid, r_in_args.get(), r_in_args_size, nullptr, 0); >>> if (error) { >>> kprintf("[virtiofs] inode %lld, mapping removal failed\n", >>> @@ -290,7 +288,7 @@ static int virtiofs_read_direct(virtiofs_inode& >>> inode, u64 file_handle, >>> >>> // Read @read_amt bytes from @inode, using the fallback FUSE_READ >>> mechanism. >>> static int virtiofs_read_fallback(virtiofs_inode& inode, u64 >>> file_handle, >>> - u32 read_amt, u32 flags, fuse_strategy& strategy, struct uio& uio) >>> + u32 read_amt, u32 flags, virtio::fs& drv, struct uio& uio) >>> { >>> std::unique_ptr<fuse_read_in> in_args {new (std::nothrow) >>> fuse_read_in()}; >>> std::unique_ptr<void, std::function<void(void*)>> buf { >>> @@ -306,7 +304,7 @@ static int virtiofs_read_fallback(virtiofs_inode& >>> inode, u64 file_handle, >>> >>> virtiofs_debug("inode %lld, reading %lld bytes at offset %lld\n", >>> inode.nodeid, read_amt, uio.uio_offset); >>> - auto error = fuse_req_send_and_receive_reply(&strategy, FUSE_READ, >>> + auto error = fuse_req_send_and_receive_reply(&drv, FUSE_READ, >>> inode.nodeid, in_args.get(), sizeof(*in_args), buf.get(), >>> read_amt); >>> if (error) { >>> kprintf("[virtiofs] inode %lld, read failed\n", inode.nodeid); >>> @@ -345,24 +343,23 @@ static int virtiofs_read(struct vnode* vnode, >>> struct file* fp, struct uio* uio, >>> >>> auto* inode = static_cast<virtiofs_inode*>(vnode->v_data); >>> auto* file_data = static_cast<virtiofs_file_data*>(fp->f_data); >>> - auto* strategy = >>> static_cast<fuse_strategy*>(vnode->v_mount->m_data); >>> + auto* drv = static_cast<virtio::fs*>(vnode->v_mount->m_data); >>> >>> // Total read amount is what they requested, or what is left >>> auto read_amt = std::min<uint64_t>(uio->uio_resid, >>> inode->attr.size - uio->uio_offset); >>> >>> - auto* drv = static_cast<virtio::fs*>(strategy->drv); >>> if (drv->get_dax()) { >>> // Try to read from DAX >>> if (!virtiofs_read_direct(*inode, file_data->file_handle, >>> read_amt, >>> - *strategy, *uio)) { >>> + *drv, *uio)) { >>> >>> return 0; >>> } >>> } >>> // DAX unavailable or failed, use fallback >>> return virtiofs_read_fallback(*inode, file_data->file_handle, >>> read_amt, >>> - ioflag, *strategy, *uio); >>> + ioflag, *drv, *uio); >>> } >>> >>> static int virtiofs_readdir(struct vnode* vnode, struct file* fp, >>> -- >>> 2.26.2 >>> >>> -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/c38d0f7c-0856-485c-adee-b626c0fd356c%40googlegroups.com.
