overlayfs may fail to complete updates when a filesystem lacks
fileattr/xattr syscall support and responds with an ENOSYS error code,
resulting in an unexpected "Function not implemented" error.
This bug may occur with FUSE filesystems, such as davfs2.
Steps to reproduce:
# install davfs2, e.g., apk add davfs2
mkdir /test mkdir /test/lower /test/upper /test/work /test/mnt
yes '' | mount -t davfs -o ro http://some-web-dav-server/path \
/test/lower
mount -t overlay -o upperdir=/test/upper,lowerdir=/test/lower \
-o workdir=/test/work overlay /test/mnt
# when "some-file" exists in the lowerdir, this fails with "Function
# not implemented", with dmesg showing "overlayfs: failed to retrieve
# lower fileattr (/some-file, err=-38)"
touch /test/mnt/some-file
This bug is related to a regression in v5.15 that was partially fixed in
v5.16.
This patch also adds checks for ENOSYS in case the upper file system
does not support file attributes. That change is related to a partial
fix in v5.17.
Reported-by: Christian Kohlschütter <[email protected]>
Fixes: 5b0a414d06c ("ovl: fix filattr copy-up failure")
Fixes: 24d7f48c723 ("ovl: don't fail copy up if no fileattr support on upper")
Cc: <[email protected]> # v5.15
Signed-off-by: Christian Kohlschütter <[email protected]>
---
fs/overlayfs/copy_up.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 714ec569d25b..0ad88573e77a 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -142,7 +142,7 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old,
err = ovl_real_fileattr_get(old, &oldfa);
if (err) {
/* Ntfs-3g returns -EINVAL for "no fileattr support" */
- if (err == -ENOTTY || err == -EINVAL)
+ if (err == -ENOTTY || err == -EINVAL || err == -ENOSYS)
return 0;
pr_warn("failed to retrieve lower fileattr (%pd2, err=%i)\n",
old->dentry, err);
@@ -173,7 +173,7 @@ static int ovl_copy_fileattr(struct inode *inode, struct path *old,
* Returning an error if upper doesn't support fileattr will
* result in a regression, so revert to the old behavior.
*/
- if (err == -ENOTTY || err == -EINVAL) {
+ if (err == -ENOTTY || err == -EINVAL || err == -ENOSYS) {
pr_warn_once("copying fileattr: no support on upper\n");
return 0;
}
--
2.36.1
On Mon, 4 Jul 2022 at 20:36, Christian Kohlschütter
<[email protected]> wrote:
>
> overlayfs may fail to complete updates when a filesystem lacks
> fileattr/xattr syscall support and responds with an ENOSYS error code,
> resulting in an unexpected "Function not implemented" error.
Issue seems to be with fuse: nothing should be returning ENOSYS to
userspace except the syscall lookup code itself. ENOSYS means that
the syscall does not exist.
Fuse uses ENOSYS in the protocol to indicate that the filesystem does
not support that operation, but that's not the value that the
filesystem should be returning to userspace.
The getxattr/setxattr implementations already translate ENOSYS to
EOPNOTSUPP, but ioctl doesn't.
The attached patch (untested) should do this. Can you please give it a try?
Thanks,
Miklos
> Am 18.07.2022 um 11:14 schrieb Miklos Szeredi <[email protected]>:
>
> On Mon, 4 Jul 2022 at 20:36, Christian Kohlschütter
> <[email protected]> wrote:
>>
>> overlayfs may fail to complete updates when a filesystem lacks
>> fileattr/xattr syscall support and responds with an ENOSYS error code,
>> resulting in an unexpected "Function not implemented" error.
>
> Issue seems to be with fuse: nothing should be returning ENOSYS to
> userspace except the syscall lookup code itself. ENOSYS means that
> the syscall does not exist.
>
> Fuse uses ENOSYS in the protocol to indicate that the filesystem does
> not support that operation, but that's not the value that the
> filesystem should be returning to userspace.
>
> The getxattr/setxattr implementations already translate ENOSYS to
> EOPNOTSUPP, but ioctl doesn't.
>
> The attached patch (untested) should do this. Can you please give it a try?
>
> Thanks,
> Miklos
> <fuse-ioctl-translate-enosys.patch>
Yes, that change basically has the same effect for the demo use case,.
However: it will change (and potentially) break assumptions in user space. We should never break user space.
Example: lsattr /test/lower
Currently, fuse returns ENOSYS, e.g.
> lsattr: reading ./lost+found: Function not implemented
With your change, it would return ENOTTY
> lsattr: reading ./lost+found: Not a tty
I also tried the setup (without patches) on a very old 4.4.176 system, and everything works fine. ovl introduced the regression, so it should also be fixed there.
It may affect other filing systems as well (I see some other fs also return ENOSYS on occasion).
It's safe to say that adding the ENOSYS to the ovl code is probably the best move. Besides, you already have a workaround for ntfs-3g there as well.
Best,
Christian
On Mon, 18 Jul 2022 at 12:10, Christian Kohlschütter
<[email protected]> wrote:
>
> > Am 18.07.2022 um 11:14 schrieb Miklos Szeredi <[email protected]>:
> >
> > On Mon, 4 Jul 2022 at 20:36, Christian Kohlschütter
> > <[email protected]> wrote:
> >>
> >> overlayfs may fail to complete updates when a filesystem lacks
> >> fileattr/xattr syscall support and responds with an ENOSYS error code,
> >> resulting in an unexpected "Function not implemented" error.
> >
> > Issue seems to be with fuse: nothing should be returning ENOSYS to
> > userspace except the syscall lookup code itself. ENOSYS means that
> > the syscall does not exist.
> >
> > Fuse uses ENOSYS in the protocol to indicate that the filesystem does
> > not support that operation, but that's not the value that the
> > filesystem should be returning to userspace.
> >
> > The getxattr/setxattr implementations already translate ENOSYS to
> > EOPNOTSUPP, but ioctl doesn't.
> >
> > The attached patch (untested) should do this. Can you please give it a try?
> >
> > Thanks,
> > Miklos
> > <fuse-ioctl-translate-enosys.patch>
>
> Yes, that change basically has the same effect for the demo use case,.
>
> However: it will change (and potentially) break assumptions in user space. We should never break user space.
>
> Example: lsattr /test/lower
> Currently, fuse returns ENOSYS, e.g.
> > lsattr: reading ./lost+found: Function not implemented
> With your change, it would return ENOTTY
> > lsattr: reading ./lost+found: Not a tty
No, it would return success.
> I also tried the setup (without patches) on a very old 4.4.176 system, and everything works fine. ovl introduced the regression, so it should also be fixed there.
> It may affect other filing systems as well (I see some other fs also return ENOSYS on occasion).
>
> It's safe to say that adding the ENOSYS to the ovl code is probably the best move. Besides, you already have a workaround for ntfs-3g there as well.
Flawed arguments. The change in overlayfs just made the preexisting
bug in fuse visible. The bug should still be fixed in fuse.
Thanks,
Miklos
--
Dr. Christian Kohlschütter
> Am 18.07.2022 um 12:31 schrieb Miklos Szeredi <[email protected]>:
>
> On Mon, 18 Jul 2022 at 12:10, Christian Kohlschütter
> <[email protected]> wrote:
>>
>>> Am 18.07.2022 um 11:14 schrieb Miklos Szeredi <[email protected]>:
>>>
>>> On Mon, 4 Jul 2022 at 20:36, Christian Kohlschütter
>>> <[email protected]> wrote:
>>>>
>>>> overlayfs may fail to complete updates when a filesystem lacks
>>>> fileattr/xattr syscall support and responds with an ENOSYS error code,
>>>> resulting in an unexpected "Function not implemented" error.
>>>
>>> Issue seems to be with fuse: nothing should be returning ENOSYS to
>>> userspace except the syscall lookup code itself. ENOSYS means that
>>> the syscall does not exist.
>>>
>>> Fuse uses ENOSYS in the protocol to indicate that the filesystem does
>>> not support that operation, but that's not the value that the
>>> filesystem should be returning to userspace.
>>>
>>> The getxattr/setxattr implementations already translate ENOSYS to
>>> EOPNOTSUPP, but ioctl doesn't.
>>>
>>> The attached patch (untested) should do this. Can you please give it a try?
>>>
>>> Thanks,
>>> Miklos
>>> <fuse-ioctl-translate-enosys.patch>
>>
>> Yes, that change basically has the same effect for the demo use case,.
>>
>> However: it will change (and potentially) break assumptions in user space. We should never break user space.
>>
>> Example: lsattr /test/lower
>> Currently, fuse returns ENOSYS, e.g.
>>> lsattr: reading ./lost+found: Function not implemented
>> With your change, it would return ENOTTY
>>> lsattr: reading ./lost+found: Not a tty
>
> No, it would return success.
I'm referring to /test/lower (powered by fuse davfs2), not /test/mnt (overlayfs).
>
>> I also tried the setup (without patches) on a very old 4.4.176 system, and everything works fine. ovl introduced the regression, so it should also be fixed there.
>> It may affect other filing systems as well (I see some other fs also return ENOSYS on occasion).
>>
>> It's safe to say that adding the ENOSYS to the ovl code is probably the best move. Besides, you already have a workaround for ntfs-3g there as well.
>
> Flawed arguments. The change in overlayfs just made the preexisting
> bug in fuse visible. The bug should still be fixed in fuse.
I understand your point from ovl's perspective, however you are proposing a fix in fuse, and so we have to see it from fuse's perspective (and its users).
From ovl's point of view, your patch fixes it because the behavior in ovl will now become how it was before the regression was introduced.
However, users of fuse that have no business with overlayfs suddenly see their ioctl return ENOTTY instead of ENOSYS.
On Mon, 18 Jul 2022 at 12:56, Christian Kohlschütter
<[email protected]> wrote:
> However, users of fuse that have no business with overlayfs suddenly see their ioctl return ENOTTY instead of ENOSYS.
And returning ENOTTY is the correct behavior. See this comment in
<asm-generic/errrno.h>:
/*
* This error code is special: arch syscall entry code will return
* -ENOSYS if users try to call a syscall that doesn't exist. To keep
* failures of syscalls that really do exist distinguishable from
* failures due to attempts to use a nonexistent syscall, syscall
* implementations should refrain from returning -ENOSYS.
*/
#define ENOSYS 38 /* Invalid system call number */
Thanks,
Miklos
Am 18.07.2022 um 14:21 schrieb Miklos Szeredi <[email protected]>:
>
> On Mon, 18 Jul 2022 at 12:56, Christian Kohlschütter
> <[email protected]> wrote:
>
>> However, users of fuse that have no business with overlayfs suddenly see their ioctl return ENOTTY instead of ENOSYS.
>
> And returning ENOTTY is the correct behavior. See this comment in
> <asm-generic/errrno.h>:
>
> /*
> * This error code is special: arch syscall entry code will return
> * -ENOSYS if users try to call a syscall that doesn't exist. To keep
> * failures of syscalls that really do exist distinguishable from
> * failures due to attempts to use a nonexistent syscall, syscall
> * implementations should refrain from returning -ENOSYS.
> */
> #define ENOSYS 38 /* Invalid system call number */
>
> Thanks,
> Miklos
That ship is sailed since ENOSYS was returned to user-space for the first time.
It reminds me a bit of Linus' "we do not break userspace" email from 2012 [1, 2], where Linus wrote:
> Applications *do* care about error return values. There's no way in
> hell you can willy-nilly just change them. And if you do change them,
> and applications break, there is no way in hell you can then blame the
> application.
(Adding Linus for clarification whether his statement is still true in 2022, and marking subject with regression tag for visibility.)
As far as I, a fuse user, understand, fuse uses ENOSYS as a specific marker to indicate permanent failure:
From libfuse https://libfuse.github.io/doxygen/structfuse__lowlevel__ops.html:
> If this request is answered with an error code of ENOSYS, this is treated as a permanent failure with error code EOPNOTSUPP, i.e. all future getxattr() requests will fail with EOPNOTSUPP without being send to the filesystem process.
(also see https://man.openbsd.org/fuse_new.3)
Again, in summary:
1. I believe the fix should be in ovl, because ovl caused the regression.
2. Fixing in fuse would not be sufficient because other lower filesystems that also return ENOSYS remain affected (a cursory search indicates ecryptfs also returns ENOSYS).
3. Fixing in fuse would break user-space. We do not break userspace.
Cheers,
Christian
[1] https://lkml.org/lkml/2012/12/23/75
[2] https://lkml.org/lkml/2012/12/23/99
On Mon, 18 Jul 2022 at 15:03, Christian Kohlschütter
<[email protected]> wrote:
>
> Am 18.07.2022 um 14:21 schrieb Miklos Szeredi <[email protected]>:
> >
> > On Mon, 18 Jul 2022 at 12:56, Christian Kohlschütter
> > <[email protected]> wrote:
> >
> >> However, users of fuse that have no business with overlayfs suddenly see their ioctl return ENOTTY instead of ENOSYS.
> >
> > And returning ENOTTY is the correct behavior. See this comment in
> > <asm-generic/errrno.h>:
> >
> > /*
> > * This error code is special: arch syscall entry code will return
> > * -ENOSYS if users try to call a syscall that doesn't exist. To keep
> > * failures of syscalls that really do exist distinguishable from
> > * failures due to attempts to use a nonexistent syscall, syscall
> > * implementations should refrain from returning -ENOSYS.
> > */
> > #define ENOSYS 38 /* Invalid system call number */
> >
> > Thanks,
> > Miklos
>
> That ship is sailed since ENOSYS was returned to user-space for the first time.
>
> It reminds me a bit of Linus' "we do not break userspace" email from 2012 [1, 2], where Linus wrote:
> > Applications *do* care about error return values. There's no way in
> > hell you can willy-nilly just change them. And if you do change them,
> > and applications break, there is no way in hell you can then blame the
> > application.
Correct. The question is whether any application would break in this
case. I think not, but you are free to prove otherwise.
Thanks,
Miklos
> Am 18.07.2022 um 15:13 schrieb Miklos Szeredi <[email protected]>:
>
> On Mon, 18 Jul 2022 at 15:03, Christian Kohlschütter
> <[email protected]> wrote:
>>
>> Am 18.07.2022 um 14:21 schrieb Miklos Szeredi <[email protected]>:
>>>
>>> On Mon, 18 Jul 2022 at 12:56, Christian Kohlschütter
>>> <[email protected]> wrote:
>>>
>>>> However, users of fuse that have no business with overlayfs suddenly see their ioctl return ENOTTY instead of ENOSYS.
>>>
>>> And returning ENOTTY is the correct behavior. See this comment in
>>> <asm-generic/errrno.h>:
>>>
>>> /*
>>> * This error code is special: arch syscall entry code will return
>>> * -ENOSYS if users try to call a syscall that doesn't exist. To keep
>>> * failures of syscalls that really do exist distinguishable from
>>> * failures due to attempts to use a nonexistent syscall, syscall
>>> * implementations should refrain from returning -ENOSYS.
>>> */
>>> #define ENOSYS 38 /* Invalid system call number */
>>>
>>> Thanks,
>>> Miklos
>>
>> That ship is sailed since ENOSYS was returned to user-space for the first time.
>>
>> It reminds me a bit of Linus' "we do not break userspace" email from 2012 [1, 2], where Linus wrote:
>>> Applications *do* care about error return values. There's no way in
>>> hell you can willy-nilly just change them. And if you do change them,
>>> and applications break, there is no way in hell you can then blame the
>>> application.
>
> Correct. The question is whether any application would break in this
> case. I think not, but you are free to prove otherwise.
>
> Thanks,
> Miklos
I'm not going to do that since I expect any answer I give would not change your position here. All I know is there is a non-zero chance such programs exist.
If you're willing to go ahead with the fuse change you proposed, I see no purpose in debating with you further since you're the kernel maintainer of both file systems.
That change "fixes" the problem that I had seen in my setup; I do not know the extent of side effects, but I expect some could surface eventually.
Once you're done fixing fuse, please also talk to the folks over at https://github.com/trapexit/mergerfs who explicitly return ENOSYS upon request. Who knows, maybe someone is audacious enough to try mergerfs as a lower filesystem for overlay?
Alas, I think this a clash between the philosophies of writing robust code versus writing against a personal interpretation of some specification.
You refer to "asm-generic/errno.h" as the specification and rationale for treating ENOSYS as sacrosanct. Note that the comment says "should refrain from", it doesn't say "must not", and that's why we're in this mess.
It therefore wouldn't hurt to be lenient when a lower filesystem returns an error code known to refer to "unsupported operation", and that's what my original patch to ovl does.
I thought this approach would resonate with you, since you must have been following the same logic when you added the special-case check for "EINVAL" as an exception for ntfs-3g in the commit that most likely triggered the regression ("ovl: fix filattr copy-up failure") 9 months ago.
I honestly wonder why you're risking further breakage, having introduced that regression only recently.
So long,
Christian
On 7/18/22 10:25, Christian Kohlschütter wrote:
>> Am 18.07.2022 um 15:13 schrieb Miklos Szeredi <[email protected]>:
>>
>> On Mon, 18 Jul 2022 at 15:03, Christian Kohlschütter
>> <[email protected]> wrote:
>>> Am 18.07.2022 um 14:21 schrieb Miklos Szeredi <[email protected]>:
>>>> On Mon, 18 Jul 2022 at 12:56, Christian Kohlschütter
>>>> <[email protected]> wrote:
>>>>
>>>>> However, users of fuse that have no business with overlayfs suddenly see their ioctl return ENOTTY instead of ENOSYS.
>>>> And returning ENOTTY is the correct behavior. See this comment in
>>>> <asm-generic/errrno.h>:
>>>>
>>>> /*
>>>> * This error code is special: arch syscall entry code will return
>>>> * -ENOSYS if users try to call a syscall that doesn't exist. To keep
>>>> * failures of syscalls that really do exist distinguishable from
>>>> * failures due to attempts to use a nonexistent syscall, syscall
>>>> * implementations should refrain from returning -ENOSYS.
>>>> */
>>>> #define ENOSYS 38 /* Invalid system call number */
>>>>
>>>> Thanks,
>>>> Miklos
>>> That ship is sailed since ENOSYS was returned to user-space for the first time.
>>>
>>> It reminds me a bit of Linus' "we do not break userspace" email from 2012 [1, 2], where Linus wrote:
>>>> Applications *do* care about error return values. There's no way in
>>>> hell you can willy-nilly just change them. And if you do change them,
>>>> and applications break, there is no way in hell you can then blame the
>>>> application.
>> Correct. The question is whether any application would break in this
>> case. I think not, but you are free to prove otherwise.
>>
>> Thanks,
>> Miklos
> I'm not going to do that since I expect any answer I give would not change your position here. All I know is there is a non-zero chance such programs exist.
>
> If you're willing to go ahead with the fuse change you proposed, I see no purpose in debating with you further since you're the kernel maintainer of both file systems.
> That change "fixes" the problem that I had seen in my setup; I do not know the extent of side effects, but I expect some could surface eventually.
>
> Once you're done fixing fuse, please also talk to the folks over at https://github.com/trapexit/mergerfs who explicitly return ENOSYS upon request. Who knows, maybe someone is audacious enough to try mergerfs as a lower filesystem for overlay?
>
> Alas, I think this a clash between the philosophies of writing robust code versus writing against a personal interpretation of some specification.
> You refer to "asm-generic/errno.h" as the specification and rationale for treating ENOSYS as sacrosanct. Note that the comment says "should refrain from", it doesn't say "must not", and that's why we're in this mess.
>
> It therefore wouldn't hurt to be lenient when a lower filesystem returns an error code known to refer to "unsupported operation", and that's what my original patch to ovl does.
>
> I thought this approach would resonate with you, since you must have been following the same logic when you added the special-case check for "EINVAL" as an exception for ntfs-3g in the commit that most likely triggered the regression ("ovl: fix filattr copy-up failure") 9 months ago.
>
> I honestly wonder why you're risking further breakage, having introduced that regression only recently.
>
> So long,
> Christian
Author of mergerfs here. What are you referring to exactly? It's
possible I'm forgetting something but I should only be returning ENOSYS
in similar cases to libfuse where some function is not supported or when
wishing to disable xattr calls.
On Mon, 18 Jul 2022 at 16:25, Christian Kohlschütter
<[email protected]> wrote:
>
> > Am 18.07.2022 um 15:13 schrieb Miklos Szeredi <[email protected]>:
> > Correct. The question is whether any application would break in this
> > case. I think not, but you are free to prove otherwise.
> >
> > Thanks,
> > Miklos
>
> I'm not going to do that since I expect any answer I give would not change your position here. All I know is there is a non-zero chance such programs exist.
If you (or anyone) gave a real life example of an application relying
on e.g. ioctl(..., FS_IOC_SETFLAGS) returning ENOSYS, then I would
have no choice but to revert this change.
However I think that's highly unlikely given that such an application
would only have been tested on fuse filesystems and also given that
very few (if any) fuse filesystems support these ioctls in the first
place.
Thanks,
Miklos
On Mon, Jul 18, 2022 at 6:13 AM Miklos Szeredi <[email protected]> wrote:
>
> Correct. The question is whether any application would break in this
> case. I think not, but you are free to prove otherwise.
Most often, an error is "just an error", and most applications usually
won't care.
There are exceptions: some errors are very much "do something special"
(eg EAGAIN or EINTR _are_ often separately tested for and often mean
"just retry"). And permission error handling is often different from
EINVAL etc.
And ENOSYS can easily be such an error - people probing whether they
are running on a new kernel that supports a new system call or not.
And yeah, some of our ioctl's are odd, and we have a lot of drivers
(and driver infrastructure) that basically does "this device does not
support this ioctl, so return ENOSYS".
I don't think that's the right thing to do, but I think it's
understandable. The traditional error for "this device does not
support this ioctl" is ENOTTY, which sounds so crazy to non-tty people
that I understand why people have used ENOSYS instead.
It's sad that it's called "ENOTTY" and some (at least historical)
strerror() implementations will indeed return "Not a tty". Never mind
that modern ones will say "inappropriate ioctl for device" - even when
the string has been updated, the error number isn't called
EINAPPROPRAITEDEVICE.
But it is what it is, and so I think ENOTTY is understandably not used
in many situations just because it's such a senseless historical name.
And so if people don't use ENOSYS, they use EINVAL.
I *suspect* no application cares: partly because ioctl error numbers
are so random anyway, but also very much if this is a "without
overlayfs it does X, with overlayfs it does Y".
The sanest thing to do is likely to make ovl match what a non-ovl
setup would do in the same situation (_either_ of the overlaid
filesystems - there might be multiple cases).
But I'm missing the original report. It sounds like there was a
regression and we already have a case of "changing the error number
broke something". If so, that regression should be fixed.
In general, I'm perfectly happy with people fixing error numbers and
changing them.
The only thing I require is that if those cleanups and fixes are
reported to break something, people quickly revert (and preferably add
a big comment about "Use *this* error number, because while this
*other* error number would make sense, application XyZ expects AbC"..)
Linus
Am 18.07.2022 um 20:29 schrieb Linus Torvalds <[email protected]>:
>
> On Mon, Jul 18, 2022 at 6:13 AM Miklos Szeredi <[email protected]> wrote:
>>
>> Correct. The question is whether any application would break in this
>> case. I think not, but you are free to prove otherwise.
>
> Most often, an error is "just an error", and most applications usually
> won't care.
>
> There are exceptions: some errors are very much "do something special"
> (eg EAGAIN or EINTR _are_ often separately tested for and often mean
> "just retry"). And permission error handling is often different from
> EINVAL etc.
>
> And ENOSYS can easily be such an error - people probing whether they
> are running on a new kernel that supports a new system call or not.
>
> And yeah, some of our ioctl's are odd, and we have a lot of drivers
> (and driver infrastructure) that basically does "this device does not
> support this ioctl, so return ENOSYS".
>
> I don't think that's the right thing to do, but I think it's
> understandable. The traditional error for "this device does not
> support this ioctl" is ENOTTY, which sounds so crazy to non-tty people
> that I understand why people have used ENOSYS instead.
>
> It's sad that it's called "ENOTTY" and some (at least historical)
> strerror() implementations will indeed return "Not a tty". Never mind
> that modern ones will say "inappropriate ioctl for device" - even when
> the string has been updated, the error number isn't called
> EINAPPROPRAITEDEVICE.
>
> But it is what it is, and so I think ENOTTY is understandably not used
> in many situations just because it's such a senseless historical name.
>
> And so if people don't use ENOSYS, they use EINVAL.
>
> I *suspect* no application cares: partly because ioctl error numbers
> are so random anyway, but also very much if this is a "without
> overlayfs it does X, with overlayfs it does Y".
>
> The sanest thing to do is likely to make ovl match what a non-ovl
> setup would do in the same situation (_either_ of the overlaid
> filesystems - there might be multiple cases).
>
> But I'm missing the original report. It sounds like there was a
> regression and we already have a case of "changing the error number
> broke something". If so, that regression should be fixed.
>
> In general, I'm perfectly happy with people fixing error numbers and
> changing them.
>
> The only thing I require is that if those cleanups and fixes are
> reported to break something, people quickly revert (and preferably add
> a big comment about "Use *this* error number, because while this
> *other* error number would make sense, application XyZ expects AbC"..)
>
> Linus
Thanks for clarifying, Linus!
The regression in question caused overlayfs to erroneously return ENOSYS when one lower filesystem (e.g., davfs2) returned this upon checking extended attributes (there were two relevant submissions triggering this somewhere around 5.15, 5.16)
My original patch: https://lore.kernel.org/lkml/[email protected]/T/
This was supposed to augment the following commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5b0a414d06c3ed2097e32ef7944a4abb644b89bd
There, checking for the exact error code indeed matters, because it is supposed to swallow all conditions marking "no fileattr support" but doesn't catch fuse's ENOSYS.
You see similar code checking for ENOSYS appearing in other places, like util-linux, for example
here https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/commit/?id=f6385a6adeea6be255d68016977c5dd5eaab05da
and there's of course EOPNOTSUPP as well, as in here https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/commit/?id=7b8fda2e8e7b1752cba1fab01d7f569b5d87e661
Best,
Christian
On Mon, Jul 18, 2022 at 12:04 PM Christian Kohlschütter
<[email protected]> wrote:
>
> The regression in question caused overlayfs to erroneously return ENOSYS when one lower filesystem (e.g., davfs2) returned this upon checking extended attributes (there were two relevant submissions triggering this somewhere around 5.15, 5.16)
Well, if that's the case, isn't the proper fix to just fix davfs2?
If ENOSYS isn't a valid error, and has broken apps that want to just
ignore "no fattr support", then it's a davfs2 bug, and fixing it there
will presumably magically just fix the ovl case too?
Yes, yes, you point to that commit to util-linux to also accept
ENOSYS, but that's from 2021.
So it's presumably triggered by the same issue - a rare (or new) and
broken filesystem returned the wrong error code.
Let's just fix that.
I do not object to *also* doing the ovlfs "accept ENOSYS too", since
it seems harmless and understandable, but at the same time this all
does make me go "the actual *fundamental* cause of this was davfs2
being confused, it should be fixed there too.
And yes, yes, I realize that davfs2 is out-of-tree fuse filesystem,
and is not in the kernel. But have people made the bug-report to the
maintainers there?
I don't think we should *only* have a kernel-side fix for a broken
FUSE filesystem. Particularly not one to some random bystander like
ovlfs.
In fact if we do a kernel patch for this dodgy filesystem, it would
seem to me to make more sense to have FUSE notice that "ok, ENOSYS is
broken for this situation, let's translate it to the right ENOTTY",
and that would have fixed both the ovlfs case and the util-linux one.
Hmm?
Linus
On Mon, 18 Jul 2022 at 21:17, Linus Torvalds
<[email protected]> wrote:
>
> On Mon, Jul 18, 2022 at 12:04 PM Christian Kohlschütter
> <[email protected]> wrote:
> >
> > The regression in question caused overlayfs to erroneously return ENOSYS when one lower filesystem (e.g., davfs2) returned this upon checking extended attributes (there were two relevant submissions triggering this somewhere around 5.15, 5.16)
>
> Well, if that's the case, isn't the proper fix to just fix davfs2?
>
> If ENOSYS isn't a valid error, and has broken apps that want to just
> ignore "no fattr support", then it's a davfs2 bug, and fixing it there
> will presumably magically just fix the ovl case too?
Libfuse returns ENOSYS for requests which the filesystem doesn't
support. Hence this is not a davfs bug, davfs knows nothing about
itoctls, the userspace filesystem is just returning the error value
that is used to mean "no support for this type of request".
So this is a bug in the kernel part of fuse, that doesn't catch and
convert ENOSYS in case of the ioctl request.
Fixing it in fuse should fix it in overlayfs as well, as you say.
Thanks,
Miklos
On Mon, Jul 18, 2022 at 12:28 PM Miklos Szeredi <[email protected]> wrote:
>
> So this is a bug in the kernel part of fuse, that doesn't catch and
> convert ENOSYS in case of the ioctl request.
Ahh, even better. No need to worry about external issues.
Linus