linux-kernel - Re: [PATCH 5/6] fuse: fix synchronous case of fuse_file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5411CA4F.7040006@parallels.com>
Date:	Thu, 11 Sep 2014 20:14:07 +0400
From:	Maxim Patlasov <mpatlasov@...allels.com>
To:	Miklos Szeredi <miklos@...redi.hu>
CC:	fuse-devel <fuse-devel@...ts.sourceforge.net>,
	Anand Avati <avati@...ster.org>,
	Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5/6] fuse: fix synchronous case of fuse_file_put()

On 08/22/2014 06:08 PM, Miklos Szeredi wrote:
> On Thu, Aug 21, 2014 at 6:09 PM, Maxim Patlasov <MPatlasov@...allels.com> wrote:
>> If fuse_file_put() is called with sync==true, the user may be blocked for
>> a while, until userspace ACKs our FUSE_RELEASE request. This blocking must be
>> uninterruptible. Otherwise request could be interrupted, but file association
>> in user space remains.
>>
>> Signed-off-by: Maxim Patlasov <mpatlasov@...allels.com>
>> ---
>>   fs/fuse/file.c |    4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index cd55488..b92143a 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -136,6 +136,10 @@ static void fuse_file_put(struct fuse_file *ff, bool sync)
>>                          path_put(&req->misc.release.path);
>>                          fuse_put_request(ff->fc, req);
>>                  } else if (sync) {
>> +                       /* Must force. Otherwise request could be interrupted,
>> +                        * but file association in user space remains.
>> +                        */
>> +                       req->force = 1;
>>                          req->background = 0;
>>                          fuse_request_send(ff->fc, req);
>>                          path_put(&req->misc.release.path);
>>
>
> Some thought needs to go into this:  if RELEASE is interrupted, then
> we should possibly allow that, effectively backgrounding the request.
>
> The synchronous nature is just an optimization and we really don't
> know where we are being interrupted, possibly in a place which very
> much *should* allow interruption.

I really need your help to proceed with this patch. Could you please 
explain what those places are where we should allow interruption.

BTW, as for "just an optimization", I've recently noticed that __fput() 
calls locks_remove_file(). So guarantees provided by the patch-set are 
on the same level as flock(2) behaviour.

>
> Also fuse really should distinguish fatal and non-fatal interruptions
> and handle them accordingly...

And elaborate on this concern, please.

Thanks,
Maxim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/