[PATCH RFC] fuse: Prevent background write requests increase inode size

[PATCH RFC] fuse: Prevent background write requests increase inode size

[Kirill Tkhai]

Hi, Miklos,

this is not a well-tested patch, this is a concept,
showing the places, where it looks we have a problem.

Commit 7879c4e58b7c made io->async careless about inode size,
and this is wrong. Asyncronuos background requests may be sent
to userspace after inode becomes unlocked, when background
queue is throttled. In this case we execute a write request
extending inode size without any protection, and this ruines
everything. Fix that.

Also, some write background requests do not increment fi->writectr,
e.g.:
	fuse_direct_IO()
	  fuse_direct_io()
	    fuse_send_write()
	      fuse_async_req_send()
	        fuse_request_send_background()

This is a reason fuse_sync_writes() does not work as expected.
Fix that too.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 fs/fuse/file.c |   20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index cc2121b37bf5..0b71a3c96168 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -610,6 +610,9 @@ static void fuse_aio_complete(struct fuse_io_priv *io, int err, ssize_t pos)
 static void fuse_aio_complete_req(struct fuse_conn *fc, struct fuse_req *req)
 {
 	struct fuse_io_priv *io = req->io;
+	struct kiocb *iocb = io->iocb;
+	struct file *file = iocb->ki_filp;
+	struct fuse_inode *fi = get_fuse_inode(file_inode(file));
 	ssize_t pos = -1;
 
 	fuse_release_user_pages(req, io->should_dirty);
@@ -625,6 +628,12 @@ static void fuse_aio_complete_req(struct fuse_conn *fc, struct fuse_req *req)
 	}
 
 	fuse_aio_complete(io, req->out.h.error, pos);
