linux-kernel - Re: [PATCH] FUSE: add the async option for the flush/release operation

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] [day] [month] [year] [list]

Message-ID: <469c452c-6255-d92e-88c3-110eabdfcfef@cisco.com>
Date:	Tue, 16 Aug 2016 12:53:49 -0700
From:	Enke Chen <enkechen@...co.com>
To:	Miklos Szeredi <miklos@...redi.hu>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	"Helios Tsoi (hetsoi)" <hetsoi@...co.com>,
	Enke Chen <enkechen@...co.com>
Subject: Re: [PATCH] FUSE: add the async option for the flush/release
 operation

Hi, Miklos:

Thanks for your reply and explanation. Please see my comments below.

On 8/15/16 2:36 AM, Miklos Szeredi wrote:
> On Wed, Aug 10, 2016 at 6:50 PM, Enke Chen <enkechen@...co.com> wrote:
>> Hi, Miklos:
>>
>> On 8/9/16 11:52 PM, Miklos Szeredi wrote:
>>> On Wed, Aug 10, 2016 at 5:26 AM, Enke Chen <enkechen@...co.com> wrote:
>>>> Hi, Miklos:
>>>>
>>>> This patch adds the async option for the flush/release operation in FUSE.
>>>>
>>>> The async flush/release option allows a FUSE-based application to be terminated
>>>> without being blocked in the flush/release operation even in the presence of
>>>> complex external interactions. In addition, the async operation can be more
>>>> efficient when a large number of fuse-based files is involved.
>>>>
>>>> ---
>>>> Deadlock Example:
>>>>
>>>>     Process A is a multi-threaded application that interacts with Process B,
>>>>     a FUSE-server.
>>>>
>>>>
>>>>                UNIX-domain socket
>>>>     App (A)  -----------------------  FUSE-server (B)
>>>>        |                                   |
>>>>        |                                   |
>>>>        |                                   |
>>>>        +-----------------------------------+
>>>>                open/flush/release
>>>
>>> Why would the fuse server want to communicate with the app (using
>>> other than the filesystem)?
>>
>> In this particular case, the other communication channel is used to coordinate
>> the allocation (with "open") and de-alocation (with "flush/release") of the
>> shared memory associated with the opened "file".
>>
>> In general an application may have special handling for the "flush/release"
>> operation that involve external interactions with one or more other processes,
> 
> Sure, it can interact with other processes, but *not* with the process
> accessing the filesystem.  There are no end of possible deadlocks that
> way, and it goes straight against the design philosophy of kernel
> interfaces.
> 
> Maybe I'm missing something, but this sure looks like a case of bad
> system design.   You need to give more details to convince me
> otherwise.

On the "system design", I agree and I certainly prefer simpler external
interactions among processes.  But we do not always know what libs would
do, and when another interaction would be introduced.

> 
>> and that is where this "async" operation can help.
>>
>> IMO it would be even better if the "async" operation can be made the default so
>> folks do not need to worry about this types of deadlocks.  From reading of the
>> code, it seems that FUSE does "async" release under certain conditions already.
> 
> Release being async is okay, and is the default for non-fuseblk
> mounts.  For fuseblk it is not the default, because it could cause
> problems:
> 
>   5a18ec176c93 ("fuse: fix hang of single threaded fuseblk filesystem")
> 
> We could make release optionally async for fuseblk.
> 
> Flush being async is not okay, it needs to return an error value.  If
> the filesystem does not want to return an error value, it may omit a
> flush implementation completely.

I was not aware that the release operation is already async by default for
non-fuseblk mounts. That is really what we need in order to break the deadlock
in the example. (I had the "flush" operation there for the sake of completeness,
and not out of necessity.)

The deadlock I described was an old problem from several years ago. I just
re-ran the test with the newer kernel (3.14 and 4.7), and confirmed that the
issue is gone with the "release" operation being async.  The deadlock was also
reproduced after changing the release operation from "async" to "sync" in the
fuse module.

So the patch is no longer needed unless we want to modify it to support the
"async release" for the fuseblk.

Thanks again.  -- Enke