On Fri, Apr 22, 2022 at 03:43:08PM +0200, Sven Schnelle wrote:
> gcc-12 shows a lot of array bound warnings on s390. This is caused
> by our S390_lowcore macro:
>
> which uses an hardcoded address of 0. Wrapping that with
> absolute_pointer() works, but gcc no longer knows that a 12 bit
> instruction is sufficient to access lowcore. So it emits instructions
> like 'lghi %r1,0; l %rx,xxx(%r1)' instead of a single load/store
> instruction. As s390 stores variables often read/written in lowcore,
> this is considered problematic. Therefore disable -Warray-bounds on
> s390 for now until there is a better real solution.
>
> Signed-off-by: Sven Schnelle <[email protected]>
It looks like the source of this problem (the literal-values-treated-as-NULL)
is gcc-12 specific. From the discussions, it sounded like Jacob was
going to fix this "correctly" in gcc-13. It might be a good idea to make
this version-checked? (i.e. only disable on gcc-12)
Either way:
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
On Fri, Apr 22, 2022 at 10:54:09AM -0700, Kees Cook wrote:
> On Fri, Apr 22, 2022 at 03:43:08PM +0200, Sven Schnelle wrote:
> > gcc-12 shows a lot of array bound warnings on s390. This is caused
> > by our S390_lowcore macro:
> >
> > which uses an hardcoded address of 0. Wrapping that with
> > absolute_pointer() works, but gcc no longer knows that a 12 bit
> > instruction is sufficient to access lowcore. So it emits instructions
> > like 'lghi %r1,0; l %rx,xxx(%r1)' instead of a single load/store
> > instruction. As s390 stores variables often read/written in lowcore,
> > this is considered problematic. Therefore disable -Warray-bounds on
> > s390 for now until there is a better real solution.
> >
> > Signed-off-by: Sven Schnelle <[email protected]>
>
> It looks like the source of this problem (the literal-values-treated-as-NULL)
> is gcc-12 specific. From the discussions, it sounded like Jacob was
> going to fix this "correctly" in gcc-13. It might be a good idea to make
> this version-checked? (i.e. only disable on gcc-12)
That makes sense, so we still get at least some coverage for compilers
< gcc 12; and also latest clang still seems to do the right thing.
Sven, could you either send an updated patch, or an addon patch,
please? Whatever you prefer.
Coming back to this, because my rc2 week tends to be quiet as people
take a breather, and as such a good time for me to do system upgrades.
And gcc-12 dropped in Fedora 36, and shows problems on x86 too.
So I suspect we'll have to disable -Warray-bounds globally on gcc-12,
not just on s390.
Unless Kees has patches ready to go already.
Some of the warnings look potentially simple, ie
struct mbus_dram_target_info;
in <linux/mbus.h> has the comment
* [..] Peripherals are
* required to support at least 4 decode windows.
and then as a result has
int num_cs;
struct mbus_dram_window {
[..]
} cs[4];
and that "cs[4]" looks just bogus - it can be a much larger array, the
'4' is just a minimum. The real limit is that 'num_cs' one.
But there's a *lot* of warnings, and many of them are due to this, and
while some are obvious, others aren't.
There are other things too in gcc-12 that seem half-baked. I was
interested to see the new '-Wdangling-pointer' thing, but then when I
looked at it, the two cases I looked at were just bogus, so ..
Linus
On Fri, Apr 22, 2022 at 10:54 AM Kees Cook <[email protected]> wrote:
>
> On Fri, Apr 22, 2022 at 03:43:08PM +0200, Sven Schnelle wrote:
> > gcc-12 shows a lot of array bound warnings on s390. This is caused
> > by our S390_lowcore macro:
> >
> > which uses an hardcoded address of 0. Wrapping that with
> > absolute_pointer() works, but gcc no longer knows that a 12 bit
> > instruction is sufficient to access lowcore. So it emits instructions
> > like 'lghi %r1,0; l %rx,xxx(%r1)' instead of a single load/store
> > instruction. As s390 stores variables often read/written in lowcore,
> > this is considered problematic. Therefore disable -Warray-bounds on
> > s390 for now until there is a better real solution.
> >
> > Signed-off-by: Sven Schnelle <[email protected]>
>
> It looks like the source of this problem (the literal-values-treated-as-NULL)
> is gcc-12 specific. From the discussions, it sounded like Jacob was
> going to fix this "correctly" in gcc-13. It might be a good idea to make
> this version-checked? (i.e. only disable on gcc-12)
On Wed, Jun 08, 2022 at 01:07:05PM -0700, Linus Torvalds wrote:
> Coming back to this, because my rc2 week tends to be quiet as people
> take a breather, and as such a good time for me to do system upgrades.
>
> And gcc-12 dropped in Fedora 36, and shows problems on x86 too.
>
> So I suspect we'll have to disable -Warray-bounds globally on gcc-12,
> not just on s390.
>
> Unless Kees has patches ready to go already.
I and others have been working through a bunch of them, though yes,
they're not all fixed yet. I've been trying to track it here[1], but
many of those fixes are only in -next.
> Some of the warnings look potentially simple, ie
>
> struct mbus_dram_target_info;
>
> in <linux/mbus.h> has the comment
>
> * [..] Peripherals are
> * required to support at least 4 decode windows.
>
> and then as a result has
>
> int num_cs;
> struct mbus_dram_window {
> [..]
> } cs[4];
>
> and that "cs[4]" looks just bogus - it can be a much larger array, the
> '4' is just a minimum. The real limit is that 'num_cs' one.
>
> But there's a *lot* of warnings, and many of them are due to this, and
> while some are obvious, others aren't.
When I did a count in -next 2 weeks ago, there were 182 warnings (x86
allmodconfig) from GCC 12 where 153 were from -Warray-bounds. Today
we're now down to 80 total (61 from -Warray-bounds), so we've solved
over half of them.
> There are other things too in gcc-12 that seem half-baked. I was
> interested to see the new '-Wdangling-pointer' thing, but then when I
> looked at it, the two cases I looked at were just bogus, so ..
Yes, GCC 12 is very odd in places. Besides the literal-as-pointer issue
that still causes problems for s390[2], there seem to be at least a
few other bugs associated with the internal diagnostics infrastructure
that informs -Warray-bounds, -Wstringop-overflow, etc. I narrowed down
one recently with UBSAN_BOUNDS[3] (which therefore impacts all*config
builds), but there is no GCC fix yet. :(
So, it's unclear to me if we want to try to get back to 0 warnings
(where we were with v5.18 and GCC 11) in the next couple weeks, or if we
need to just disable it for GCC 12 until everything is fixed again.
-Kees
[1] https://github.com/KSPP/linux/issues/190
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
[3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105679
--
Kees Cook
On Wed, Jun 8, 2022 at 2:33 PM Kees Cook <[email protected]> wrote:
>
> I and others have been working through a bunch of them, though yes,
> they're not all fixed yet. I've been trying to track it here[1], but
> many of those fixes are only in -next.
Hmm. Even with that disabled, I get a few warnings I *really* would
want to get rid of.
The one in ipuv3-crtc.c seems valid about "address used as boolean is
always true".
The 'dangling-pointer' warning does seem interesting, but not when the
compiler does as bad a job as gcc seems to do.
See the attached patch for
(a) make the s390 "use -Wno-array-bounds for gcc-12" be generic
(b) fix the ipuv3-crtc.c one. IMX people?
(c) disable -Wdangling-pointer entirely for now
but that still leaves the netfs_i_context games, which gcc-12 is very
unhappy about:
In function ‘fortify_memset_chk’,
inlined from ‘netfs_i_context_init’ at ./include/linux/netfs.h:327:2,
inlined from ‘afs_set_netfs_context’ at fs/afs/inode.c:61:2,
inlined from ‘afs_inode_init_from_status’ at fs/afs/inode.c:139:2:
./include/linux/fortify-string.h:258:25: error: call to
‘__write_overflow_field’ declared with attribute warning: detected
write beyond size of field (1st parameter); maybe use struct_group()?
[-Werror=attribute-warning]
and I do kind of agree with the compiler in that case. That code
should have some kind of
struct container {
struct inode inode;
struct netfs_i_context ctx;
};
thing, and aim to do that instead of the pointer arithmetic games.
Ceph seems to trigger the exact same thing.
There's also an annoying mlx5 issue, with gcc apparently not tracking
the usage of
struct lag_tracker tracker;
well enough (it's never used if do_bond is false, but probably some
inlining change means that gcc doesn't see that).
DavidH - mind looking at the netfs_i_context_init() thing?
I'd like to use something more surgical than
CONFIG_CC_NO_ARRAY_BOUNDS, but considering the s390 issues, it may not
even be worth it. Kees, just how far away are we from that being ok on
x86-64?
I did consider making CONFIG_CC_NO_ARRAY_BOUNDS be more akin to
config CC_NO_ARRAY_BOUNDS
bool
depends on CC_IS_GCC
depends on GCC_VERSION >= 120000 && GCC_VERSION < 130000
default GCC12_NO_ARRAY_BOUNDS
and then s390 and any subsystem that triggers the -Warray-bounds problem can do
select GCC12_NO_ARRAY_BOUNDS
to show that they have issues with the new gcc12 rules.
That would be at least a bit more surgical..
Linus
On June 8, 2022 4:59:29 PM PDT, Linus Torvalds <[email protected]> wrote:
>On Wed, Jun 8, 2022 at 2:33 PM Kees Cook <[email protected]> wrote:
>>
>> I and others have been working through a bunch of them, though yes,
>> they're not all fixed yet. I've been trying to track it here[1], but
>> many of those fixes are only in -next.
>
>Hmm. Even with that disabled, I get a few warnings I *really* would
>want to get rid of.
Yup! :)
>
>The one in ipuv3-crtc.c seems valid about "address used as boolean is
>always true".
>
>The 'dangling-pointer' warning does seem interesting, but not when the
>compiler does as bad a job as gcc seems to do.
>
>See the attached patch for
>
> (a) make the s390 "use -Wno-array-bounds for gcc-12" be generic
>
> (b) fix the ipuv3-crtc.c one. IMX people?
>
> (c) disable -Wdangling-pointer entirely for now
I'll take a look; thanks! Should I send them back as a pull request?
>but that still leaves the netfs_i_context games, which gcc-12 is very
>unhappy about:
Yeah. Happily, this has already been solved, but it looks like David didn't do a pull yet for it?
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-next
>I'd like to use something more surgical than
>CONFIG_CC_NO_ARRAY_BOUNDS, but considering the s390 issues, it may not
>even be worth it. Kees, just how far away are we from that being ok on
>x86-64?
For gcc's UBSAN_SHIFT (I typoed this in my first reply) bug, netdev has been moving it to W=1 builds on a per-source basis for the moment:
https://git.kernel.org/linus/e95032988053c17baf6c7e27024f5103a19a5f4a
Some discussion:
https://lore.kernel.org/lkml/202205231229.CF6B8471@keescook/
Perhaps these could be even more carefully limited to GCC 12 only, using the Kconfig you suggested?
-Kees
--
Kees Cook
On Wed, Jun 8, 2022 at 5:39 PM Kees Cook <[email protected]> wrote:
>
> I'll take a look; thanks! Should I send them back as a pull request?
That would be good.
> Yeah. Happily, this has already been solved, but it looks like David didn't do a pull yet for it?
>
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-next
Good.
> For gcc's UBSAN_SHIFT (I typoed this in my first reply) bug, netdev has been moving it to W=1 builds on a per-source basis for the moment:
>
> https://git.kernel.org/linus/e95032988053c17baf6c7e27024f5103a19a5f4a
Ugh. That's sad. Since now the gcc-12 misfeature ends up biting
everybody else too.
> Perhaps these could be even more carefully limited to GCC 12 only, using the Kconfig you suggested?
Yeah, I'd rather just say "gcc-12 gets this thing entirely wrong,
let's disable it there" than disable it for compilers that get it
right.
In fact, I'd rather have that global "gcc-12 is broken, disable it",
than marking "this file shouldn't get checked" kind of logic.
It's wrong blaming the C code, when the compiler is doing bad sh*t.
Linus
Hi Kees,
On Mi, 2022-06-08 at 17:39 -0700, Kees Cook wrote:
[...]
> > See the attached patch for
> >
> > (a) make the s390 "use -Wno-array-bounds for gcc-12" be generic
> >
> > (b) fix the ipuv3-crtc.c one. IMX people?
> >
> > (c) disable -Wdangling-pointer entirely for now
>
> I'll take a look; thanks! Should I send them back as a pull request?
Does this refer to the whole patch, including (a) and (b), or am I to
pick up the ipuv3-crtc.c fix?
regards
Philipp
Hi Linus,
On Mi, 2022-06-08 at 16:59 -0700, Linus Torvalds wrote:
> On Wed, Jun 8, 2022 at 2:33 PM Kees Cook <[email protected]> wrote:
> >
> > I and others have been working through a bunch of them, though yes,
> > they're not all fixed yet. I've been trying to track it here[1], but
> > many of those fixes are only in -next.
>
> Hmm. Even with that disabled, I get a few warnings I *really* would
> want to get rid of.
>
> The one in ipuv3-crtc.c seems valid about "address used as boolean is
> always true".
>
> The 'dangling-pointer' warning does seem interesting, but not when the
> compiler does as bad a job as gcc seems to do.
>
> See the attached patch for
>
> (a) make the s390 "use -Wno-array-bounds for gcc-12" be generic
>
> (b) fix the ipuv3-crtc.c one. IMX people?
Thank you, this fix clearly matches the original intention.
Acked-by: Philipp Zabel <[email protected]>
The mistake had no adverse effect since the following condition doesn't
actually dereference the NULL pointer, but given the compiler warning
this
Fixes: eb8c88808c83 ("drm/imx: add deferred plane disabling")
regards
Philipp
On June 9, 2022 2:56:47 AM PDT, Philipp Zabel <[email protected]> wrote:
>Hi Kees,
>
>On Mi, 2022-06-08 at 17:39 -0700, Kees Cook wrote:
>[...]
>> > See the attached patch for
>> >
>> > (a) make the s390 "use -Wno-array-bounds for gcc-12" be generic
>> >
>> > (b) fix the ipuv3-crtc.c one. IMX people?
>> >
>> > (c) disable -Wdangling-pointer entirely for now
>>
>> I'll take a look; thanks! Should I send them back as a pull request?
>
>Does this refer to the whole patch, including (a) and (b), or am I to
>pick up the ipuv3-crtc.c fix?
Go ahead and grab that one please; that's more "normal" :)
Thanks!
--
Kees Cook
Linus Torvalds <[email protected]> wrote:
> > Yeah. Happily, this has already been solved, but it looks like David didn't do a pull yet for it?
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-next
>
> Good.
Do you want it tagging and a pull req generating, even though it's a single
patch?
Note that Dave Chinner would rather I converted code like:
struct myfs_inode *myfsinode = xyz;
myfsinode->netfs.inode.i_ino = 123;
to something like:
struct myfs_inode *myfsinode = xyz;
struct inode *inode = VFS_I(myfsinode);
inode->i_ino = 123;
where the translation is wrapped inside a VFS_I() macro in every filesystem
and wants this across all filesystems. I think the former looks cleaner, but
he has a point about how to deal with yet another layer of wrapping being
inserted in the future. Do you have a preference?
David
On Wed, Jun 08, 2022 at 04:59:29PM -0700, Linus Torvalds wrote:
Just one small drive by comment in case this ends up going in in one
form or another.
> Makefile | 4 ++++
> arch/s390/Makefile | 10 +---------
> drivers/gpu/drm/imx/ipuv3-crtc.c | 2 +-
> init/Kconfig | 5 +++++
> 4 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index c43d825a3c4c..b2e93c1a8021 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -788,6 +788,7 @@ stackp-flags-$(CONFIG_STACKPROTECTOR_STRONG) := -fstack-protector-strong
> KBUILD_CFLAGS += $(stackp-flags-y)
>
> KBUILD_CFLAGS-$(CONFIG_WERROR) += -Werror
> +KBUILD_CFLAGS-$(CONFIG_CC_NO_ARRAY_BOUNDS) += -Wno-array-bounds
> KBUILD_CFLAGS += $(KBUILD_CFLAGS-y) $(CONFIG_CC_IMPLICIT_FALLTHROUGH)
>
> ifdef CONFIG_CC_IS_CLANG
> @@ -805,6 +806,9 @@ endif
> KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
> KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
>
> +# These result in bogus false positives
> +KBUILD_CFLAGS += $(call cc-disable-warning, dangling-pointer)
> +
> ifdef CONFIG_FRAME_POINTER
> KBUILD_CFLAGS += -fno-omit-frame-pointer -fno-optimize-sibling-calls
> else
> diff --git a/arch/s390/Makefile b/arch/s390/Makefile
> index d73611b35164..e1abb0d03824 100644
> --- a/arch/s390/Makefile
> +++ b/arch/s390/Makefile
> @@ -32,15 +32,7 @@ KBUILD_CFLAGS_DECOMPRESSOR += -fno-stack-protector
> KBUILD_CFLAGS_DECOMPRESSOR += $(call cc-disable-warning, address-of-packed-member)
> KBUILD_CFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO),-g)
> KBUILD_CFLAGS_DECOMPRESSOR += $(if $(CONFIG_DEBUG_INFO_DWARF4), $(call cc-option, -gdwarf-4,))
> -
> -ifdef CONFIG_CC_IS_GCC
> - ifeq ($(call cc-ifversion, -ge, 1200, y), y)
> - ifeq ($(call cc-ifversion, -lt, 1300, y), y)
> - KBUILD_CFLAGS += $(call cc-disable-warning, array-bounds)
> - KBUILD_CFLAGS_DECOMPRESSOR += $(call cc-disable-warning, array-bounds)
> - endif
> - endif
> -endif
> +KBUILD_CFLAGS_DECOMPRESSOR += $(if $(CC_NO_ARRAY_BOUNDS),-Wno-array-bounds)
I think this should be $(CONFIG_CC_NO_ARRAY_BOUNDS)?
>
> UTS_MACHINE := s390x
> STACK_SIZE := $(if $(CONFIG_KASAN),65536,16384)
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index 9c8829f945b2..f7863d6dea80 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -69,7 +69,7 @@ static void ipu_crtc_disable_planes(struct ipu_crtc *ipu_crtc,
> drm_atomic_crtc_state_for_each_plane(plane, old_crtc_state) {
> if (plane == &ipu_crtc->plane[0]->base)
> disable_full = true;
> - if (&ipu_crtc->plane[1] && plane == &ipu_crtc->plane[1]->base)
> + if (ipu_crtc->plane[1] && plane == &ipu_crtc->plane[1]->base)
> disable_partial = true;
> }
>
> diff --git a/init/Kconfig b/init/Kconfig
> index c984afc489de..ccb1302d6edd 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -885,6 +885,11 @@ config CC_IMPLICIT_FALLTHROUGH
> default "-Wimplicit-fallthrough=5" if CC_IS_GCC && $(cc-option,-Wimplicit-fallthrough=5)
> default "-Wimplicit-fallthrough" if CC_IS_CLANG && $(cc-option,-Wunreachable-code-fallthrough)
>
> +config CC_NO_ARRAY_BOUNDS
> + bool
> + depends on CC_IS_GCC
> + default y if GCC_VERSION >= 120000 && GCC_VERSION < 130000
> +
> #
> # For architectures that know their GCC __int128 support is sound
> #
Cheers,
Nathan
On Thu, Jun 9, 2022 at 7:14 AM David Howells <[email protected]> wrote:
>
> Note that Dave Chinner would rather I converted code like:
>
> struct myfs_inode *myfsinode = xyz;
> myfsinode->netfs.inode.i_ino = 123;
>
> to something like:
>
> struct myfs_inode *myfsinode = xyz;
> struct inode *inode = VFS_I(myfsinode);
> inode->i_ino = 123;
>
> where the translation is wrapped inside a VFS_I() macro in every filesystem
> and wants this across all filesystems.
What? No. That's absolutely disgusting.
Maybe I'm mis-undestanding.
The usual way filesystems should handle this is that they have their
own inode information that contains a 'struct inode', and then they
have an inline function to go from that generic VFS inode to their one
using "container_of()".
And yeah, maybe they call that container_of() thing MYINODE() or
something, although I think an inline function without the ugly
all-uppercase is right.
But the way they go the other way is literally to just dereference the
inode that they have, ie they just use a
if (S_ISREG(inode->vfs_inode.i_mode)) ..
kind pattern. There's no reason or excuse to try to "wrap" that, and
it would be a big step backwards to introduce some kind of VFS_I()
macro.
There's also no reason to make that generic. At no point should you
ever go from "random filesystem inode" to "actual generic VFS inode"
in some uncontrolled manner.
But maybe Dave is talking about something else, and I'm missing the point.
Linus
On Thu, Jun 9, 2022 at 7:55 AM Nathan Chancellor <[email protected]> wrote:
>
> I think this should be $(CONFIG_CC_NO_ARRAY_BOUNDS)?
Thanks, fixed.
Anyway, in order to deal with the (few - the rc2 week does tend to be
small) pull requests I have pending, I have basically worked around
all the new warnings I see.
Some of the workarounds are the proper fixes, but mostly it's a pretty
harsh "just shut that warning up". That includes for things that have
proper fixes pending (ie the netfs issue), where I just did a pretty
ugly but very localized
#pragma GCC diagnostic ignored "-Wattribute-warning"
in the affected files.
End result: I have a clean 'allmodconfig' build again, and hopefully
most of these workarounds can either be tightened up or removed
entirely at some point.
It's this in my tree now:
507160f46c55 ("netfs: gcc-12: temporarily disable
'-Wattribute-warning' for now")
f0be87c42cbd ("gcc-12: disable '-Warray-bounds' universally for now")
842c3b3ddc5f ("mellanox: mlx5: avoid uninitialized variable
warning with gcc-12")
49beadbd47c2 ("gcc-12: disable '-Wdangling-pointer' warning for now")
7aefd8b53815 ("drm: imx: fix compiler warning with gcc-12")
6bfb56e93bce ("cert host tools: Stop complaining about deprecated
OpenSSL functions")
in case people care.
Linus
On Thu, Jun 09, 2022 at 11:20:02AM -0700, Linus Torvalds wrote:
> On Thu, Jun 9, 2022 at 7:14 AM David Howells <[email protected]> wrote:
> >
> > Note that Dave Chinner would rather I converted code like:
> >
> > struct myfs_inode *myfsinode = xyz;
> > myfsinode->netfs.inode.i_ino = 123;
> >
> > to something like:
> >
> > struct myfs_inode *myfsinode = xyz;
> > struct inode *inode = VFS_I(myfsinode);
> > inode->i_ino = 123;
> >
> > where the translation is wrapped inside a VFS_I() macro in every filesystem
> > and wants this across all filesystems.
>
> What? No. That's absolutely disgusting.
>
> Maybe I'm mis-undestanding.
Perhaps, because I think what I said looks very different when taken
out of context.
I saw a heap of different implementations of the same thing with no
consistency across them (i.e. inode container definitions) and a
mess of a patch to convert them without solving the problem that
there's no consistent convention for doing filesystem inode -> VFS
inode container conversion
> The usual way filesystems should handle this is that they have their
> own inode information that contains a 'struct inode', and then they
> have an inline function to go from that generic VFS inode to their one
> using "container_of()".
>
> And yeah, maybe they call that container_of() thing MYINODE() or
> something, although I think an inline function without the ugly
> all-uppercase is right.
Right, BTRFS_I(), EXT4_I(), F2FS_I(), AFS_FS_I(), P9FS_I(), etc.
It's a convention, it dates back to macro days (hence upper case
even though most are static inlines these days), and it obvious no
matter what filesystem code I read that when I see this XXX_I(inode)
convention I know the code is accessing the filesystem inode in the
container, not the VFS indoe.
> But the way they go the other way is literally to just dereference the
> inode that they have, ie they just use a
>
> if (S_ISREG(inode->vfs_inode.i_mode)) ..
The problem with this is that we have very similar names in both the
VFS inode and the filesysetm inodes (e.g. i_flags), and without a
clear demarcation of which inode is being referenced it can lead to
confusion and bugs.
> kind pattern. There's no reason or excuse to try to "wrap" that, and
> it would be a big step backwards to introduce some kind of VFS_I()
> macro.
If the result of adding a helper convention is that every reverse
inode container resolution looks identical across all filesystems,
then we no longer have to know the details of the fs specific
container to get the conversion right. All the code across all the
filesystems would look the same, even though the wrapper would be
different.
We do helper conversions like this all the time to make the code
easier to read, understand and maintain, so I really don't see why
this would be considered a step backwards....
> There's also no reason to make that generic. At no point should you
> ever go from "random filesystem inode" to "actual generic VFS inode"
> in some uncontrolled manner.
We never do any conversions in an uncontrolled manner. We often need
to go from fs inode to vfs inode because we are deep in filesystem
implementation code passing around filesystem inodes, but the piece
of information we need to access is stored in the VFS inode (e.g.
uid, gid, etc). That's what this netfs inode container was requiring
in the patchset...
> But maybe Dave is talking about something else, and I'm missing the point.
Perhaps - my comment was not about the VFS_I() name or implementation;
I used it simply because I can point at code that uses it as an
example of having a symmetric, easily recognisable convention.
My point was that the fs inode to vfs inode conversion is a common
operation performed across all filesystems that lacks any
consistency in implementation. Some filesystems use a symmetric API
for these container conversions and so I was simply suggesting that
converting them all to use a common symmetric convention would
simplify the maintenance of filesystem code in future and make it
easier for other people to understand...
Cheers,
Dave.
--
Dave Chinner
[email protected]
On Thu, Jun 9, 2022 at 4:59 PM Dave Chinner <[email protected]> wrote:
>
> I saw a heap of different implementations of the same thing with no
> consistency across them (i.e. inode container definitions) and a
> mess of a patch to convert them without solving the problem that
> there's no consistent convention for doing filesystem inode -> VFS
> inode container conversion
Sure. But the thing is, they aren't actually all the same.
We do have a pattern - embed the generic vfs inode inside the
filesystem-specific one - but the exact details of how you do it isn't
fixed in stone.
And this netfs thing is actually an example of why it *shouldn't* be
fixed in stone, exactly because a netfs user doesn't want to just
"embed the vfs inode" - it wants to embed something *else* that in
turn embeds the vfs inode.
So yes, most filesystems do similar things, but they aren't exactly
the same. And they *could* be more different than they actually are
(there's nothing that says you *have* to embed the generic VFS inode
in the filesystem-specific one, it's just that we make it easy and
it's a pattern that has been copied because it works really well)
And yes, we could just enforce naming, and force everybody do use
#define VFS_I(myino) (&(myino)->vfs_inode)
but then we really would have been in trouble with this whole netfs
embedded struct.
And no, it wouldn't be some kind of insurmountable issue, using an
unnamed union (so that "vfs_inode" would be the inode, and
"netfs_inode" would be the bigger netfs inode+context) would have made
it all work out.
But I really don't see the point of trying to just force everybody to
use the same name, and force people to use a common macro that doesn't
really *buy* you anything.
I think just writing 'inode->vfs_inode.i_mode' is very clear, and is
particularly obvious in that there's no costly translation.
'VFS_I(inode)->i_mode' might be shorter to write, but that's mainly
because of the ugly shortened macro name. If you want ugly short
names, you could have called the inode member just 'vfs_i' in the
first place.
And yes, we could go even further, and just make the rule be that
everybody should actually put the generic VFS inode struct at the
beginning of the filesystem inode. I think people do that already in
practice.
Then we could maybe use some language tricks to make the filesystems
get their own inode pointer directly as arguments, instead of getting
a 'struct inode *" and having to do that
struct ext4_inode_info *ei = EXT4_I(inode);
at all.
I suspect we'd have to use a macro with a cast at the op assignment
time, which would be really ugly, though, but maybe there's some gcc
language extension that allows that kind of thing.
Anyway, my point is that yes, we could enforce tighter rules here, and
make everybody match some particular pattern.
But I don't think we'd actually benefit from it, and I think it would
have just caused more pain in this situation, for example.
Linus