linux-kernel - Re: [PATCH v2] Revert "fuse: Apply flags2 only when userspace set the FUSE_INIT

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: <5cd87a64-c506-46f2-9fed-ac8a74658631@ddn.com>
Date:   Wed, 18 Oct 2023 15:51:19 +0000
From:   Bernd Schubert <bschubert@....com>
To:     André Draszik <andre.draszik@...aro.org>
CC:     Miklos Szeredi <mszeredi@...hat.com>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Paul Lawrence <paullawrence@...gle.com>,
        Daniel Rosenberg <drosen@...gle.com>,
        Alessio Balsini <balsini@...roid.com>,
        Amir Goldstein <amir73il@...il.com>
Subject: Re: [PATCH v2] Revert "fuse: Apply flags2 only when userspace set the
 FUSE_INIT_EXT"

On 10/18/23 16:40, Bernd Schubert wrote:
> On 10/18/23 16:26, André Draszik wrote:
>> On Wed, 2023-10-18 at 11:52 +0000, Bernd Schubert wrote:
>>> On 10/18/23 13:46, André Draszik wrote:
>>>> On Wed, 2023-10-18 at 11:39 +0000, Bernd Schubert wrote:
>>>>> On 10/18/23 13:15, André Draszik wrote:
>>>>>> From: André Draszik <andre.draszik@...aro.org>
>>>>>>
>>>>>> This reverts commit 3066ff93476c35679cb07a97cce37d9bb07632ff.
>>>>>>
>>>>>> This patch breaks all existing userspace by requiring updates
>>>>>> as
>>>>>> mentioned in the commit message, which is not allowed.
>>>>>>
>>>>>> Revert to restore compatibility with existing userspace
>>>>>> implementations.
>>>>>
>>>>> Which fuse file system does it exactly break? In fact there
>>>>> haven't
>>>>> been
>>>>> added too many flags after - what exactly is broken?
>>>>
>>>> The original patch broke the existing kernel <-> user ABI by now
>>>> requiring user space applications to pass in an extra flag.
>>>> There are various side-effects of this, like unbootable systems,
>>>> just
>>>> because the kernel was updated.
>>>> Breaking the ABI is the one thing that is not allowed. This is not
>>>> specific to any particular fuse file system.
>>>
>>> How exactly did it break it?
>>
>> At least in Android, creating new files, or reading existing files
>> returns -EFAULT
> 
> Hmm, could you please point me to the corresponding android userspace 
> library? I guess it is not using libfuse? At least I would like to 
> understand the issue...
> 
>>
>>>   These are feature flags - is there really a
>>> file system that relies on these flag to the extend that it does not
>>> work anymore?
>>
>> I don't know enough about the implementation details, but even outside
>> Android user space had to be updated as a prerequisite for this kernel
>> patch:
>> https://lore.kernel.org/all/YmUKZQKNAGimupv7@redhat.com/
>> https://github.com/libfuse/libfuse/pull/662
>>
>> Which means any non-Android user space predating those changes isn't
>> working anymore either.
> 
> The patch in libfuse is from me, there was nothing broken.
> And I don't think that any of the additional flags added are a 
> _requirement_ for libfuse file systems to work. I'm not sure if DAX and 
> the other flags before the patch was merged are a _requirement_ for 
> virtiofsd or just a nice feature to have...


Looking at the android kernel source:

/*
  * For FUSE < 7.36 FUSE_PASSTHROUGH has value (1 << 31).
  * This condition check is not really required, but would prevent having a
  * broken commit in the tree.
  */
#if FUSE_KERNEL_VERSION > 7 ||                                                 \
         (FUSE_KERNEL_VERSION == 7 && FUSE_KERNEL_MINOR_VERSION >= 36)
#define FUSE_PASSTHROUGH (1ULL << 63)
#else
#define FUSE_PASSTHROUGH (1 << 31)
#endif


So passthrough gets broken with this check and android heavily uses that.
Would be interesting to know if this could result in EFAULT.


Thanks,
Bernd