Re: [PATCH v4 07/12] virtiofsd: Let lo_inode_open() return a TempFd
On 18.10.21 21:18, Vivek Goyal wrote:
On Thu, Sep 16, 2021 at 10:40:40AM +0200, Hanna Reitz wrote:
Strictly speaking, this is not necessary, because lo_inode_open() will
always return a new FD owned by the caller, so TempFd.owned will always
be true.
The auto-cleanup is nice, though. Also, we get a more unified interface
where you always get a TempFd when you need an FD for an lo_inode
(regardless of whether it is an O_PATH FD or a non-O_PATH FD).
Signed-off-by: Hanna Reitz
---
tools/virtiofsd/passthrough_ll.c | 156 +++
1 file changed, 75 insertions(+), 81 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 3bf20b8659..d257eda129 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -293,10 +293,8 @@ static void temp_fd_clear(TempFd *temp_fd)
/**
* Return an owned fd from *temp_fd that will not be closed when
* *temp_fd goes out of scope.
- *
- * (TODO: Remove __attribute__ once this is used.)
*/
-static __attribute__((unused)) int temp_fd_steal(TempFd *temp_fd)
+static int temp_fd_steal(TempFd *temp_fd)
{
if (temp_fd->owned) {
temp_fd->owned = false;
@@ -309,10 +307,8 @@ static __attribute__((unused)) int temp_fd_steal(TempFd
*temp_fd)
/**
* Create a borrowing copy of an existing TempFd. Note that *to is
* only valid as long as *from is valid.
- *
- * (TODO: Remove __attribute__ once this is used.)
*/
-static __attribute__((unused)) void temp_fd_copy(const TempFd *from, TempFd
*to)
+static void temp_fd_copy(const TempFd *from, TempFd *to)
{
*to = (TempFd) {
.fd = from->fd,
@@ -689,9 +685,12 @@ static int lo_fd(fuse_req_t req, fuse_ino_t ino, TempFd
*tfd)
* when a malicious client opens special files such as block device nodes.
* Symlink inodes are also rejected since symlinks must already have been
* traversed on the client side.
+ *
+ * The fd is returned in tfd->fd. The return value is 0 on success and -errno
+ * otherwise.
*/
static int lo_inode_open(struct lo_data *lo, struct lo_inode *inode,
- int open_flags)
+ int open_flags, TempFd *tfd)
{
g_autofree char *fd_str = g_strdup_printf("%d", inode->fd);
int fd;
@@ -710,7 +709,13 @@ static int lo_inode_open(struct lo_data *lo, struct
lo_inode *inode,
if (fd < 0) {
return -errno;
}
-return fd;
+
+*tfd = (TempFd) {
+.fd = fd,
+.owned = true,
+};
+
+return 0;
}
static void lo_init(void *userdata, struct fuse_conn_info *conn)
@@ -854,7 +859,8 @@ static int lo_fi_fd(fuse_req_t req, struct fuse_file_info
*fi)
static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
int valid, struct fuse_file_info *fi)
{
-g_auto(TempFd) path_fd = TEMP_FD_INIT;
+g_auto(TempFd) path_fd = TEMP_FD_INIT; /* at least an O_PATH fd */
What does atleast O_PATH fd mean?
It means it might as well be an O_RDWR FD. When we open an O_RDWR FD,
it’s pointless to open an O_PATH FD, too, because we can use the O_RDWR
FD everywhere where we’d need an O_PATH FD. Hence in this case, we open
rw_fd and copy it to path_fd.
Users still use rw_fd and path_fd according to what they need.
+g_auto(TempFd) rw_fd = TEMP_FD_INIT; /* O_RDWR fd */
int saverr;
char procname[64];
struct lo_data *lo = lo_data(req);
@@ -868,7 +874,15 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino,
struct stat *attr,
return;
}
-res = lo_inode_fd(inode, _fd);
+if (!fi && (valid & FUSE_SET_ATTR_SIZE)) {
+/* We need an O_RDWR FD for ftruncate() */
+res = lo_inode_open(lo, inode, O_RDWR, _fd);
+if (res >= 0) {
+temp_fd_copy(_fd, _fd);
I am lost here. If lo_inode_open() failed, why are we calling this
temp_fd_copy()? path_fd is not even a valid fd yet.
We’re calling it on success.
Still beats me that why open rw_fd now instead of down in
FUSE_SET_ATTR_SIZE block. I think we had this discussion and you
had some reasons to move it up.
Because it’s pointless overhead to open two FDs.
Before file handles, getting the O_PATH FD was a trivial operation. If
we needed an O_RDWR FD later, we’d open it later. No duplicate work
involved.
With file handles, getting the O_PATH FD may mean opening a new FD.
Opening another FD from the same file handle, just with different flags,
later on would be strange, because we could have just opened the FD as
O_RDWR right from the start.
Hanna
Re: [PATCH v4 07/12] virtiofsd: Let lo_inode_open() return a TempFd
On Thu, Sep 16, 2021 at 10:40:40AM +0200, Hanna Reitz wrote:
> Strictly speaking, this is not necessary, because lo_inode_open() will
> always return a new FD owned by the caller, so TempFd.owned will always
> be true.
>
> The auto-cleanup is nice, though. Also, we get a more unified interface
> where you always get a TempFd when you need an FD for an lo_inode
> (regardless of whether it is an O_PATH FD or a non-O_PATH FD).
>
> Signed-off-by: Hanna Reitz
> ---
> tools/virtiofsd/passthrough_ll.c | 156 +++
> 1 file changed, 75 insertions(+), 81 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c
> b/tools/virtiofsd/passthrough_ll.c
> index 3bf20b8659..d257eda129 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -293,10 +293,8 @@ static void temp_fd_clear(TempFd *temp_fd)
> /**
> * Return an owned fd from *temp_fd that will not be closed when
> * *temp_fd goes out of scope.
> - *
> - * (TODO: Remove __attribute__ once this is used.)
> */
> -static __attribute__((unused)) int temp_fd_steal(TempFd *temp_fd)
> +static int temp_fd_steal(TempFd *temp_fd)
> {
> if (temp_fd->owned) {
> temp_fd->owned = false;
> @@ -309,10 +307,8 @@ static __attribute__((unused)) int temp_fd_steal(TempFd
> *temp_fd)
> /**
> * Create a borrowing copy of an existing TempFd. Note that *to is
> * only valid as long as *from is valid.
> - *
> - * (TODO: Remove __attribute__ once this is used.)
> */
> -static __attribute__((unused)) void temp_fd_copy(const TempFd *from, TempFd
> *to)
> +static void temp_fd_copy(const TempFd *from, TempFd *to)
> {
> *to = (TempFd) {
> .fd = from->fd,
> @@ -689,9 +685,12 @@ static int lo_fd(fuse_req_t req, fuse_ino_t ino, TempFd
> *tfd)
> * when a malicious client opens special files such as block device nodes.
> * Symlink inodes are also rejected since symlinks must already have been
> * traversed on the client side.
> + *
> + * The fd is returned in tfd->fd. The return value is 0 on success and
> -errno
> + * otherwise.
> */
> static int lo_inode_open(struct lo_data *lo, struct lo_inode *inode,
> - int open_flags)
> + int open_flags, TempFd *tfd)
> {
> g_autofree char *fd_str = g_strdup_printf("%d", inode->fd);
> int fd;
> @@ -710,7 +709,13 @@ static int lo_inode_open(struct lo_data *lo, struct
> lo_inode *inode,
> if (fd < 0) {
> return -errno;
> }
> -return fd;
> +
> +*tfd = (TempFd) {
> +.fd = fd,
> +.owned = true,
> +};
> +
> +return 0;
> }
>
> static void lo_init(void *userdata, struct fuse_conn_info *conn)
> @@ -854,7 +859,8 @@ static int lo_fi_fd(fuse_req_t req, struct fuse_file_info
> *fi)
> static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
> int valid, struct fuse_file_info *fi)
> {
> -g_auto(TempFd) path_fd = TEMP_FD_INIT;
> +g_auto(TempFd) path_fd = TEMP_FD_INIT; /* at least an O_PATH fd */
What does atleast O_PATH fd mean?
> +g_auto(TempFd) rw_fd = TEMP_FD_INIT; /* O_RDWR fd */
> int saverr;
> char procname[64];
> struct lo_data *lo = lo_data(req);
> @@ -868,7 +874,15 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino,
> struct stat *attr,
> return;
> }
>
> -res = lo_inode_fd(inode, _fd);
> +if (!fi && (valid & FUSE_SET_ATTR_SIZE)) {
> +/* We need an O_RDWR FD for ftruncate() */
> +res = lo_inode_open(lo, inode, O_RDWR, _fd);
> +if (res >= 0) {
> +temp_fd_copy(_fd, _fd);
I am lost here. If lo_inode_open() failed, why are we calling this
temp_fd_copy()? path_fd is not even a valid fd yet.
Still beats me that why open rw_fd now instead of down in
FUSE_SET_ATTR_SIZE block. I think we had this discussion and you
had some reasons to move it up.
Vivek
> +}
> +} else {
> +res = lo_inode_fd(inode, _fd);
> +}
> if (res < 0) {
> saverr = -res;
> goto out_err;
> @@ -916,18 +930,12 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino,
> struct stat *attr,
> if (fi) {
> truncfd = fd;
> } else {
> -truncfd = lo_inode_open(lo, inode, O_RDWR);
> -if (truncfd < 0) {
> -saverr = -truncfd;
> -goto out_err;
> -}
> +assert(rw_fd.fd >= 0);
> +truncfd = rw_fd.fd;
> }
>
> saverr = drop_security_capability(lo, truncfd);
> if (saverr) {
> -if (!fi) {
> -close(truncfd);
> -}
> goto out_err;
> }
>
> @@ -935,9 +943,6 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino,
> struct stat *attr,
> res = drop_effective_cap("FSETID", _fsetid_dropped);
> if (res != 0) {
> saverr = res;
> -if
[PATCH v4 07/12] virtiofsd: Let lo_inode_open() return a TempFd
Strictly speaking, this is not necessary, because lo_inode_open() will
always return a new FD owned by the caller, so TempFd.owned will always
be true.
The auto-cleanup is nice, though. Also, we get a more unified interface
where you always get a TempFd when you need an FD for an lo_inode
(regardless of whether it is an O_PATH FD or a non-O_PATH FD).
Signed-off-by: Hanna Reitz
---
tools/virtiofsd/passthrough_ll.c | 156 +++
1 file changed, 75 insertions(+), 81 deletions(-)
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 3bf20b8659..d257eda129 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -293,10 +293,8 @@ static void temp_fd_clear(TempFd *temp_fd)
/**
* Return an owned fd from *temp_fd that will not be closed when
* *temp_fd goes out of scope.
- *
- * (TODO: Remove __attribute__ once this is used.)
*/
-static __attribute__((unused)) int temp_fd_steal(TempFd *temp_fd)
+static int temp_fd_steal(TempFd *temp_fd)
{
if (temp_fd->owned) {
temp_fd->owned = false;
@@ -309,10 +307,8 @@ static __attribute__((unused)) int temp_fd_steal(TempFd
*temp_fd)
/**
* Create a borrowing copy of an existing TempFd. Note that *to is
* only valid as long as *from is valid.
- *
- * (TODO: Remove __attribute__ once this is used.)
*/
-static __attribute__((unused)) void temp_fd_copy(const TempFd *from, TempFd
*to)
+static void temp_fd_copy(const TempFd *from, TempFd *to)
{
*to = (TempFd) {
.fd = from->fd,
@@ -689,9 +685,12 @@ static int lo_fd(fuse_req_t req, fuse_ino_t ino, TempFd
*tfd)
* when a malicious client opens special files such as block device nodes.
* Symlink inodes are also rejected since symlinks must already have been
* traversed on the client side.
+ *
+ * The fd is returned in tfd->fd. The return value is 0 on success and -errno
+ * otherwise.
*/
static int lo_inode_open(struct lo_data *lo, struct lo_inode *inode,
- int open_flags)
+ int open_flags, TempFd *tfd)
{
g_autofree char *fd_str = g_strdup_printf("%d", inode->fd);
int fd;
@@ -710,7 +709,13 @@ static int lo_inode_open(struct lo_data *lo, struct
lo_inode *inode,
if (fd < 0) {
return -errno;
}
-return fd;
+
+*tfd = (TempFd) {
+.fd = fd,
+.owned = true,
+};
+
+return 0;
}
static void lo_init(void *userdata, struct fuse_conn_info *conn)
@@ -854,7 +859,8 @@ static int lo_fi_fd(fuse_req_t req, struct fuse_file_info
*fi)
static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
int valid, struct fuse_file_info *fi)
{
-g_auto(TempFd) path_fd = TEMP_FD_INIT;
+g_auto(TempFd) path_fd = TEMP_FD_INIT; /* at least an O_PATH fd */
+g_auto(TempFd) rw_fd = TEMP_FD_INIT; /* O_RDWR fd */
int saverr;
char procname[64];
struct lo_data *lo = lo_data(req);
@@ -868,7 +874,15 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino,
struct stat *attr,
return;
}
-res = lo_inode_fd(inode, _fd);
+if (!fi && (valid & FUSE_SET_ATTR_SIZE)) {
+/* We need an O_RDWR FD for ftruncate() */
+res = lo_inode_open(lo, inode, O_RDWR, _fd);
+if (res >= 0) {
+temp_fd_copy(_fd, _fd);
+}
+} else {
+res = lo_inode_fd(inode, _fd);
+}
if (res < 0) {
saverr = -res;
goto out_err;
@@ -916,18 +930,12 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino,
struct stat *attr,
if (fi) {
truncfd = fd;
} else {
-truncfd = lo_inode_open(lo, inode, O_RDWR);
-if (truncfd < 0) {
-saverr = -truncfd;
-goto out_err;
-}
+assert(rw_fd.fd >= 0);
+truncfd = rw_fd.fd;
}
saverr = drop_security_capability(lo, truncfd);
if (saverr) {
-if (!fi) {
-close(truncfd);
-}
goto out_err;
}
@@ -935,9 +943,6 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino,
struct stat *attr,
res = drop_effective_cap("FSETID", _fsetid_dropped);
if (res != 0) {
saverr = res;
-if (!fi) {
-close(truncfd);
-}
goto out_err;
}
}
@@ -950,9 +955,6 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino,
struct stat *attr,
fuse_log(FUSE_LOG_ERR, "Failed to gain CAP_FSETID\n");
}
}
-if (!fi) {
-close(truncfd);
-}
if (res == -1) {
goto out_err;
}
@@ -1840,11 +1842,13 @@ static struct lo_dirp *lo_dirp(fuse_req_t req, struct
fuse_file_info *fi)
static void lo_opendir(fuse_req_t req, fuse_ino_t ino,
struct fuse_file_info *fi)
{
+
