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

[Linux regression tracking (Thorsten Leemhuis)]

Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting
for once, to make this easily accessible to everyone.

Miklos, I'm wondering what the status here is. The description in the
reverts André sent[1] are maybe a bit vague[2], but it sounds a lot like
he ran into a big regression that should be addressed somehow -- maybe
with a revert. But it seems we haven't got any closer to that in all
those ~7 weeks since the first revert was posted. But I might be missing
something, hence a quick evaluation from your side would help me a lot
here to understand the situation.

[1]
https://lore.kernel.org/lkml/20230904133321.104584-1-git@xxxxxxxxxx/
https://lore.kernel.org/lkml/20231018111508.3913860-1-git@xxxxxxxxxx/

[2] Does this happen on all Android versions or just some? And what is
actually breaking (this was answered somewhere in the thread iirc)?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

On 18.10.23 17:51, Bernd Schubert wrote:
> 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