fs/fuse kio: satisfy pure FALLOC_FL_KEEP_SIZE immediately

Submitted by Pavel Butsykin on Dec. 24, 2018, 12:54 p.m.

Details

Message ID 20181224125433.2484-1-pbutsykin@virtuozzo.com
State New
Series "fs/fuse kio: satisfy pure FALLOC_FL_KEEP_SIZE immediately"
Headers show

Commit Message

Pavel Butsykin Dec. 24, 2018, 12:54 p.m.
Fallocate without mutex lock can race with setattr size request, as a result,
may be various problems, including incorrectly changed file size. At the same
time pure FALLOC_FL_KEEP_SIZE for vstorage is just nope, so we can immediately
complete fallocate with mode == FALLOC_FL_KEEP_SIZE. Also move mutex_is_locked,
since all other mode combinations either have to be under mutex or not valid.

#VSTOR-19317

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
---
 fs/fuse/kio/pcs/pcs_fuse_kdirect.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
index de54fedeb5e4..89866370a341 100644
--- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
+++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
@@ -924,9 +924,11 @@  static void pcs_fuse_submit(struct pcs_fuse_cluster *pfc, struct fuse_req *req,
 		if (inarg->offset >= di->fileinfo.attr.size)
 			inarg->mode &= ~FALLOC_FL_ZERO_RANGE;
 
+		if (inarg->mode == FALLOC_FL_KEEP_SIZE)
+			break; /* NOPE */
+
+		WARN_ON_ONCE(!mutex_is_locked(&fi->inode.i_mutex));
 		if (inarg->mode & (FALLOC_FL_ZERO_RANGE|FALLOC_FL_PUNCH_HOLE)) {
-			WARN_ON_ONCE(!mutex_is_locked(&fi->inode.i_mutex));
-
 			if ((inarg->offset & (PAGE_SIZE - 1)) || (inarg->length & (PAGE_SIZE - 1))) {
 				r->req.out.h.error = -EINVAL;
 				goto error;

Comments

Kirill Tkhai Dec. 25, 2018, 9:28 a.m.
On 24.12.2018 15:54, Pavel Butsykin wrote:
> Fallocate without mutex lock can race with setattr size request, as a result,
> may be various problems, including incorrectly changed file size. At the same
> time pure FALLOC_FL_KEEP_SIZE for vstorage is just nope, so we can immediately

man 2 fallocate:

  "Specifying  the FALLOC_FL_ZERO_RANGE flag (available since Linux 3.15) in mode zeros space in the byte range starting at offset and
   continuing for len bytes.  Within the specified range, blocks are preallocated for the regions that span the  holes  in  the  file.
   After a successful call, subsequent reads from this range will return zeros".

So, this patch makes pcs_fuse_kdirect() complete a request without error
and without content changing, while userspace thinks the range was zeroed.
It's wrong.

> complete fallocate with mode == FALLOC_FL_KEEP_SIZE. Also move mutex_is_locked,
> since all other mode combinations either have to be under mutex or not valid.
> 
> #VSTOR-19317
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> ---
>  fs/fuse/kio/pcs/pcs_fuse_kdirect.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> index de54fedeb5e4..89866370a341 100644
> --- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> +++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> @@ -924,9 +924,11 @@ static void pcs_fuse_submit(struct pcs_fuse_cluster *pfc, struct fuse_req *req,
>  		if (inarg->offset >= di->fileinfo.attr.size)
>  			inarg->mode &= ~FALLOC_FL_ZERO_RANGE;
>  
> +		if (inarg->mode == FALLOC_FL_KEEP_SIZE)
> +			break; /* NOPE */

This is untidy to add new branch, which makes further "if (inarg->mode & FALLOC_FL_KEEP_SIZE)"
check a dead code.

> +
> +		WARN_ON_ONCE(!mutex_is_locked(&fi->inode.i_mutex));
>  		if (inarg->mode & (FALLOC_FL_ZERO_RANGE|FALLOC_FL_PUNCH_HOLE)) {
> -			WARN_ON_ONCE(!mutex_is_locked(&fi->inode.i_mutex));
> -
>  			if ((inarg->offset & (PAGE_SIZE - 1)) || (inarg->length & (PAGE_SIZE - 1))) {
>  				r->req.out.h.error = -EINVAL;
>  				goto error;
>
Pavel Butsykin Dec. 25, 2018, 9:45 a.m.
On 25.12.2018 12:28, Kirill Tkhai wrote:
> On 24.12.2018 15:54, Pavel Butsykin wrote:
>> Fallocate without mutex lock can race with setattr size request, as a result,
>> may be various problems, including incorrectly changed file size. At the same
>> time pure FALLOC_FL_KEEP_SIZE for vstorage is just nope, so we can immediately
> 
> man 2 fallocate:
> 
>    "Specifying  the FALLOC_FL_ZERO_RANGE flag (available since Linux 3.15) in mode zeros space in the byte range starting at offset and
>     continuing for len bytes.  Within the specified range, blocks are preallocated for the regions that span the  holes  in  the  file.
>     After a successful call, subsequent reads from this range will return zeros".

Yes, FALLOC_FL_ZERO_RANGE | FALLOC_FL_ZERO_RANGE mode combinations
should always be under mutex, so we can just move my check before:
		if (inarg->offset >= di->fileinfo.attr.size)
			inarg->mode &= ~FALLOC_FL_ZERO_RANGE;

> So, this patch makes pcs_fuse_kdirect() complete a request without error
> and without content changing, while userspace thinks the range was zeroed.
> It's wrong.
> 
>> complete fallocate with mode == FALLOC_FL_KEEP_SIZE. Also move mutex_is_locked,
>> since all other mode combinations either have to be under mutex or not valid.
>>
>> #VSTOR-19317
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> ---
>>   fs/fuse/kio/pcs/pcs_fuse_kdirect.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>> index de54fedeb5e4..89866370a341 100644
>> --- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>> +++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>> @@ -924,9 +924,11 @@ static void pcs_fuse_submit(struct pcs_fuse_cluster *pfc, struct fuse_req *req,
>>   		if (inarg->offset >= di->fileinfo.attr.size)
>>   			inarg->mode &= ~FALLOC_FL_ZERO_RANGE;
>>   
>> +		if (inarg->mode == FALLOC_FL_KEEP_SIZE)
>> +			break; /* NOPE */
> 
> This is untidy to add new branch, which makes further "if (inarg->mode & FALLOC_FL_KEEP_SIZE)"
> check a dead code.

Why? mode = FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE won't pass my
check, but will pass the check "if (inarg->mode & FALLOC_FL_KEEP_SIZE)",
it's not a dead code.

>> +
>> +		WARN_ON_ONCE(!mutex_is_locked(&fi->inode.i_mutex));
>>   		if (inarg->mode & (FALLOC_FL_ZERO_RANGE|FALLOC_FL_PUNCH_HOLE)) {
>> -			WARN_ON_ONCE(!mutex_is_locked(&fi->inode.i_mutex));
>> -
>>   			if ((inarg->offset & (PAGE_SIZE - 1)) || (inarg->length & (PAGE_SIZE - 1))) {
>>   				r->req.out.h.error = -EINVAL;
>>   				goto error;
>>
Kirill Tkhai Dec. 25, 2018, 9:56 a.m.
On 25.12.2018 12:45, Pavel Butsykin wrote:
> 
> 
> On 25.12.2018 12:28, Kirill Tkhai wrote:
>> On 24.12.2018 15:54, Pavel Butsykin wrote:
>>> Fallocate without mutex lock can race with setattr size request, as a result,
>>> may be various problems, including incorrectly changed file size. At the same
>>> time pure FALLOC_FL_KEEP_SIZE for vstorage is just nope, so we can immediately
>>
>> man 2 fallocate:
>>
>>    "Specifying  the FALLOC_FL_ZERO_RANGE flag (available since Linux 3.15) in mode zeros space in the byte range starting at offset and
>>     continuing for len bytes.  Within the specified range, blocks are preallocated for the regions that span the  holes  in  the  file.
>>     After a successful call, subsequent reads from this range will return zeros".
> 
> Yes, FALLOC_FL_ZERO_RANGE | FALLOC_FL_ZERO_RANGE mode combinations
> should always be under mutex, so we can just move my check before:
> 		if (inarg->offset >= di->fileinfo.attr.size)
> 			inarg->mode &= ~FALLOC_FL_ZERO_RANGE;
> 
>> So, this patch makes pcs_fuse_kdirect() complete a request without error
>> and without content changing, while userspace thinks the range was zeroed.
>> It's wrong.
>>
>>> complete fallocate with mode == FALLOC_FL_KEEP_SIZE. Also move mutex_is_locked,
>>> since all other mode combinations either have to be under mutex or not valid.
>>>
>>> #VSTOR-19317
>>>
>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>> ---
>>>   fs/fuse/kio/pcs/pcs_fuse_kdirect.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>>> index de54fedeb5e4..89866370a341 100644
>>> --- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>>> +++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>>> @@ -924,9 +924,11 @@ static void pcs_fuse_submit(struct pcs_fuse_cluster *pfc, struct fuse_req *req,
>>>   		if (inarg->offset >= di->fileinfo.attr.size)
>>>   			inarg->mode &= ~FALLOC_FL_ZERO_RANGE;
>>>   
>>> +		if (inarg->mode == FALLOC_FL_KEEP_SIZE)
>>> +			break; /* NOPE */
>>
>> This is untidy to add new branch, which makes further "if (inarg->mode & FALLOC_FL_KEEP_SIZE)"
>> check a dead code.
> 
> Why? mode = FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE won't pass my
> check, but will pass the check "if (inarg->mode & FALLOC_FL_KEEP_SIZE)",
> it's not a dead code.

Oh, I misread that. Yeah, there is no problem.

>>> +
>>> +		WARN_ON_ONCE(!mutex_is_locked(&fi->inode.i_mutex));
>>>   		if (inarg->mode & (FALLOC_FL_ZERO_RANGE|FALLOC_FL_PUNCH_HOLE)) {
>>> -			WARN_ON_ONCE(!mutex_is_locked(&fi->inode.i_mutex));
>>> -
>>>   			if ((inarg->offset & (PAGE_SIZE - 1)) || (inarg->length & (PAGE_SIZE - 1))) {
>>>   				r->req.out.h.error = -EINVAL;
>>>   				goto error;
>>>
Pavel Butsykin Dec. 25, 2018, 10 a.m.
On 25.12.2018 12:28, Kirill Tkhai wrote:
> On 24.12.2018 15:54, Pavel Butsykin wrote:
>> Fallocate without mutex lock can race with setattr size request, as a result,
>> may be various problems, including incorrectly changed file size. At the same
>> time pure FALLOC_FL_KEEP_SIZE for vstorage is just nope, so we can immediately
> 
> man 2 fallocate:
> 
>    "Specifying  the FALLOC_FL_ZERO_RANGE flag (available since Linux 3.15) in mode zeros space in the byte range starting at offset and
>     continuing for len bytes.  Within the specified range, blocks are preallocated for the regions that span the  holes  in  the  file.
>     After a successful call, subsequent reads from this range will return zeros".
> 
> So, this patch makes pcs_fuse_kdirect() complete a request without error
> and without content changing, while userspace thinks the range was zeroed.
> It's wrong.

I don't quite understand what case you're considering, which breaks
after my patch. fallocate with mode = 
FALLOC_FL_KEEP_SIZE|FALLOC_FL_ZERO_RANGE
and offset > file_size, should have no effect on the file.

>> complete fallocate with mode == FALLOC_FL_KEEP_SIZE. Also move mutex_is_locked,
>> since all other mode combinations either have to be under mutex or not valid.
>>
>> #VSTOR-19317
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> ---
>>   fs/fuse/kio/pcs/pcs_fuse_kdirect.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>> index de54fedeb5e4..89866370a341 100644
>> --- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>> +++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>> @@ -924,9 +924,11 @@ static void pcs_fuse_submit(struct pcs_fuse_cluster *pfc, struct fuse_req *req,
>>   		if (inarg->offset >= di->fileinfo.attr.size)
>>   			inarg->mode &= ~FALLOC_FL_ZERO_RANGE;
>>   
>> +		if (inarg->mode == FALLOC_FL_KEEP_SIZE)
>> +			break; /* NOPE */
> 
> This is untidy to add new branch, which makes further "if (inarg->mode & FALLOC_FL_KEEP_SIZE)"
> check a dead code.
> 
>> +
>> +		WARN_ON_ONCE(!mutex_is_locked(&fi->inode.i_mutex));
>>   		if (inarg->mode & (FALLOC_FL_ZERO_RANGE|FALLOC_FL_PUNCH_HOLE)) {
>> -			WARN_ON_ONCE(!mutex_is_locked(&fi->inode.i_mutex));
>> -
>>   			if ((inarg->offset & (PAGE_SIZE - 1)) || (inarg->length & (PAGE_SIZE - 1))) {
>>   				r->req.out.h.error = -EINVAL;
>>   				goto error;
>>
Kirill Tkhai Dec. 25, 2018, 10:01 a.m.
On 25.12.2018 13:00, Pavel Butsykin wrote:
> 
> 
> On 25.12.2018 12:28, Kirill Tkhai wrote:
>> On 24.12.2018 15:54, Pavel Butsykin wrote:
>>> Fallocate without mutex lock can race with setattr size request, as a result,
>>> may be various problems, including incorrectly changed file size. At the same
>>> time pure FALLOC_FL_KEEP_SIZE for vstorage is just nope, so we can immediately
>>
>> man 2 fallocate:
>>
>>    "Specifying  the FALLOC_FL_ZERO_RANGE flag (available since Linux 3.15) in mode zeros space in the byte range starting at offset and
>>     continuing for len bytes.  Within the specified range, blocks are preallocated for the regions that span the  holes  in  the  file.
>>     After a successful call, subsequent reads from this range will return zeros".
>>
>> So, this patch makes pcs_fuse_kdirect() complete a request without error
>> and without content changing, while userspace thinks the range was zeroed.
>> It's wrong.
> 
> I don't quite understand what case you're considering, which breaks
> after my patch. fallocate with mode = 
> FALLOC_FL_KEEP_SIZE|FALLOC_FL_ZERO_RANGE
> and offset > file_size, should have no effect on the file.

Yes, everything looks OK for me now.

Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>
 
>>> complete fallocate with mode == FALLOC_FL_KEEP_SIZE. Also move mutex_is_locked,
>>> since all other mode combinations either have to be under mutex or not valid.
>>>
>>> #VSTOR-19317
>>>
>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>> ---
>>>   fs/fuse/kio/pcs/pcs_fuse_kdirect.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>>> index de54fedeb5e4..89866370a341 100644
>>> --- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>>> +++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>>> @@ -924,9 +924,11 @@ static void pcs_fuse_submit(struct pcs_fuse_cluster *pfc, struct fuse_req *req,
>>>   		if (inarg->offset >= di->fileinfo.attr.size)
>>>   			inarg->mode &= ~FALLOC_FL_ZERO_RANGE;
>>>   
>>> +		if (inarg->mode == FALLOC_FL_KEEP_SIZE)
>>> +			break; /* NOPE */
>>
>> This is untidy to add new branch, which makes further "if (inarg->mode & FALLOC_FL_KEEP_SIZE)"
>> check a dead code.
>>
>>> +
>>> +		WARN_ON_ONCE(!mutex_is_locked(&fi->inode.i_mutex));
>>>   		if (inarg->mode & (FALLOC_FL_ZERO_RANGE|FALLOC_FL_PUNCH_HOLE)) {
>>> -			WARN_ON_ONCE(!mutex_is_locked(&fi->inode.i_mutex));
>>> -
>>>   			if ((inarg->offset & (PAGE_SIZE - 1)) || (inarg->length & (PAGE_SIZE - 1))) {
>>>   				r->req.out.h.error = -EINVAL;
>>>   				goto error;
>>>