Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Miklos Szeredi <miklos@szeredi.hu>
Cc:     Alexey Gladkov <gladkov.alexey@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-fsdevel@vger.kernel.org, Alexey Gladkov <legion@kernel.org>
References: <1e796f9e008fb78fb96358ff74f39bd4865a7c88.1604926010.git.gladkov.alexey@gmail.com>
        <CAJfpegua_ahmNa4p0me6R10wtcPpQVKNiKQOVKjuNW67RHFOOA@mail.gmail.com>
Date:   Mon, 09 Nov 2020 12:54:36 -0600
In-Reply-To: <CAJfpegua_ahmNa4p0me6R10wtcPpQVKNiKQOVKjuNW67RHFOOA@mail.gmail.com>
        (Miklos Szeredi's message of "Mon, 9 Nov 2020 16:27:05 +0100")
Message-ID: <87v9ee2wer.fsf@x220.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [RESEND PATCH v3] fuse: Abort waiting for a response if the daemon receives a fatal signal
Precedence: bulk

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Mon, Nov 9, 2020 at 1:48 PM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
>>
>> This patch removes one kind of the deadlocks inside the fuse daemon. The
>> problem appear when the fuse daemon itself makes a file operation on its
>> filesystem and receives a fatal signal.
>>
>> This deadlock can be interrupted via fusectl filesystem. But if you have
>> many fuse mountpoints, it will be difficult to figure out which
>> connection to break.
>>
>> This patch aborts the connection if the fuse server receives a fatal
>> signal.
>
> The patch itself might be acceptable, but I have some questions.
>
> To logic of this patch says:
>
> "If a task having the fuse device open in it's fd table receives
> SIGKILL (and filesystem was initially mounted in a non-init user
> namespace), then abort the filesystem operation"
>
> You just say "server" instead of "task having the fuse device open in
> it's fd table" which is sloppy to say the least.  It might also lead
> to regressions, although I agree that it's unlikely.
>
> Also how is this solving any security issue?   Just create the request
> loop using two fuse filesystems and the deadlock avoidance has just
> been circumvented.   So AFAICS "selling" this as a CVE fix is not
> appropriate.

The original report came in with a CVE on it.  So referencing that CVE
seems reasonable.  Even if the issue isn't particularly serious.  It is
very annoying not to be able to kill processes with SIGKILL or the OOM
killer.

You have a good point about the looping issue.  I wonder if there is a
way to enhance this comparatively simple approach to prevent the more
complex scenario you mention.

Does tweaking the code to close every connection represented by a fuse
file descriptor after a SIGKILL has been delevered create any problems?

> What's the reason for making this user-ns only?  If we drop the
> security aspect, then I don't see any reason not to do this
> unconditionally.


> Also note, there's a proper solution for making fuse requests always
> killable, and that is to introduce a shadow locking that ensures
> correct fs operation in the face of requests that have returned and
> released their respective VFS locks.   Now this would be a much more
> complex solution, but also a much more correct one, not having issues
> with correctly defining what a server is (which is not a solvable
> problem).

Is this the solution that was removed at some point from fuse,
or are you talking about something else?

I think you are talking about adding a set of fuse specific locks
so fuse does not need to rely on the vfs locks.  I don't quite have
enough insight to see that bigger problem so if you can expand in more
detail I would appreciate it.

Eric