+
+	if (io->write) {
+		spin_lock(&fc->lock);
+		fi->writectr--;
+		spin_unlock(&fc->lock);
+	}
 }
 
 static size_t fuse_async_req_send(struct fuse_conn *fc, struct fuse_req *req,
@@ -966,6 +975,7 @@ static size_t fuse_send_write(struct fuse_req *req, struct fuse_io_priv *io,
 {
 	struct kiocb *iocb = io->iocb;
 	struct file *file = iocb->ki_filp;
+	struct fuse_inode *fi = get_fuse_inode(file_inode(file));
 	struct fuse_file *ff = file->private_data;
 	struct fuse_conn *fc = ff->fc;
 	struct fuse_write_in *inarg = &req->misc.write.in;
@@ -981,8 +991,12 @@ static size_t fuse_send_write(struct fuse_req *req, struct fuse_io_priv *io,
 		inarg->lock_owner = fuse_lock_owner_id(fc, owner);
 	}
 
-	if (io->async)
+	if (io->async) {
+		spin_lock(&fc->lock);
+		fi->writectr++;
+		spin_unlock(&fc->lock);
 		return fuse_async_req_send(fc, req, count, io);
+	}
 
 	fuse_request_send(fc, req);
 	return req->misc.write.out.size;
@@ -2904,8 +2918,10 @@ fuse_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 	 * We cannot asynchronously extend the size of a file.
 	 * In such case the aio will behave exactly like sync io.
 	 */
-	if ((offset + count > i_size) && iov_iter_rw(iter) == WRITE)
+	if ((offset + count > i_size) && iov_iter_rw(iter) == WRITE) {
 		io->blocking = true;
+		io->async = false;
+	}
 
 	if (io->async && io->blocking) {
 		/*

Re: [PATCH RFC] fuse: Prevent background write requests increase inode size

[Kirill Tkhai]

Hi, Miklos,

please take a look into two problems this patch points:

1)we have to set "io->async = false" in fuse_direct_IO(),
  when inode size is changed (we mustn't increase inode
  size in background);
2)we have to increment fi->writectr on all background requests.

Thanks,
Kirill

On 06.11.2018 17:02, Kirill Tkhai wrote:
> Hi, Miklos,
> 
> this is not a well-tested patch, this is a concept,
> showing the places, where it looks we have a problem.
> 
> Commit 7879c4e58b7c made io->async careless about inode size,
> and this is wrong. Asyncronuos background requests may be sent
> to userspace after inode becomes unlocked, when background
> queue is throttled. In this case we execute a write request
> extending inode size without any protection, and this ruines
> everything. Fix that.
> 
> Also, some write background requests do not increment fi->writectr,
> e.g.:
> 	fuse_direct_IO()
> 	  fuse_direct_io()
> 	    fuse_send_write()
> 	      fuse_async_req_send()
> 	        fuse_request_send_background()
> 
> This is a reason fuse_sync_writes() does not work as expected.
> Fix that too.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  fs/fuse/file.c |   20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index cc2121b37bf5..0b71a3c96168 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -610,6 +610,9 @@ static void fuse_aio_complete(struct fuse_io_priv *io, int err, ssize_t pos)
>  static void fuse_aio_complete_req(struct fuse_conn *fc, struct fuse_req *req)
>  {
>  	struct fuse_io_priv *io = req->io;
> +	struct kiocb *iocb = io->iocb;
> +	struct file *file = iocb->ki_filp;
> +	struct fuse_inode *fi = get_fuse_inode(file_inode(file));
>  	ssize_t pos = -1;
>  
>  	fuse_release_user_pages(req, io->should_dirty);
> @@ -625,6 +628,12 @@ static void fuse_aio_complete_req(struct fuse_conn *fc, struct fuse_req *req)
>  	}
>  
>  	fuse_aio_complete(io, req->out.h.error, pos);
> +
> +	if (io->write) {
> +		spin_lock(&fc->lock);
> +		fi->writectr--;
> +		spin_unlock(&fc->lock);
> +	}
>  }
>  
>  static size_t fuse_async_req_send(struct fuse_conn *fc, struct fuse_req *req,
> @@ -966,6 +975,7 @@ static size_t fuse_send_write(struct fuse_req *req, struct fuse_io_priv *io,
>  {
>  	struct kiocb *iocb = io->iocb;
>  	struct file *file = iocb->ki_filp;
> +	struct fuse_inode *fi = get_fuse_inode(file_inode(file));
>  	struct fuse_file *ff = file->private_data;
>  	struct fuse_conn *fc = ff->fc;
>  	struct fuse_write_in *inarg = &req->misc.write.in;
> @@ -981,8 +991,12 @@ static size_t fuse_send_write(struct fuse_req *req, struct fuse_io_priv *io,
>  		inarg->lock_owner = fuse_lock_owner_id(fc, owner);
>  	}
>  
> -	if (io->async)
> +	if (io->async) {
> +		spin_lock(&fc->lock);
> +		fi->writectr++;
> +		spin_unlock(&fc->lock);
>  		return fuse_async_req_send(fc, req, count, io);
> +	}
>  
>  	fuse_request_send(fc, req);
>  	return req->misc.write.out.size;
> @@ -2904,8 +2918,10 @@ fuse_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>  	 * We cannot asynchronously extend the size of a file.
>  	 * In such case the aio will behave exactly like sync io.
>  	 */
> -	if ((offset + count > i_size) && iov_iter_rw(iter) == WRITE)
> +	if ((offset + count > i_size) && iov_iter_rw(iter) == WRITE) {
>  		io->blocking = true;
> +		io->async = false;
> +	}
>  
>  	if (io->async && io->blocking) {
>  		/*
> 

Re: [PATCH RFC] fuse: Prevent background write requests increase inode size

[Miklos Szeredi]

On Tue, Nov 6, 2018 at 3:03 PM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>
> Hi, Miklos,
>
> this is not a well-tested patch, this is a concept,
> showing the places, where it looks we have a problem.
>
> Commit 7879c4e58b7c made io->async careless about inode size,
> and this is wrong. Asyncronuos background requests may be sent
> to userspace after inode becomes unlocked, when background
> queue is throttled. In this case we execute a write request
> extending inode size without any protection, and this ruines
> everything. Fix that.
>
> Also, some write background requests do not increment fi->writectr,
> e.g.:
>         fuse_direct_IO()
>           fuse_direct_io()
>             fuse_send_write()
>               fuse_async_req_send()
>                 fuse_request_send_background()

inode_lock should prevent mischief for sync DIO.  For AIO DIO the
order of operations is not deterministic, so it's fine if truncate and
an extending AIO write race, as far as I understand.

Thanks,
Miklos