All of lore.kernel.org
 help / color / mirror / Atom feed
From: Max Reitz <mreitz@redhat.com>
To: qemu-devel@nongnu.org, virtio-fs@redhat.com
Cc: Stefan Hajnoczi <stefanha@redhat.com>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
	Vivek Goyal <vgoyal@redhat.com>, Max Reitz <mreitz@redhat.com>
Subject: [PATCH v3 00/10] virtiofsd: Allow using file handles instead of O_PATH FDs
Date: Fri, 30 Jul 2021 17:01:24 +0200	[thread overview]
Message-ID: <20210730150134.216126-1-mreitz@redhat.com> (raw)

Hi,

v1 cover letter for an overview:
https://listman.redhat.com/archives/virtio-fs/2021-June/msg00033.html

v2 cover letter:
https://listman.redhat.com/archives/virtio-fs/2021-June/msg00074.html

For v3, at first I attempted to have errors related to file handle
generation (name_to_handle_at()) be returned to the guest unless they
are cases where file name generation is simply not supported, and only
then do a fallback to an O_PATH FD, as Vivek has suggested.

However, I found that to be rather complicated.  (Always falling back is
just simpler.)  Furthermore, because we believe that name_to_handle_at()
can rarely fail except for EOPNOTSUPP, there should be little difference
in practice.

Therefore, in v3, I kept the v2 model of always falling back to an
O_PATH FD when an error occurred during handle generation.

What did change in v3 is the following:
- I added patch 1, because f1aa1774dfb happened in the meantime, and
  this is basically what we did for virtiofsd-rs in the form of
  31e7ac63944 (virtiofsd-rs commit hash)

- Patch 4: In lookup_name(), I noticed that I failed to invoke
  lo_inode_put() to match the lo_inode() from the beginning of the
  function in all error paths.  Fixed by adding a common error path.

- Patch 6: Mostly contextual rebase conflicts (partly because of patch
  1), but also one functional change: I Dropped the `assert(fd >= 0)`
  under `if (open_inode)` in lo_setxattr(), because `fd` is dropped by
  this patch (and `inode_fd` is used regardless of the value of
  `open_inode` we can’t assert anything similar on it).

- Patch 8:
  - Fixed the condition to reject results found by st_ino lookup.
    - st_ino on its own is only a valid identifier/key if we have an
      O_PATH fd for its respective lo_inode, because otherwise the inode
      may be unlinked and its st_ino might be reused by some new inode
    - It does not matter whether lo_find()’s caller has supplied a file
      handle for a prior lookup by handle or not, so drop that part of
      the condition
    - Semantically, it does not matter whether the lo_inode has a file
      handle or not – what matters is whether it has an O_PATH fd or
      not.  (The two are linked by a `handle <=> !fd` condition, so that
      part wasn’t technically wrong, just semantically.)
    - In accordance with the last point, I rewrote the comment
      explaining why we have to reject such results.
  - Rebase conflict in lookup_name() because of the fix in patch 4

- Patch 9:
  - Non-functional change in lo_do_lookup() to separate the
    get_file_handle()/openat() part from the do_statx() calls (and have
    the do_statx() calls be side by side) – as a side effect, this makes
    the diff to master slightly smaller.
  - Rebase conflict in lookup_name() because of the fix in patch 4

- Patch 10:
  - Rebase conflict in lookup_name() because of the fix in patch 4


Max Reitz (10):
  virtiofsd: Limit setxattr()'s creds-dropped region
  virtiofsd: Add TempFd structure
  virtiofsd: Use lo_inode_open() instead of openat()
  virtiofsd: Add lo_inode_fd() helper
  virtiofsd: Let lo_fd() return a TempFd
  virtiofsd: Let lo_inode_open() return a TempFd
  virtiofsd: Add lo_inode.fhandle
  virtiofsd: Add inodes_by_handle hash table
  virtiofsd: Optionally fill lo_inode.fhandle
  virtiofsd: Add lazy lo_do_find()

 tools/virtiofsd/helper.c              |   3 +
 tools/virtiofsd/passthrough_ll.c      | 869 +++++++++++++++++++++-----
 tools/virtiofsd/passthrough_seccomp.c |   2 +
 3 files changed, 720 insertions(+), 154 deletions(-)

