This patchset adds support of per-file DAX for virtiofs, which is
inspired by Ira Weiny's work on ext4[1] and xfs[2].
Any comment is welcome.
[1] commit 9cb20f94afcd ("fs/ext4: Make DAX mount option a tri-state")
[2] commit 02beb2686ff9 ("fs/xfs: Make DAX mount option a tri-state")
changes since v3:
- bug fix (patch 6): s/"IS_DAX(inode) != newdax"/"!!IS_DAX(inode) != newdax"
- during FUSE_INIT, advertise capability for per-file DAX only when
mounted as "-o dax=inode" (patch 4)
changes since v2:
- modify fuse_show_options() accordingly to make it compatible with
new tri-state mount option (patch 2)
- extract FUSE protocol changes into one seperate patch (patch 3)
- FUSE server/client need to negotiate if they support per-file DAX
(patch 4)
- extract DONT_CACHE logic into patch 6/7
v3: https://www.spinics.net/lists/linux-fsdevel/msg200852.html
v2: https://www.spinics.net/lists/linux-fsdevel/msg199584.html
v1: https://www.spinics.net/lists/linux-virtualization/msg51008.html
Jeffle Xu (8):
fuse: add fuse_should_enable_dax() helper
fuse: Make DAX mount option a tri-state
fuse: support per-file DAX
fuse: negotiate if server/client supports per-file DAX
fuse: enable per-file DAX
fuse: mark inode DONT_CACHE when per-file DAX indication changes
fuse: support changing per-file DAX flag inside guest
fuse: show '-o dax=inode' option only when FUSE server supports
fs/fuse/dax.c | 32 +++++++++++++++++++++++++++++---
fs/fuse/file.c | 4 ++--
fs/fuse/fuse_i.h | 22 ++++++++++++++++++----
fs/fuse/inode.c | 27 +++++++++++++++++++--------
fs/fuse/ioctl.c | 15 +++++++++++++--
fs/fuse/virtio_fs.c | 16 ++++++++++++++--
include/uapi/linux/fuse.h | 9 ++++++++-
7 files changed, 103 insertions(+), 22 deletions(-)
--
2.27.0
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
This is in prep for following per-file DAX checking.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
fs/fuse/dax.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
We add 'always', 'never', and 'inode' (default). '-o dax' continues to
operate the same which is equivalent to 'always'.
By the time this patch is applied, 'inode' mode is actually equal to
'always' mode, before the per-file DAX flag is introduced in the
following patch.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
fs/fuse/dax.c | 9 +++++++--
fs/fuse/fuse_i.h | 14 ++++++++++++--
fs/fuse/inode.c | 10 +++++++---
fs/fuse/virtio_fs.c | 16 ++++++++++++++--
4 files changed, 40 insertions(+), 9 deletions(-)
@@ -110,7 +119,10 @@ static int virtio_fs_parse_param(struct fs_context *fc,switch(opt){caseOPT_DAX:-ctx->dax=1;+ctx->dax_mode=FUSE_DAX_ALWAYS;+break;+caseOPT_DAX_ENUM:+ctx->dax_mode=result.uint_32;break;default:return-EINVAL;
@@ -1323,7 +1335,7 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)/* virtiofs allocates and installs its own fuse devices */ctx->fudptr=NULL;-if(ctx->dax){+if(ctx->dax_mode!=FUSE_DAX_NEVER){if(!fs->dax_dev){err=-EINVAL;pr_err("virtio-fs: dax can't be enabled as filesystem"
--
2.27.0
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Expand the fuse protocol to support per-file DAX.
FUSE_PERFILE_DAX flag is added indicating if fuse server/client
supporting per-file DAX when sending or replying FUSE_INIT request.
Besides, FUSE_ATTR_DAX flag is added indicating if DAX shall be enabled
for corresponding file when replying FUSE_LOOKUP request.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
include/uapi/linux/fuse.h | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
@@ -219,7 +222,7 @@#define FUSE_KERNEL_VERSION 7/** Minor version number of this interface */-#define FUSE_KERNEL_MINOR_VERSION 34+#define FUSE_KERNEL_MINOR_VERSION 35/** The node ID of the root inode */#define FUSE_ROOT_ID 1
@@ -770,6 +770,9 @@ struct fuse_conn {/* Propagate syncfs() to server */unsignedintsync_fs:1;+/* Does the filesystem support per-file DAX? */+unsignedintperfile_dax:1;+/** The number of requests waiting for completion */atomic_tnum_waiting;
Enable per-file DAX if fuse server advertises that the file supports
that.
Currently the state whether the file enables DAX or not is initialized
only when inode is instantiated.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
fs/fuse/dax.c | 12 ++++++++----
fs/fuse/file.c | 4 ++--
fs/fuse/fuse_i.h | 4 ++--
fs/fuse/inode.c | 2 +-
4 files changed, 13 insertions(+), 9 deletions(-)
Fuse client can enable or disable per-file DAX inside kernel/guest by
chattr(1). Similarly the new state won't be updated until the file is
closed and reopened later.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
fs/fuse/ioctl.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
When the per-file DAX indication changes while the file is still
*opened*, it is quite complicated and maybe fragile to dynamically
change the DAX state.
Hence mark the inode and corresponding dentries as DONE_CACHE once the
per-file DAX indication changes, so that the inode instance will be
evicted and freed as soon as possible once the file is closed and the
last reference to the inode is put. And then when the file gets reopened
next time, the inode will reflect the new DAX state.
In summary, when the per-file DAX indication changes for an *opened*
file, the state of the file won't be updated until this file is closed
and reopened later.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
fs/fuse/dax.c | 9 +++++++++
fs/fuse/fuse_i.h | 1 +
fs/fuse/inode.c | 3 +++
3 files changed, 13 insertions(+)
Prior of this patch, the mount option will still show '-o dax=inode'
when FUSE server advertises that it doesn't support per-file DAX.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
fs/fuse/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
I mentioned in virtiofsd PATCH v1 that virtiofsd exits once ioctl() is
called. After depper investigation into this issue, I find that it is
because ioctl() is blocked out the whitelist of seccomp of virtiofsd.
To support ioctl, ioctl syscall shall be added into the whitelist (see
patch
1).
And this is the complete workable version for virtiofsd:
- virtiofsd now supports FUSE_IOCTL now, though currently only
FS_IOC_G[S]ETFLAGS/FS_IOC_FSG[S]ETXATTR are supported.
- During FUSE_INIT, virtiofsd advertise support for per-file DAX only
when the backend fs is ext4/xfs.
- FS_IOC_SETFLAGS/FS_IOC_FSSETXATTR FUSE_IOCTL will be directed to host,
so that FS_DAX_FL could be flushed to backed fs persistently.
- During FUSE_LOOKUP, virtiofsd will decide DAX shall be enabled for
current file according to if this file is marked with FS_DAX_FL in the
backend fs.
changes since v2/v3:
Patch 4 in v2 is incomplete by mistake and it will fail to be compiled.
I had ever sent a seperate patch 4 of v3. Now I send the whole complete
set in v4. Except for this, there's no other diferrence.
Jeffle Xu (4):
virtiofsd: add .ioctl() support
virtiofsd: expand fuse protocol to support per-file DAX
virtiofsd: support per-file DAX negotiation in FUSE_INIT
virtiofsd: support per-file DAX in FUSE_LOOKUP
include/standard-headers/linux/fuse.h | 2 +
tools/virtiofsd/fuse_common.h | 5 ++
tools/virtiofsd/fuse_lowlevel.c | 6 ++
tools/virtiofsd/passthrough_ll.c | 125 ++++++++++++++++++++++++++
tools/virtiofsd/passthrough_seccomp.c | 1 +
5 files changed, 139 insertions(+)
--
2.27.0
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Add .ioctl() support for passthrough, in prep for the following support
for following per-file DAX feature.
Once advertising support for per-file DAX feature, virtiofsd should
support storing FS_DAX_FL flag persistently passed by
FS_IOC_SETFLAGS/FS_IOC_FSSETXATTR ioctl, and set FUSE_ATTR_DAX in
FUSE_LOOKUP accordingly if the file is capable of per-file DAX.
When it comes to passthrough, it passes corresponding ioctls to host
directly. Currently only these ioctls that are needed for per-file DAX
feature, i.e., FS_IOC_GETFLAGS/FS_IOC_SETFLAGS and
FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR are supported. Later we can restrict
the flags/attributes allowed to be set to reinforce the security, or
extend the scope of allowed ioctls if it is really needed later.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
tools/virtiofsd/passthrough_ll.c | 53 +++++++++++++++++++++++++++
tools/virtiofsd/passthrough_seccomp.c | 1 +
2 files changed, 54 insertions(+)
In FUSE_INIT negotiating phase, server/client should advertise if it
supports per-file DAX.
Once advertising support for per-file DAX feature, virtiofsd should
support storing FS_DAX_FL flag persistently passed by
FS_IOC_SETFLAGS/FS_IOC_FSSETXATTR ioctl, and set FUSE_ATTR_DAX in
FUSE_LOOKUP accordingly if the file is capable of per-file DAX.
Currently only ext4/xfs since linux kernel v5.8 support storing
FS_DAX_FL flag persistently, and thus advertise support for per-file
DAX feature only when the backend fs type is ext4 and xfs.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
tools/virtiofsd/fuse_common.h | 5 +++++
tools/virtiofsd/fuse_lowlevel.c | 6 ++++++
tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++++++++++
3 files changed, 40 insertions(+)
For passthrough, when the corresponding virtiofs in guest is mounted
with '-o dax=inode', advertise that the file is capable of per-file
DAX if the inode in the backend fs is marked with FS_DAX_FL flag.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
tools/virtiofsd/passthrough_ll.c | 43 ++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
@@ -983,6 +988,41 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,return0;}+/*+*IfthefileismarkedwithFS_DAX_FLorFS_XFLAG_DAX,thenDAXshouldbe+*enabledforthisfile.+*/+staticboollo_should_enable_dax(structlo_data*lo,structlo_inode*dir,+constchar*name)+{+intres,fd;+intret=false;;+unsignedintattr;+structfsxattrxattr;++if(!lo->perfile_dax)+returnfalse;++/* Open file without O_PATH, so that ioctl can be called. */+fd=openat(dir->fd,name,O_NOFOLLOW);+if(fd==-1)+returnfalse;++if(lo->perfile_dax_cap==DAX_CAP_FLAGS){+res=ioctl(fd,FS_IOC_GETFLAGS,&attr);+if(!res&&(attr&FS_DAX_FL))+ret=true;+}+elseif(lo->perfile_dax_cap==DAX_CAP_XATTR){+res=ioctl(fd,FS_IOC_FSGETXATTR,&xattr);+if(!res&&(xattr.fsx_xflags&FS_XFLAG_DAX))+ret=true;+}++close(fd);+returnret;+}+/**Incrementsnlookupontheinodeonsuccess.unref_inode_lolocked()mustbe*calledeventuallytodecrementnlookupagain.Ifinodepisnon-NULL,the
From: Dr. David Alan Gilbert <hidden> Date: 2021-08-17 10:26:51
* Jeffle Xu (jefflexu@linux.alibaba.com) wrote:
When the per-file DAX indication changes while the file is still
*opened*, it is quite complicated and maybe fragile to dynamically
change the DAX state.
Hence mark the inode and corresponding dentries as DONE_CACHE once the
^^^^^^^^^^
typo as DONT ?
Dave
quoted hunk
per-file DAX indication changes, so that the inode instance will be
evicted and freed as soon as possible once the file is closed and the
last reference to the inode is put. And then when the file gets reopened
next time, the inode will reflect the new DAX state.
In summary, when the per-file DAX indication changes for an *opened*
file, the state of the file won't be updated until this file is closed
and reopened later.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
fs/fuse/dax.c | 9 +++++++++
fs/fuse/fuse_i.h | 1 +
fs/fuse/inode.c | 3 +++
3 files changed, 13 insertions(+)
When the per-file DAX indication changes while the file is still
*opened*, it is quite complicated and maybe fragile to dynamically
change the DAX state.
Hence mark the inode and corresponding dentries as DONE_CACHE once the
^^^^^^^^^^
typo as DONT ?
Thanks. I will fix it.
quoted
per-file DAX indication changes, so that the inode instance will be
evicted and freed as soon as possible once the file is closed and the
last reference to the inode is put. And then when the file gets reopened
next time, the inode will reflect the new DAX state.
In summary, when the per-file DAX indication changes for an *opened*
file, the state of the file won't be updated until this file is closed
and reopened later.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
fs/fuse/dax.c | 9 +++++++++
fs/fuse/fuse_i.h | 1 +
fs/fuse/inode.c | 3 +++
3 files changed, 13 insertions(+)
From: Dr. David Alan Gilbert <hidden> Date: 2021-08-17 17:16:05
* Jeffle Xu (jefflexu@linux.alibaba.com) wrote:
In FUSE_INIT negotiating phase, server/client should advertise if it
supports per-file DAX.
Once advertising support for per-file DAX feature, virtiofsd should
support storing FS_DAX_FL flag persistently passed by
FS_IOC_SETFLAGS/FS_IOC_FSSETXATTR ioctl, and set FUSE_ATTR_DAX in
FUSE_LOOKUP accordingly if the file is capable of per-file DAX.
Currently only ext4/xfs since linux kernel v5.8 support storing
FS_DAX_FL flag persistently, and thus advertise support for per-file
DAX feature only when the backend fs type is ext4 and xfs.
I'm a little worried about the meaning of the flags we're storing and
the fact we're storing them in the normal host DAX flags.
Doesn't this mean that we're using a single host flag to mean:
a) It can be mapped as DAX on the host if it was a real DAX device
b) We can map it as DAX inside the guest with virtiofs?
what happens when we're using usernamespaces for the guest?
Dave
From: Dr. David Alan Gilbert <hidden> Date: 2021-08-17 19:00:32
* Jeffle Xu (jefflexu@linux.alibaba.com) wrote:
quoted hunk
For passthrough, when the corresponding virtiofs in guest is mounted
with '-o dax=inode', advertise that the file is capable of per-file
DAX if the inode in the backend fs is marked with FS_DAX_FL flag.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
tools/virtiofsd/passthrough_ll.c | 43 ++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
@@ -983,6 +988,41 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,return0;}+/*+*IfthefileismarkedwithFS_DAX_FLorFS_XFLAG_DAX,thenDAXshouldbe+*enabledforthisfile.+*/+staticboollo_should_enable_dax(structlo_data*lo,structlo_inode*dir,+constchar*name)+{+intres,fd;+intret=false;;+unsignedintattr;+structfsxattrxattr;++if(!lo->perfile_dax)+returnfalse;++/* Open file without O_PATH, so that ioctl can be called. */+fd=openat(dir->fd,name,O_NOFOLLOW);+if(fd==-1)+returnfalse;
Doesn't that defeat the whole benefit of using O_PATH - i.e. that we
might stumble into a /dev node or something else we're not allowed to
open?
quoted hunk
+ if (lo->perfile_dax_cap == DAX_CAP_FLAGS) {+ res = ioctl(fd, FS_IOC_GETFLAGS, &attr);+ if (!res && (attr & FS_DAX_FL))+ ret = true;+ }+ else if (lo->perfile_dax_cap == DAX_CAP_XATTR) {+ res = ioctl(fd, FS_IOC_FSGETXATTR, &xattr);+ if (!res && (xattr.fsx_xflags & FS_XFLAG_DAX))+ ret = true;+ }
This all looks pretty expensive for each lookup.
Dave
quoted hunk
+ close(fd);+ return ret;+}+ /* * Increments nlookup on the inode on success. unref_inode_lolocked() must be * called eventually to decrement nlookup again. If inodep is non-NULL, the
In FUSE_INIT negotiating phase, server/client should advertise if it
supports per-file DAX.
Once advertising support for per-file DAX feature, virtiofsd should
support storing FS_DAX_FL flag persistently passed by
FS_IOC_SETFLAGS/FS_IOC_FSSETXATTR ioctl, and set FUSE_ATTR_DAX in
FUSE_LOOKUP accordingly if the file is capable of per-file DAX.
Currently only ext4/xfs since linux kernel v5.8 support storing
FS_DAX_FL flag persistently, and thus advertise support for per-file
DAX feature only when the backend fs type is ext4 and xfs.
I'm a little worried about the meaning of the flags we're storing and
the fact we're storing them in the normal host DAX flags.
Doesn't this mean that we're using a single host flag to mean:
a) It can be mapped as DAX on the host if it was a real DAX device
b) We can map it as DAX inside the guest with virtiofs?
Yes the side effect is that the host file is also dax enabled if the
backend fs is built upon real nvdimm device.
The rationale here is that, fuse daemon shall be capable of *marking*
the file as dax capable *persistently*, so that it can be informed that
this file is capable of dax later.
I'm not sure if xattr (extent attribute) is a better option for this?
what happens when we're using usernamespaces for the guest?
Dave
For passthrough, when the corresponding virtiofs in guest is mounted
with '-o dax=inode', advertise that the file is capable of per-file
DAX if the inode in the backend fs is marked with FS_DAX_FL flag.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
tools/virtiofsd/passthrough_ll.c | 43 ++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
@@ -983,6 +988,41 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,return0;}+/*+*IfthefileismarkedwithFS_DAX_FLorFS_XFLAG_DAX,thenDAXshouldbe+*enabledforthisfile.+*/+staticboollo_should_enable_dax(structlo_data*lo,structlo_inode*dir,+constchar*name)+{+intres,fd;+intret=false;;+unsignedintattr;+structfsxattrxattr;++if(!lo->perfile_dax)+returnfalse;++/* Open file without O_PATH, so that ioctl can be called. */+fd=openat(dir->fd,name,O_NOFOLLOW);+if(fd==-1)+returnfalse;
Doesn't that defeat the whole benefit of using O_PATH - i.e. that we
might stumble into a /dev node or something else we're not allowed to
open?
As far as I know, virtiofsd will pivot_root/chroot to the source
directory, and can only access files inside the source directory
specified by "-o source=". Then where do these unexpected files come
from? Besides, fd opened without O_PATH here is temporary and used for
FS_IOC_GETFLAGS/FS_IOC_FSGETXATTR ioctl only. It's closed when the
function returns.
quoted
+ if (lo->perfile_dax_cap == DAX_CAP_FLAGS) {+ res = ioctl(fd, FS_IOC_GETFLAGS, &attr);+ if (!res && (attr & FS_DAX_FL))+ ret = true;+ }+ else if (lo->perfile_dax_cap == DAX_CAP_XATTR) {+ res = ioctl(fd, FS_IOC_FSGETXATTR, &xattr);+ if (!res && (xattr.fsx_xflags & FS_XFLAG_DAX))+ ret = true;+ }
This all looks pretty expensive for each lookup.
Yes. it can be somehow optimized if we can agree on the way of storing
the dax flag persistently.
--
Thanks,
Jeffle
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Tue, Aug 17, 2021 at 06:15:58PM +0100, Dr. David Alan Gilbert wrote:
* Jeffle Xu (jefflexu@linux.alibaba.com) wrote:
quoted
In FUSE_INIT negotiating phase, server/client should advertise if it
supports per-file DAX.
Once advertising support for per-file DAX feature, virtiofsd should
support storing FS_DAX_FL flag persistently passed by
FS_IOC_SETFLAGS/FS_IOC_FSSETXATTR ioctl, and set FUSE_ATTR_DAX in
FUSE_LOOKUP accordingly if the file is capable of per-file DAX.
Currently only ext4/xfs since linux kernel v5.8 support storing
FS_DAX_FL flag persistently, and thus advertise support for per-file
DAX feature only when the backend fs type is ext4 and xfs.
I'm a little worried about the meaning of the flags we're storing and
the fact we're storing them in the normal host DAX flags.
Doesn't this mean that we're using a single host flag to mean:
a) It can be mapped as DAX on the host if it was a real DAX device
b) We can map it as DAX inside the guest with virtiofs?
That's how passthrough filesystem is. Every attribute is passthrough.
So if guest sets something, host sees it same way. (file uid/gid,
file mode bits, xattrs etc.). Only exception now seems to be remapping
of xattrs if users choses to do so.
what happens when we're using usernamespaces for the guest?
I don't think file attrs are namespaced. So if virtiofsd has permission
do to so, it will be just able to set attrs on file.
Vivek
On Tue, Aug 17, 2021 at 10:23:44AM +0800, Jeffle Xu wrote:
Add .ioctl() support for passthrough, in prep for the following support
for following per-file DAX feature.
Once advertising support for per-file DAX feature, virtiofsd should
support storing FS_DAX_FL flag persistently passed by
FS_IOC_SETFLAGS/FS_IOC_FSSETXATTR ioctl, and set FUSE_ATTR_DAX in
FUSE_LOOKUP accordingly if the file is capable of per-file DAX.
When it comes to passthrough, it passes corresponding ioctls to host
directly. Currently only these ioctls that are needed for per-file DAX
feature, i.e., FS_IOC_GETFLAGS/FS_IOC_SETFLAGS and
FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR are supported. Later we can restrict
the flags/attributes allowed to be set to reinforce the security, or
extend the scope of allowed ioctls if it is really needed later.
Dave had concerns about which attrs should be allowed to be set by
guest. And we were also wondering why virtiofs is not supporting
ioctl yet.
It think that it probably will make sense that supporting ioctls,
is a separate patch series for virtiofs. Anyway, we probably will
need to add it.
Vivek
From: Dr. David Alan Gilbert <hidden> Date: 2021-08-19 13:08:50
* JeffleXu (jefflexu@linux.alibaba.com) wrote:
On 8/18/21 3:00 AM, Dr. David Alan Gilbert wrote:
quoted
* Jeffle Xu (jefflexu@linux.alibaba.com) wrote:
quoted
For passthrough, when the corresponding virtiofs in guest is mounted
with '-o dax=inode', advertise that the file is capable of per-file
DAX if the inode in the backend fs is marked with FS_DAX_FL flag.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
tools/virtiofsd/passthrough_ll.c | 43 ++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
@@ -983,6 +988,41 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,return0;}+/*+*IfthefileismarkedwithFS_DAX_FLorFS_XFLAG_DAX,thenDAXshouldbe+*enabledforthisfile.+*/+staticboollo_should_enable_dax(structlo_data*lo,structlo_inode*dir,+constchar*name)+{+intres,fd;+intret=false;;+unsignedintattr;+structfsxattrxattr;++if(!lo->perfile_dax)+returnfalse;++/* Open file without O_PATH, so that ioctl can be called. */+fd=openat(dir->fd,name,O_NOFOLLOW);+if(fd==-1)+returnfalse;
Doesn't that defeat the whole benefit of using O_PATH - i.e. that we
might stumble into a /dev node or something else we're not allowed to
open?
As far as I know, virtiofsd will pivot_root/chroot to the source
directory, and can only access files inside the source directory
specified by "-o source=". Then where do these unexpected files come
from? Besides, fd opened without O_PATH here is temporary and used for
FS_IOC_GETFLAGS/FS_IOC_FSGETXATTR ioctl only. It's closed when the
function returns.
The guest is still allowed to mknod.
See:
https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg05461.html
also it's legal to expose a root filesystem for a guest; the virtiofsd
should *never* open a device other than O_PATH - and it's really tricky
to do a check to see if it is a device in a race-free way.
quoted
quoted
+ if (lo->perfile_dax_cap == DAX_CAP_FLAGS) {+ res = ioctl(fd, FS_IOC_GETFLAGS, &attr);+ if (!res && (attr & FS_DAX_FL))+ ret = true;+ }+ else if (lo->perfile_dax_cap == DAX_CAP_XATTR) {+ res = ioctl(fd, FS_IOC_FSGETXATTR, &xattr);+ if (!res && (xattr.fsx_xflags & FS_XFLAG_DAX))+ ret = true;+ }
This all looks pretty expensive for each lookup.
Yes. it can be somehow optimized if we can agree on the way of storing
the dax flag persistently.
From: Dr. David Alan Gilbert <hidden> Date: 2021-08-19 13:57:57
* JeffleXu (jefflexu@linux.alibaba.com) wrote:
On 8/18/21 1:15 AM, Dr. David Alan Gilbert wrote:
quoted
* Jeffle Xu (jefflexu@linux.alibaba.com) wrote:
quoted
In FUSE_INIT negotiating phase, server/client should advertise if it
supports per-file DAX.
Once advertising support for per-file DAX feature, virtiofsd should
support storing FS_DAX_FL flag persistently passed by
FS_IOC_SETFLAGS/FS_IOC_FSSETXATTR ioctl, and set FUSE_ATTR_DAX in
FUSE_LOOKUP accordingly if the file is capable of per-file DAX.
Currently only ext4/xfs since linux kernel v5.8 support storing
FS_DAX_FL flag persistently, and thus advertise support for per-file
DAX feature only when the backend fs type is ext4 and xfs.
I'm a little worried about the meaning of the flags we're storing and
the fact we're storing them in the normal host DAX flags.
Doesn't this mean that we're using a single host flag to mean:
a) It can be mapped as DAX on the host if it was a real DAX device
b) We can map it as DAX inside the guest with virtiofs?
Yes the side effect is that the host file is also dax enabled if the
backend fs is built upon real nvdimm device.
The rationale here is that, fuse daemon shall be capable of *marking*
the file as dax capable *persistently*, so that it can be informed that
this file is capable of dax later.
Right, so my worry here is that the untrusted guest changes both it's
own behaviour (fine) and also the behaviour of the host (less fine).
I'm not sure if xattr (extent attribute) is a better option for this?
Well, if you used an xattr for it, it wouldn't clash with whatever the
host did (especially if it used the xattr mapping).
Dave
quoted
what happens when we're using usernamespaces for the guest?
Dave
For passthrough, when the corresponding virtiofs in guest is mounted
with '-o dax=inode', advertise that the file is capable of per-file
DAX if the inode in the backend fs is marked with FS_DAX_FL flag.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
tools/virtiofsd/passthrough_ll.c | 43 ++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
@@ -983,6 +988,41 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,return0;}+/*+*IfthefileismarkedwithFS_DAX_FLorFS_XFLAG_DAX,thenDAXshouldbe+*enabledforthisfile.+*/+staticboollo_should_enable_dax(structlo_data*lo,structlo_inode*dir,+constchar*name)+{+intres,fd;+intret=false;;+unsignedintattr;+structfsxattrxattr;++if(!lo->perfile_dax)+returnfalse;++/* Open file without O_PATH, so that ioctl can be called. */+fd=openat(dir->fd,name,O_NOFOLLOW);+if(fd==-1)+returnfalse;
Doesn't that defeat the whole benefit of using O_PATH - i.e. that we
might stumble into a /dev node or something else we're not allowed to
open?
As far as I know, virtiofsd will pivot_root/chroot to the source
directory, and can only access files inside the source directory
specified by "-o source=". Then where do these unexpected files come
from? Besides, fd opened without O_PATH here is temporary and used for
FS_IOC_GETFLAGS/FS_IOC_FSGETXATTR ioctl only. It's closed when the
function returns.
The guest is still allowed to mknod.
See:
https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg05461.html
also it's legal to expose a root filesystem for a guest; the virtiofsd
should *never* open a device other than O_PATH - and it's really tricky
to do a check to see if it is a device in a race-free way.
Fine. Got it. However the returned fd (opened without O_PATH) is only
used for FS_IOC_GETFLAGS/FS_IOC_FSGETXATTR ioctl, while in most cases
for special device files, these two ioctls should return -ENOTTY.
If it's really a security issue, then lo_inode_open() could be used to
get a temporary fd, i.e., check if it's a special file before opening.
After all, FUSE_OPEN also handles in this way. Besides, I can't
understand what "race-free way" means.
--
Thanks,
Jeffle
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Greg Kurz <hidden> Date: 2021-08-24 10:15:37
On Fri, 20 Aug 2021 13:03:23 +0800
JeffleXu [off-list ref] wrote:
On 8/19/21 9:08 PM, Dr. David Alan Gilbert wrote:
quoted
* JeffleXu (jefflexu@linux.alibaba.com) wrote:
quoted
On 8/18/21 3:00 AM, Dr. David Alan Gilbert wrote:
quoted
* Jeffle Xu (jefflexu@linux.alibaba.com) wrote:
quoted
For passthrough, when the corresponding virtiofs in guest is mounted
with '-o dax=inode', advertise that the file is capable of per-file
DAX if the inode in the backend fs is marked with FS_DAX_FL flag.
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
tools/virtiofsd/passthrough_ll.c | 43 ++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
@@ -983,6 +988,41 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname,return0;}+/*+*IfthefileismarkedwithFS_DAX_FLorFS_XFLAG_DAX,thenDAXshouldbe+*enabledforthisfile.+*/+staticboollo_should_enable_dax(structlo_data*lo,structlo_inode*dir,+constchar*name)+{+intres,fd;+intret=false;;+unsignedintattr;+structfsxattrxattr;++if(!lo->perfile_dax)+returnfalse;++/* Open file without O_PATH, so that ioctl can be called. */+fd=openat(dir->fd,name,O_NOFOLLOW);+if(fd==-1)+returnfalse;
Doesn't that defeat the whole benefit of using O_PATH - i.e. that we
might stumble into a /dev node or something else we're not allowed to
open?
As far as I know, virtiofsd will pivot_root/chroot to the source
directory, and can only access files inside the source directory
specified by "-o source=". Then where do these unexpected files come
from? Besides, fd opened without O_PATH here is temporary and used for
FS_IOC_GETFLAGS/FS_IOC_FSGETXATTR ioctl only. It's closed when the
function returns.
The guest is still allowed to mknod.
See:
https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg05461.html
also it's legal to expose a root filesystem for a guest; the virtiofsd
should *never* open a device other than O_PATH - and it's really tricky
to do a check to see if it is a device in a race-free way.
Fine. Got it. However the returned fd (opened without O_PATH) is only
used for FS_IOC_GETFLAGS/FS_IOC_FSGETXATTR ioctl, while in most cases
for special device files, these two ioctls should return -ENOTTY.
The actual problem is that a FIFO will cause openat() to block until
the other end of the FIFO is open for writing...
If it's really a security issue, then lo_inode_open() could be used to
... and cause a DoS on virtiofsd. So yes, this is a security issue and
lo_inode_open() was introduced specifically to handle this.
get a temporary fd, i.e., check if it's a special file before opening.
After all, FUSE_OPEN also handles in this way. Besides, I can't
understand what "race-free way" means.
"race-free way" means a way that guarantees that file type
cannot change between the time you check it and the time
you open it (TOCTOU error). For example, doing a plain stat(),
checking st_mode and proceeding to open() is wrong : nothing
prevents the file to be unlinked and replaced by something
else between stat() and open().
We avoid that by keeping O_PATH fds around and using
lo_inode_open() instead of openat().
In your case, it seems that you should do the checking after
you have an actual lo_inode for the target file, and pass
that to lo_should_enable_dax() instead of the parent lo_inode
and target name.
Cheers,
--
Greg
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Fri, 20 Aug 2021 13:03:23 +0800
JeffleXu [off-list ref] wrote:
quoted
Fine. Got it. However the returned fd (opened without O_PATH) is only
used for FS_IOC_GETFLAGS/FS_IOC_FSGETXATTR ioctl, while in most cases
for special device files, these two ioctls should return -ENOTTY.
The actual problem is that a FIFO will cause openat() to block until
the other end of the FIFO is open for writing...
Got it.
quoted
If it's really a security issue, then lo_inode_open() could be used to
... and cause a DoS on virtiofsd. So yes, this is a security issue and
lo_inode_open() was introduced specifically to handle this.
quoted
get a temporary fd, i.e., check if it's a special file before opening.
After all, FUSE_OPEN also handles in this way. Besides, I can't
understand what "race-free way" means.
"race-free way" means a way that guarantees that file type
cannot change between the time you check it and the time
you open it (TOCTOU error). For example, doing a plain stat(),
checking st_mode and proceeding to open() is wrong : nothing
prevents the file to be unlinked and replaced by something
else between stat() and open().
We avoid that by keeping O_PATH fds around and using
lo_inode_open() instead of openat().
Thanks for the detailed explanation. Got it.
In your case, it seems that you should do the checking after
you have an actual lo_inode for the target file, and pass
that to lo_should_enable_dax() instead of the parent lo_inode
and target name.