linux-kernel - Re: [PATCH] [REGRESSION] ovl: Handle ENOSYS when fileattr support is missing in lower/upper fs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <EE5E5841-3561-4530-8813-95C16A36D94A@kohlschutter.com>
Date:   Mon, 18 Jul 2022 21:04:00 +0200
From:   Christian Kohlschütter 
        <christian@...lschutter.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Miklos Szeredi <miklos@...redi.hu>,
        overlayfs <linux-unionfs@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] [REGRESSION] ovl: Handle ENOSYS when fileattr support is
 missing in lower/upper fs

Am 18.07.2022 um 20:29 schrieb Linus Torvalds <torvalds@...ux-foundation.org>:
> 
> On Mon, Jul 18, 2022 at 6:13 AM Miklos Szeredi <miklos@...redi.hu> 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/4B9D76D5-C794-4A49-A76F-3D4C10385EE0@kohlschutter.com/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