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

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

[André Draszik]

From: André Draszik <andre.draszik@linaro.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.

Cc: Bernd Schubert <bschubert@ddn.com>
Cc: Miklos Szeredi <mszeredi@redhat.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: André Draszik <andre.draszik@linaro.org>

---
resend because of missing people in Cc
---
 fs/fuse/inode.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 549358ffea8b..0b966b0e0962 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1132,10 +1132,7 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 		process_init_limits(fc, arg);
 
 		if (arg->minor >= 6) {
-			u64 flags = arg->flags;
-
-			if (flags & FUSE_INIT_EXT)
-				flags |= (u64) arg->flags2 << 32;
+			u64 flags = arg->flags | (u64) arg->flags2 << 32;
 
 			ra_pages = arg->max_readahead / PAGE_SIZE;
 			if (flags & FUSE_ASYNC_READ)
-- 
2.40.1

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

[Miklos Szeredi]

On Mon, 4 Sept 2023 at 15:34, André Draszik <git@andred.net> wrote:
>
> From: André Draszik <andre.draszik@linaro.org>
>
> This reverts commit 3066ff93476c35679cb07a97cce37d9bb07632ff.
>
> This patch breaks all existing userspace by requiring updates as
> mentioned in the commit message, which is not allowed.

It might break something, but you need to tell us what that is, please.

Thanks,
Miklos

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

[André Draszik]

On Mon, 2023-09-04 at 15:41 +0200, Miklos Szeredi wrote:
> On Mon, 4 Sept 2023 at 15:34, André Draszik <git@andred.net> wrote:
> > 
> > From: André Draszik <andre.draszik@linaro.org>
> > 
> > This reverts commit 3066ff93476c35679cb07a97cce37d9bb07632ff.
> > 
> > This patch breaks all existing userspace by requiring updates as
> > mentioned in the commit message, which is not allowed.
> 
> It might break something, but you need to tell us what that is, please.

In my case, it's Android.

More generally this breaks all user-spaces that haven't been updated. Not
breaking user-space is one of the top rules the kernel has, if not the topmost.


Cheers,
Andre

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

[Bernd Schubert]

On 9/4/23 16:21, André Draszik wrote:
> On Mon, 2023-09-04 at 15:41 +0200, Miklos Szeredi wrote:
>> On Mon, 4 Sept 2023 at 15:34, André Draszik <git@andred.net> wrote:
>>>
>>> From: André Draszik <andre.draszik@linaro.org>
>>>
>>> This reverts commit 3066ff93476c35679cb07a97cce37d9bb07632ff.
>>>
>>> This patch breaks all existing userspace by requiring updates as
>>> mentioned in the commit message, which is not allowed.
>>
>> It might break something, but you need to tell us what that is, please.
> 
> In my case, it's Android.
> 
> More generally this breaks all user-spaces that haven't been updated. Not
> breaking user-space is one of the top rules the kernel has, if not the topmost.


Hmm, I guess Android is using one of the extended flags in the mean time.
Do you have more data what exactly fails? I had posted this patch last year,
when it was still rather early introduction of FUSE_INIT_EXT, hoping there was
nothing in production yet using these flags. But virtiofsd was already using it,
so the patch got delayed (I had actually assumed it would just get dropped).


Sorry for the trouble!


Bernd

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

[André Draszik]

From: André Draszik <andre.draszik@linaro.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.

Cc: Bernd Schubert <bschubert@ddn.com>
Cc: Miklos Szeredi <mszeredi@redhat.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: André Draszik <andre.draszik@linaro.org>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
v2: ping & add ack
v1: resend because of missing people in Cc
---
 fs/fuse/inode.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 2e4eb7cf26fb..b21ccc85c47b 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1154,10 +1154,7 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 		process_init_limits(fc, arg);
 
 		if (arg->minor >= 6) {
-			u64 flags = arg->flags;
-
-			if (flags & FUSE_INIT_EXT)
-				flags |= (u64) arg->flags2 << 32;
+			u64 flags = arg->flags | (u64) arg->flags2 << 32;
 
 			ra_pages = arg->max_readahead / PAGE_SIZE;
 			if (flags & FUSE_ASYNC_READ)
-- 
2.40.1

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

[Bernd Schubert]

On 10/18/23 13:15, André Draszik wrote:
> From: André Draszik <andre.draszik@linaro.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?

Thanks,
Bernd

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

[André Draszik]

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@linaro.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.

Kind Regards,
Andre

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

[Bernd Schubert]

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@linaro.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? These are feature flags - is there really a 
file system that relies on these flag to the extend that it does not 
work anymore?

Also, reverting this patch has the side effect that you can ask the 
kernel to use initialized bits - which obviously has other side effects.



Thanks,
Bernd

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

[André Draszik]

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@linaro.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

>  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.



Cheers,
Andre

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

[Bernd Schubert]

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@linaro.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...

In anyway, please still consider that using possibly uninitialized flags 
is not a good idea either and could randomly break things as well.


Thanks,
Bernd

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@linaro.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

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@andred.net/
https://lore.kernel.org/lkml/20231018111508.3913860-1-git@andred.net/

[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@linaro.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

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

[Miklos Szeredi]

On Wed, Oct 25, 2023 at 1:30 PM Linux regression tracking (Thorsten
Leemhuis) <regressions@leemhuis.info> wrote:

> 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.

I don't think the Android use case counts as a regression.

If they'd use an unmodified upstream kernel, it would be a different case.

But they modify the kernel heavily, and AFAICS this breakage is
related to such a modification (as pointed out by Bernd upthread).

André might want to clarify, but I've not seen any concrete real world
examples of regressions caused by this change outside of Android.

Thanks,
Miklos

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

[Thorsten Leemhuis]

On 25.10.23 15:17, Miklos Szeredi wrote:
> On Wed, Oct 25, 2023 at 1:30 PM Linux regression tracking (Thorsten
> Leemhuis) <regressions@leemhuis.info> wrote:
> 
>> 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.

First, many thx for the reply.

> I don't think the Android use case counts as a regression.
> 
> If they'd use an unmodified upstream kernel, it would be a different case.
> 
> But they modify the kernel heavily, and AFAICS this breakage is
> related to such a modification (as pointed out by Bernd upthread).

Not sure who you mean with "they" here.

Isn't the main question if André used a vanilla kernel beforehand on
those Android devices and now is unable to do so? André, is that the
case? Or did you only encounter this regression when switching from a
patched kernel to a vanilla kernel?

Also: André, do you see this in some test env, or in some real use case
where others might also run into the problem?

> André might want to clarify, but I've not seen any concrete real world
> examples of regressions caused by this change outside of Android.

Yeah, some clarification from André really would be helpful.

Thx again for the answer.

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.

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

[Greg Kroah-Hartman]

On Wed, Oct 25, 2023 at 03:17:09PM +0200, Miklos Szeredi wrote:
> On Wed, Oct 25, 2023 at 1:30 PM Linux regression tracking (Thorsten
> Leemhuis) <regressions@leemhuis.info> wrote:
> 
> > 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.
> 
> I don't think the Android use case counts as a regression.

Why not?  In the changelog for this commit, it says:

    There is a risk with this change, though - it might break existing user
    space libraries, which are already using flags2 without setting
    FUSE_INIT_EXT.

And that's exactly what Android was doing.  Not all the world uses libfuse,
unfortunatly.

Yes, Android did have an out-of-tree change to support a fuse extension that is
not accepted upstream yet (but I think they submitted it already), and
they had to figure out the "safest" way to do so to keep compability
with everything else.

Now yes, that attempt failed, and now older Android userspace breaks
with newer kernels because of this commit, which you all even agreed
might happen here!

So either you have a policy of "we only care about libfuse use cases for
this api", or you don't, which is fine, just say so.  But that's not
what the changelog says.

> If they'd use an unmodified upstream kernel, it would be a different case.
> 
> But they modify the kernel heavily, and AFAICS this breakage is
> related to such a modification (as pointed out by Bernd upthread).

They add a new fuse extension, yes.  How do you suggest they do so in an
abi-safe way for the future when features are not accepted by upstream?

> André might want to clarify, but I've not seen any concrete real world
> examples of regressions caused by this change outside of Android.

Android is _only_ a few billion devices, it doesn't get much more "real
world" than that.  All other Linux instances are just a rounding error :)

thanks,

gre gk-h

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

[Miklos Szeredi]

On Fri, Oct 27, 2023 at 3:12 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:

> So because Android userspace is sending a flag value that is not in the
> upstream table, this breakage is ok?  Or do you mean something else, I'm
> getting confused.

From my POV the regression in the Android kernel was due to the
Android patch that added those flags.

Not all flags are equal, some applications use a specific set of flags
and another set of applications use another set.  Non-Android apps
won't use the flag that Android added, for obvious reasons.

I still don't see why we'd need to revert this patch due to
regressions in Android.  Maybe I'm really dumb, but I just don't get
it.

Thanks,
Miklos

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

[Linux regression tracking (Thorsten Leemhuis)]

On 27.10.23 16:05, Miklos Szeredi wrote:
> On Fri, Oct 27, 2023 at 3:39 PM André Draszik <andre.draszik@linaro.org> wrote:
>> On Fri, 2023-10-27 at 15:24 +0200, Miklos Szeredi wrote:
>>> On Fri, Oct 27, 2023 at 3:14 PM André Draszik
>>> <andre.draszik@linaro.org> wrote:
>>>
>>>> The patch in question has broken all users that use the higher
>>>> flags
>>>> and that don't use your version of libfuse, not just Android.
>>>> You're
>>>> filtering them out now when you didn't at the time that those
>>>> ('official) high flags were added. There are a couple more high
>>>> flags
>>>> than just the one that Android added.
>>>
>>> Okay.  Where are all those users?
>>
>> That's not the point. The point is the kernel<->user API has rendered
>> them too non-working.
> 
> It is a very important point.  A theoretical bug isn't a regression.
> Nor is a broken test case BTW.
> 
> Please read section 'What is a "regression" and what is the "no
> regressions rule"?' in
> Documentation/admin-guide/reporting-regressions.rst.

I'm taken a bit back and forth here and it seems we are stuck again. So
let me try again to hopefully clear things up a bit:

André, could you please state

* What practical use-case actually stopped working?

* What Linux kernel version actually worked for your (because if thing
broke when you upgraded from a vendor kernel to a vanilla kernel than
this does not qualify as regression IMHO)

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.

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

[André Draszik]

Hi Thorsten,

(sorry for the slow reply)

On Wed, 2023-11-01 at 13:36 +0100, Linux regression tracking (Thorsten
Leemhuis) wrote:
> I'm taken a bit back and forth here and it seems we are stuck again.
> So
> let me try again to hopefully clear things up a bit:
> 
> André, could you please state
> 
> * What practical use-case actually stopped working?

It's impossible to use a newer kernel together with an older user space
which is something that Android had been supporting for a long time.

> * What Linux kernel version actually worked for your (because if
> thing
> broke when you upgraded from a vendor kernel to a vanilla kernel than
> this does not qualify as regression IMHO)

We are using the Android kernel in all cases and Android applies
patches on top of Linus' tree, yes (as does everybody else). The
previous Android kernel worked, the current Android kernel doesn't
because of the patch in question.


I think Greg made some valid points before:
https://lore.kernel.org/all/2023102731-wobbly-glimpse-97f5@gregkh/
> I'm talking about a patch where you are changing the existing
> user/kernel api by filtering out values that you previously accepted.
> And it was done in a patch saying "this might break userspace", and
> guess what, it did!

I guess it boils down to an an agreement regarding Greg's previous
questions/points:
https://lore.kernel.org/all/2023102757-cornflake-pry-e788@gregkh/
> So because Android userspace is sending a flag value that is not in
> the upstream table, this breakage is ok?

and https://lore.kernel.org/all/2023102740-think-hatless-ab87@gregkh/ 
> now older Android userspace breaks with newer kernels because of this
> commit, which you all even agreed might happen here!
>
> So either you have a policy of "we only care about libfuse use cases
> for this api", or you don't, which is fine, just say so.  But that's
> not what the changelog says.

But I agree, it seems we're stuck and I'm not sure how to resolve this
either, Miklos has his points, Android has a different position.


Cheers,
Andre'

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

[Miklos Szeredi]

On Wed, Nov 8, 2023 at 11:31 AM André Draszik <andre.draszik@linaro.org> wrote:

> We are using the Android kernel in all cases and Android applies
> patches on top of Linus' tree, yes (as does everybody else). The
> previous Android kernel worked, the current Android kernel doesn't
> because of the patch in question.

Why don't you revert the patch in question in the Android kernel?

Thanks,
Miklos

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

[Linux regression tracking (Thorsten Leemhuis)]

On 08.11.23 11:31, André Draszik wrote:
> [...]
> But I agree, it seems we're stuck and I'm not sure how to resolve this
> either, Miklos has his points, Android has a different position.

FWIW, this thread died down without any agreement if this is an
regression or not. Continuing to track it as one likely is not worth the
effort, hence I'll remove it from the list of tracked issues. If anyone
still thinks this is something that should be fixed I'd say that person
should revive this thread and bring Linus in (but FWIW, I pointed him at
this thread once already).

#regzbot inconclusive: people can't agree if this is a regression or not
and it seems people stopped caring
#regzbot ignore-activity

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.

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

[Linux regression tracking #adding (Thorsten Leemhuis)]

[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]

On 04.09.23 15:33, André Draszik wrote:
> From: André Draszik <andre.draszik@linaro.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.

Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:

#regzbot ^introduced 3066ff93476c35679cb07a97cce37d9bb07632
#regzbot title fuse: creating new files, or reading existing files in i
Android, now returns -EFAULT
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.

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
That page also explains what to do if mails like this annoy you.