Re: [PATCH v2] Revert "fuse: Apply flags2 only when userspace set the FUSE_INIT_EXT"

[Bernd Schubert]

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@xxxxxxxxxx>
>>>>>>
>>>>>> 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@xxxxxxxxxx/
>> 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