-- 
2.31.1



WARNING: multiple messages have this Message-ID (diff)
From: Max Reitz <mreitz@redhat.com>
To: qemu-devel@nongnu.org, virtio-fs@redhat.com
Cc: Vivek Goyal <vgoyal@redhat.com>, Max Reitz <mreitz@redhat.com>
Subject: [Virtio-fs] [PATCH v3 00/10] virtiofsd: Allow using file handles instead of O_PATH FDs
Date: Fri, 30 Jul 2021 17:01:24 +0200	[thread overview]
Message-ID: <20210730150134.216126-1-mreitz@redhat.com> (raw)

Hi,

v1 cover letter for an overview:
https://listman.redhat.com/archives/virtio-fs/2021-June/msg00033.html

v2 cover letter:
https://listman.redhat.com/archives/virtio-fs/2021-June/msg00074.html

For v3, at first I attempted to have errors related to file handle
generation (name_to_handle_at()) be returned to the guest unless they
are cases where file name generation is simply not supported, and only
then do a fallback to an O_PATH FD, as Vivek has suggested.

However, I found that to be rather complicated.  (Always falling back is
just simpler.)  Furthermore, because we believe that name_to_handle_at()
can rarely fail except for EOPNOTSUPP, there should be little difference
in practice.

Therefore, in v3, I kept the v2 model of always falling back to an
O_PATH FD when an error occurred during handle generation.

What did change in v3 is the following:
- I added patch 1, because f1aa1774dfb happened in the meantime, and
  this is basically what we did for virtiofsd-rs in the form of
  31e7ac63944 (virtiofsd-rs commit hash)

- Patch 4: In lookup_name(), I noticed that I failed to invoke
  lo_inode_put() to match the lo_inode() from the beginning of the
  function in all error paths.  Fixed by adding a common error path.

- Patch 6: Mostly contextual rebase conflicts (partly because of patch
  1), but also one functional change: I Dropped the `assert(fd >= 0)`
  under `if (open_inode)` in lo_setxattr(), because `fd` is dropped by
  this patch (and `inode_fd` is used regardless of the value of
  `open_inode` we can’t assert anything similar on it).

- Patch 8:
  - Fixed the condition to reject results found by st_ino lookup.
    - st_ino on its own is only a valid identifier/key if we have an
      O_PATH fd for its respective lo_inode, because otherwise the inode
      may be unlinked and its st_ino might be reused by some new inode
    - It does not matter whether lo_find()’s caller has supplied a file
      handle for a prior lookup by handle or not, so drop that part of
      the condition
    - Semantically, it does not matter whether the lo_inode has a file
      handle or not – what matters is whether it has an O_PATH fd or
      not.  (The two are linked by a `handle <=> !fd` condition, so that
      part wasn’t technically wrong, just semantically.)
    - In accordance with the last point, I rewrote the comment
      explaining why we have to reject such results.
  - Rebase conflict in lookup_name() because of the fix in patch 4

- Patch 9:
  - Non-functional change in lo_do_lookup() to separate the
    get_file_handle()/openat() part from the do_statx() calls (and have
    the do_statx() calls be side by side) – as a side effect, this makes
    the diff to master slightly smaller.
  - Rebase conflict in lookup_name() because of the fix in patch 4

- Patch 10:
  - Rebase conflict in lookup_name() because of the fix in patch 4


