fs/fuse/inode.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
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
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
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
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
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
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
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
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
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
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
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.
© 2016 - 2025 Red Hat, Inc.