[Devel,vz7] fuse: fuse_prepare_write() cannot handle page from killed request

After fuse_prepare_write() called __fuse_readpage(file, page, ...),
the page might be already unlocked by fuse_kill_requests():

>		for (i = 0; i < req->num_pages; i++) {
>			struct page *page = req->pages[i];
>			SetPageError(page);
>			unlock_page(page);

so it is incorrect to touch it at all. The problem can be easily
fixed the same way is it was done in fuse_readpage() checking "killed"
flag.

Another minor complication is that there are three different use-cases
for that snippet from fuse_kill_requests() above: fuse_readpages(),
fuse_readpage() and fuse_prepare_write(). Among them only the latter
needs explicit page_cache_release() call. That's why the patch introduces
ad-hoc request flag "page_needs_release".

https://jira.sw.ru/browse/PSBM-54547
Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
---
 fs/fuse/file.c   |   15 ++++++++++-----
 fs/fuse/fuse_i.h |    3 +++
 fs/fuse/inode.c  |    2 ++
 3 files changed, 15 insertions(+), 5 deletions(-)

Dima, please review the patch.

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 02/14/2017 03:03 AM, Maxim Patlasov wrote:
> After fuse_prepare_write() called __fuse_readpage(file, page, ...),
> the page might be already unlocked by fuse_kill_requests():
>
>> 		for (i = 0; i < req->num_pages; i++) {
>> 			struct page *page = req->pages[i];
>> 			SetPageError(page);
>> 			unlock_page(page);
>
> so it is incorrect to touch it at all. The problem can be easily
> fixed the same way is it was done in fuse_readpage() checking "killed"
> flag.
>
> Another minor complication is that there are three different use-cases
> for that snippet from fuse_kill_requests() above: fuse_readpages(),
> fuse_readpage() and fuse_prepare_write(). Among them only the latter
> needs explicit page_cache_release() call. That's why the patch introduces
> ad-hoc request flag "page_needs_release".
>
> https://jira.sw.ru/browse/PSBM-54547
> Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
> ---
>  fs/fuse/file.c   |   15 ++++++++++-----
>  fs/fuse/fuse_i.h |    3 +++
>  fs/fuse/inode.c  |    2 ++
>  3 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index a514748..41ed6f0 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1008,7 +1008,7 @@ static void fuse_short_read(struct fuse_req *req, struct inode *inode,
>
>  static int __fuse_readpage(struct file *file, struct page *page, size_t count,
>  			   int *err, struct fuse_req **req_pp, u64 *attr_ver_p,
> -			   bool *killed_p)
> +			   bool page_needs_release, bool *killed_p)
>  {
>  	struct fuse_io_priv io = { .async = 0, .file = file };
>  	struct inode *inode = page->mapping->host;
> @@ -1040,6 +1040,7 @@ static int __fuse_readpage(struct file *file, struct page *page, size_t count,
>  	req->pages[0] = page;
>  	req->page_descs[0].length = count;
>  	req->page_cache = 1;
> +	req->page_needs_release = page_needs_release;
>
>  	num_read = fuse_send_read(req, &io, page_offset(page), count, NULL);
>  	killed = req->killed;
> @@ -1071,7 +1072,7 @@ static int fuse_readpage(struct file *file, struct page *page)
>  		goto out;
>
>  	num_read = __fuse_readpage(file, page, count, &err, &req, &attr_ver,
> -				   &killed);
> +				   false, &killed);
>  	if (!err) {
>  		/*
>  		 * Short read means EOF.  If file size is larger, truncate it
> @@ -1153,6 +1154,7 @@ static void fuse_send_readpages(struct fuse_req *req, struct file *file)
>  	req->out.page_zeroing = 1;
>  	req->out.page_replace = 1;
>  	req->page_cache = 1;
> +	req->page_needs_release = false;
>  	fuse_read_fill(req, file, pos, count, FUSE_READ);
>  	fuse_account_request(fc, count);
>  	req->misc.read.attr_ver = fuse_get_attr_version(fc);
> @@ -2368,6 +2370,7 @@ static int fuse_prepare_write(struct fuse_conn *fc, struct file *file,
>  	unsigned num_read;
>  	unsigned page_len;
>  	int err;
> +	bool killed = false;
>
>  	if (fuse_file_fail_immediately(file)) {
>  		unlock_page(page);
> @@ -2385,12 +2388,14 @@ static int fuse_prepare_write(struct fuse_conn *fc, struct file *file,
>  	}
>
>  	num_read = __fuse_readpage(file, page, page_len, &err, &req, NULL,
> -				   NULL);
> +				   true, &killed);
>  	if (req)
>  		fuse_put_request(fc, req);
>  	if (err) {
> -		unlock_page(page);
> -		page_cache_release(page);
> +		if (!killed) {
> +			unlock_page(page);
> +			page_cache_release(page);
> +		}
>  	} else if (num_read != PAGE_CACHE_SIZE) {
>  		zero_user_segment(page, num_read, PAGE_CACHE_SIZE);
>  	}
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 22eb9c9..fefa8ff 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -330,6 +330,9 @@ struct fuse_req {
>  	/** Request contains pages from page-cache */
>  	unsigned page_cache:1;
>
> +	/** Request pages need page_cache_release() */
> +	unsigned page_needs_release:1;
> +
>  	/** Request was killed -- pages were released */
>  	unsigned killed:1;
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index b63aae2..ddd858c 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -378,6 +378,8 @@ static void fuse_kill_requests(struct fuse_conn *fc, struct inode *inode,
>  				struct page *page = req->pages[i];
>  				SetPageError(page);
>  				unlock_page(page);
> +				if (req->page_needs_release)
> +					page_cache_release(page);
>  				req->pages[i] = NULL;
>  			}
>
>
> .
>

Maxim Patlasov <mpatlasov@virtuozzo.com> writes:

> After fuse_prepare_write() called __fuse_readpage(file, page, ...),
> the page might be already unlocked by fuse_kill_requests():
>
>>		for (i = 0; i < req->num_pages; i++) {
>>			struct page *page = req->pages[i];
>>			SetPageError(page);
>>			unlock_page(page);
ACK.
>
> so it is incorrect to touch it at all. The problem can be easily
> fixed the same way is it was done in fuse_readpage() checking "killed"
> flag.
>
> Another minor complication is that there are three different use-cases
> for that snippet from fuse_kill_requests() above: fuse_readpages(),
> fuse_readpage() and fuse_prepare_write(). Among them only the latter
> needs explicit page_cache_release() call. That's why the patch introduces
> ad-hoc request flag "page_needs_release".
>
> https://jira.sw.ru/browse/PSBM-54547
> Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
> ---
>  fs/fuse/file.c   |   15 ++++++++++-----
>  fs/fuse/fuse_i.h |    3 +++
>  fs/fuse/inode.c  |    2 ++
>  3 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index a514748..41ed6f0 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1008,7 +1008,7 @@ static void fuse_short_read(struct fuse_req *req, struct inode *inode,
>  
>  static int __fuse_readpage(struct file *file, struct page *page, size_t count,
>  			   int *err, struct fuse_req **req_pp, u64 *attr_ver_p,
> -			   bool *killed_p)
> +			   bool page_needs_release, bool *killed_p)
>  {
>  	struct fuse_io_priv io = { .async = 0, .file = file };
>  	struct inode *inode = page->mapping->host;
> @@ -1040,6 +1040,7 @@ static int __fuse_readpage(struct file *file, struct page *page, size_t count,
>  	req->pages[0] = page;
>  	req->page_descs[0].length = count;
>  	req->page_cache = 1;
> +	req->page_needs_release = page_needs_release;
>  
>  	num_read = fuse_send_read(req, &io, page_offset(page), count, NULL);
>  	killed = req->killed;
> @@ -1071,7 +1072,7 @@ static int fuse_readpage(struct file *file, struct page *page)
>  		goto out;
>  
>  	num_read = __fuse_readpage(file, page, count, &err, &req, &attr_ver,
> -				   &killed);
> +				   false, &killed);
>  	if (!err) {
>  		/*
>  		 * Short read means EOF.  If file size is larger, truncate it
> @@ -1153,6 +1154,7 @@ static void fuse_send_readpages(struct fuse_req *req, struct file *file)
>  	req->out.page_zeroing = 1;
>  	req->out.page_replace = 1;
>  	req->page_cache = 1;
> +	req->page_needs_release = false;
>  	fuse_read_fill(req, file, pos, count, FUSE_READ);
>  	fuse_account_request(fc, count);
>  	req->misc.read.attr_ver = fuse_get_attr_version(fc);
> @@ -2368,6 +2370,7 @@ static int fuse_prepare_write(struct fuse_conn *fc, struct file *file,
>  	unsigned num_read;
>  	unsigned page_len;
>  	int err;
> +	bool killed = false;
>  
>  	if (fuse_file_fail_immediately(file)) {
>  		unlock_page(page);
> @@ -2385,12 +2388,14 @@ static int fuse_prepare_write(struct fuse_conn *fc, struct file *file,
>  	}
>  
>  	num_read = __fuse_readpage(file, page, page_len, &err, &req, NULL,
> -				   NULL);
> +				   true, &killed);
>  	if (req)
>  		fuse_put_request(fc, req);
>  	if (err) {
> -		unlock_page(page);
> -		page_cache_release(page);
> +		if (!killed) {
> +			unlock_page(page);
> +			page_cache_release(page);
> +		}
>  	} else if (num_read != PAGE_CACHE_SIZE) {
>  		zero_user_segment(page, num_read, PAGE_CACHE_SIZE);
>  	}
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 22eb9c9..fefa8ff 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -330,6 +330,9 @@ struct fuse_req {
>  	/** Request contains pages from page-cache */
>  	unsigned page_cache:1;
>  
> +	/** Request pages need page_cache_release() */
> +	unsigned page_needs_release:1;
> +
>  	/** Request was killed -- pages were released */
>  	unsigned killed:1;
>  
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index b63aae2..ddd858c 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -378,6 +378,8 @@ static void fuse_kill_requests(struct fuse_conn *fc, struct inode *inode,
>  				struct page *page = req->pages[i];
>  				SetPageError(page);
>  				unlock_page(page);
> +				if (req->page_needs_release)
> +					page_cache_release(page);
>  				req->pages[i] = NULL;
>  			}
>