Max Reitz (10):
  virtiofsd: Limit setxattr()'s creds-dropped region
  virtiofsd: Add TempFd structure
  virtiofsd: Use lo_inode_open() instead of openat()
  virtiofsd: Add lo_inode_fd() helper
  virtiofsd: Let lo_fd() return a TempFd
  virtiofsd: Let lo_inode_open() return a TempFd
  virtiofsd: Add lo_inode.fhandle
  virtiofsd: Add inodes_by_handle hash table
  virtiofsd: Optionally fill lo_inode.fhandle
  virtiofsd: Add lazy lo_do_find()

 tools/virtiofsd/helper.c              |   3 +
 tools/virtiofsd/passthrough_ll.c      | 869 +++++++++++++++++++++-----
 tools/virtiofsd/passthrough_seccomp.c |   2 +
 3 files changed, 720 insertions(+), 154 deletions(-)

-- 
2.31.1


             reply	other threads:[~2021-07-30 15:07 UTC|newest]

Thread overview: 88+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-30 15:01 Max Reitz [this message]
2021-07-30 15:01 ` [Virtio-fs] [PATCH v3 00/10] virtiofsd: Allow using file handles instead of O_PATH FDs Max Reitz
2021-07-30 15:01 ` [PATCH v3 01/10] virtiofsd: Limit setxattr()'s creds-dropped region Max Reitz
2021-07-30 15:01   ` [Virtio-fs] " Max Reitz
2021-08-06 14:16   ` Vivek Goyal
2021-08-06 14:16     ` [Virtio-fs] " Vivek Goyal
2021-08-09 10:30     ` Max Reitz
2021-08-09 10:30       ` [Virtio-fs] " Max Reitz
2021-07-30 15:01 ` [PATCH v3 02/10] virtiofsd: Add TempFd structure Max Reitz
2021-07-30 15:01   ` [Virtio-fs] " Max Reitz
2021-08-06 14:41   ` Vivek Goyal
2021-08-06 14:41     ` [Virtio-fs] " Vivek Goyal
2021-08-09 10:44     ` Max Reitz
2021-08-09 10:44       ` [Virtio-fs] " Max Reitz
2021-07-30 15:01 ` [PATCH v3 03/10] virtiofsd: Use lo_inode_open() instead of openat() Max Reitz
2021-07-30 15:01   ` [Virtio-fs] " Max Reitz
2021-08-06 15:42   ` Vivek Goyal
2021-08-06 15:42     ` [Virtio-fs] " Vivek Goyal
2021-07-30 15:01 ` [PATCH v3 04/10] virtiofsd: Add lo_inode_fd() helper Max Reitz
2021-07-30 15:01   ` [Virtio-fs] " Max Reitz
2021-08-06 18:25   ` Vivek Goyal
2021-08-06 18:25     ` [Virtio-fs] " Vivek Goyal
2021-08-09 10:48     ` Max Reitz
2021-08-09 10:48       ` [Virtio-fs] " Max Reitz
2021-07-30 15:01 ` [PATCH v3 05/10] virtiofsd: Let lo_fd() return a TempFd Max Reitz
2021-07-30 15:01   ` [Virtio-fs] " Max Reitz
2021-07-30 15:01 ` [PATCH v3 06/10] virtiofsd: Let lo_inode_open() " Max Reitz
2021-07-30 15:01   ` [Virtio-fs] " Max Reitz
2021-08-06 19:55   ` Vivek Goyal
2021-08-06 19:55     ` [Virtio-fs] " Vivek Goyal
2021-08-09 13:40     ` Max Reitz
2021-08-09 13:40       ` [Virtio-fs] " Max Reitz
2021-07-30 15:01 ` [PATCH v3 07/10] virtiofsd: Add lo_inode.fhandle Max Reitz
2021-07-30 15:01   ` [Virtio-fs] " Max Reitz
2021-08-09 15:21   ` Vivek Goyal
2021-08-09 15:21     ` [Virtio-fs] " Vivek Goyal
2021-08-09 16:41     ` Hanna Reitz
2021-08-09 16:41       ` [Virtio-fs] " Hanna Reitz
2021-07-30 15:01 ` [PATCH v3 08/10] virtiofsd: Add inodes_by_handle hash table Max Reitz
2021-07-30 15:01   ` [Virtio-fs] " Max Reitz
2021-08-09 16:10   ` Vivek Goyal
2021-08-09 16:10     ` [Virtio-fs] " Vivek Goyal
2021-08-09 16:47     ` Hanna Reitz
2021-08-09 16:47       ` [Virtio-fs] " Hanna Reitz
2021-08-10 14:07       ` Vivek Goyal
2021-08-10 14:07         ` [Virtio-fs] " Vivek Goyal
2021-08-10 14:13         ` Hanna Reitz
2021-08-10 14:13           ` [Virtio-fs] " Hanna Reitz
2021-08-10 17:51           ` Vivek Goyal
2021-08-10 17:51             ` [Virtio-fs] " Vivek Goyal
2021-07-30 15:01 ` [PATCH v3 09/10] virtiofsd: Optionally fill lo_inode.fhandle Max Reitz
2021-07-30 15:01   ` [Virtio-fs] " Max Reitz
2021-08-09 18:41   ` Vivek Goyal
2021-08-09 18:41     ` [Virtio-fs] " Vivek Goyal
2021-08-10  8:32     ` Hanna Reitz
2021-08-10  8:32       ` [Virtio-fs] " Hanna Reitz
2021-08-10 15:23       ` Vivek Goyal
2021-08-10 15:23         ` [Virtio-fs] " Vivek Goyal
2021-08-10 15:26         ` Hanna Reitz
2021-08-10 15:26           ` [Virtio-fs] " Hanna Reitz
2021-08-10 15:57           ` Vivek Goyal
2021-08-10 15:57             ` [Virtio-fs] " Vivek Goyal
2021-08-11  6:41             ` Hanna Reitz
2021-08-11  6:41               ` [Virtio-fs] " Hanna Reitz
2021-08-16 19:44               ` Vivek Goyal
2021-08-16 19:44                 ` [Virtio-fs] " Vivek Goyal
2021-08-17  8:27                 ` Hanna Reitz
2021-08-17  8:27                   ` [Virtio-fs] " Hanna Reitz
2021-08-17 19:45                   ` Vivek Goyal
2021-08-17 19:45                     ` [Virtio-fs] " Vivek Goyal
2021-08-18  0:14                     ` Vivek Goyal
2021-08-18  0:14                       ` [Virtio-fs] " Vivek Goyal
2021-08-18 13:32                       ` Vivek Goyal
2021-08-18 13:32                         ` [Virtio-fs] " Vivek Goyal
2021-08-18 13:48                         ` Hanna Reitz
2021-08-18 13:48                           ` [Virtio-fs] " Hanna Reitz
2021-08-19 16:38   ` Dr. David Alan Gilbert
2021-08-19 16:38     ` [Virtio-fs] " Dr. David Alan Gilbert
2021-07-30 15:01 ` [PATCH v3 10/10] virtiofsd: Add lazy lo_do_find() Max Reitz
2021-07-30 15:01   ` [Virtio-fs] " Max Reitz
2021-08-09 19:08   ` Vivek Goyal
2021-08-09 19:08     ` [Virtio-fs] " Vivek Goyal
2021-08-10  8:38     ` Hanna Reitz
2021-08-10  8:38       ` [Virtio-fs] " Hanna Reitz
2021-08-10 14:12       ` Vivek Goyal
2021-08-10 14:12         ` [Virtio-fs] " Vivek Goyal
2021-08-10 14:17         ` Hanna Reitz
2021-08-10 14:17           ` [Virtio-fs] " Hanna Reitz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210730150134.216126-1-mreitz@redhat.com \
    --to=mreitz@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanha@redhat.com \
    --cc=vgoyal@redhat.com \
    --cc=virtio-fs@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.