| Message ID | 20230816140623.452869-2-przemyslaw.kitszel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | introduce DEFINE_FLEX() macro | expand |
Hi Przemek, kernel test robot noticed the following build errors: [auto build test ERROR on 479b322ee6feaff612285a0e7f22c022e8cd84eb] url: https://github.com/intel-lab-lkp/linux/commits/Przemek-Kitszel/overflow-add-DEFINE_FLEX-for-on-stack-allocs/20230816-221402 base: 479b322ee6feaff612285a0e7f22c022e8cd84eb patch link: https://lore.kernel.org/r/20230816140623.452869-2-przemyslaw.kitszel%40intel.com patch subject: [PATCH net-next v3 1/7] overflow: add DEFINE_FLEX() for on-stack allocs config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20230817/202308170000.YqabIR9D-lkp@intel.com/config) compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) reproduce: (https://download.01.org/0day-ci/archive/20230817/202308170000.YqabIR9D-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308170000.YqabIR9D-lkp@intel.com/ All errors (new ones prefixed by >>): >> thread 'main' panicked at '"ftrace_branch_data_union_(anonymous_at_include/linux/compiler_types_h_144_2)" is not a valid Ident', /opt/cross/rustc-1.68.2-bindgen-0.56.0/cargo/registry/src/github.com-1ecc6299db9ec823/proc-macro2-1.0.24/src/fallback.rs:693:9 stack backtrace: 0: rust_begin_unwind at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/std/src/panicking.rs:575:5 1: core::panicking::panic_fmt at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/core/src/panicking.rs:64:14 2: proc_macro2::fallback::Ident::_new 3: proc_macro2::Ident::new 4: bindgen::ir::context::BindgenContext::rust_ident 5: <bindgen::ir::comp::CompInfo as bindgen::codegen::CodeGenerator>::codegen 6: <bindgen::ir::ty::Type as bindgen::codegen::CodeGenerator>::codegen 7: <bindgen::ir::item::Item as bindgen::codegen::CodeGenerator>::codegen 8: <bindgen::ir::comp::CompInfo as bindgen::codegen::CodeGenerator>::codegen 9: <bindgen::ir::ty::Type as bindgen::codegen::CodeGenerator>::codegen 10: <bindgen::ir::item::Item as bindgen::codegen::CodeGenerator>::codegen 11: <bindgen::ir::module::Module as bindgen::codegen::CodeGenerator>::codegen 12: <bindgen::ir::item::Item as bindgen::codegen::CodeGenerator>::codegen 13: bindgen::ir::context::BindgenContext::gen 14: bindgen::Builder::generate 15: bindgen::main note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. make[3]: *** [rust/Makefile:316: rust/uapi/uapi_generated.rs] Error 1 make[3]: *** Deleting file 'rust/uapi/uapi_generated.rs' >> thread 'main' panicked at '"ftrace_branch_data_union_(anonymous_at_include/linux/compiler_types_h_144_2)" is not a valid Ident', /opt/cross/rustc-1.68.2-bindgen-0.56.0/cargo/registry/src/github.com-1ecc6299db9ec823/proc-macro2-1.0.24/src/fallback.rs:693:9 stack backtrace: 0: rust_begin_unwind at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/std/src/panicking.rs:575:5 1: core::panicking::panic_fmt at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/core/src/panicking.rs:64:14 2: proc_macro2::fallback::Ident::_new 3: proc_macro2::Ident::new 4: bindgen::ir::context::BindgenContext::rust_ident 5: <bindgen::ir::comp::CompInfo as bindgen::codegen::CodeGenerator>::codegen 6: <bindgen::ir::ty::Type as bindgen::codegen::CodeGenerator>::codegen 7: <bindgen::ir::item::Item as bindgen::codegen::CodeGenerator>::codegen 8: <bindgen::ir::comp::CompInfo as bindgen::codegen::CodeGenerator>::codegen 9: <bindgen::ir::ty::Type as bindgen::codegen::CodeGenerator>::codegen 10: <bindgen::ir::item::Item as bindgen::codegen::CodeGenerator>::codegen 11: <bindgen::ir::module::Module as bindgen::codegen::CodeGenerator>::codegen 12: <bindgen::ir::item::Item as bindgen::codegen::CodeGenerator>::codegen 13: bindgen::ir::context::BindgenContext::gen 14: bindgen::Builder::generate 15: bindgen::main note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. make[3]: *** [rust/Makefile:310: rust/bindings/bindings_generated.rs] Error 1 make[3]: *** Deleting file 'rust/bindings/bindings_generated.rs' make[3]: Target 'rust/' not remade because of errors. make[2]: *** [Makefile:1293: prepare] Error 2 make[1]: *** [Makefile:234: __sub-make] Error 2 make[1]: Target 'prepare' not remade because of errors. make: *** [Makefile:234: __sub-make] Error 2 make: Target 'prepare' not remade because of errors.
On Wed, Aug 16, 2023 at 10:06:17AM -0400, Przemek Kitszel wrote: > Add DEFINE_FLEX() macro for on-stack allocations of structs with > flexible array member. > > Expose __struct_size() macro outside of fortify-string.h, as it could be > used to read size of structs allocated by DEFINE_FLEX(). > Move __member_size() alongside it. > -Kees > > Using underlying array for on-stack storage lets us to declare > known-at-compile-time structures without kzalloc(). > > Actual usage for ice driver is in following patches of the series. > > Missing __has_builtin() workaround is moved up to serve also assembly > compilation with m68k-linux-gcc, see [1]. > Error was (note the .S file extension): > In file included from ../include/linux/linkage.h:5, > from ../arch/m68k/fpsp040/skeleton.S:40: > ../include/linux/compiler_types.h:331:5: warning: "__has_builtin" is not defined, evaluates to 0 [-Wundef] > 331 | #if __has_builtin(__builtin_dynamic_object_size) > | ^~~~~~~~~~~~~ > ../include/linux/compiler_types.h:331:18: error: missing binary operator before token "(" > 331 | #if __has_builtin(__builtin_dynamic_object_size) > | ^ Looks good to me! Thanks for working on this. :) Acked-by: Kees Cook <keescook@chromium.org>
From: Przemek Kitszel > Sent: Wednesday, August 16, 2023 3:06 PM > > Using underlying array for on-stack storage lets us to declare > known-at-compile-time structures without kzalloc(). Isn't DEFINE_FLEX() a bit misleading? One thing it isn't is 'flexible' since it has a fixed size. > +#define DEFINE_FLEX(type, name, member, count) \ > + union { \ > + u8 bytes[struct_size_t(type, member, count)]; \ > + type obj; \ > + } name##_u __aligned(_Alignof(type)) = {}; \ You shouldn't need the _Alignof() it is the default. I'm not sure you should be forcing the memset() either. > + type *name = (type *)&name##_u How about? type *const name = &name_##_u.obj; You might want to add: Static_assert(is_constexpr(count), "DEFINE_FLEX: non-constant count " #count); David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, Aug 17, 2023 at 02:35:23PM +0000, David Laight wrote: > From: Przemek Kitszel > > Sent: Wednesday, August 16, 2023 3:06 PM > > > > Using underlying array for on-stack storage lets us to declare > > known-at-compile-time structures without kzalloc(). > > Isn't DEFINE_FLEX() a bit misleading? > One thing it isn't is 'flexible' since it has a fixed size. It works only on flex array structs, and defines a specific instance. I think naming is okay here. > > > +#define DEFINE_FLEX(type, name, member, count) \ > > + union { \ > > + u8 bytes[struct_size_t(type, member, count)]; \ > > + type obj; \ > > + } name##_u __aligned(_Alignof(type)) = {}; \ > > You shouldn't need the _Alignof() it is the default. In the sense that since "type" is in the union, it's okay? > I'm not sure you should be forcing the memset() either. This already got discussed: better to fail safe. > > > + type *name = (type *)&name##_u > > How about? > type *const name = &name_##_u.obj; This is by design (see earlier threads) so that __builtin_object_size(name, 1) will get the correct size. Otherwise it doesn't include the FAM elements in the size. > > You might want to add: > Static_assert(is_constexpr(count), "DEFINE_FLEX: non-constant count " #count); That would be nice, though can Static_assert()s live in the middle of variable definitions? -Kees
From: Kees Cook > Sent: Thursday, August 17, 2023 6:00 PM > > On Thu, Aug 17, 2023 at 02:35:23PM +0000, David Laight wrote: > > From: Przemek Kitszel > > > Sent: Wednesday, August 16, 2023 3:06 PM ... > > > +#define DEFINE_FLEX(type, name, member, count) \ > > > + union { \ > > > + u8 bytes[struct_size_t(type, member, count)]; \ > > > + type obj; \ > > > + } name##_u __aligned(_Alignof(type)) = {}; \ > > > > You shouldn't need the _Alignof() it is the default. > > In the sense that since "type" is in the union, it's okay? The alignment of the union is the larger of the alignments of all its members. Which is what you want. > > I'm not sure you should be forcing the memset() either. > > This already got discussed: better to fail safe. Perhaps call it DEFINE_FLEX_Z() to make this clear and give the option for a non-zeroing version later. Not everyone wants the expense of zeroing everything. .. > > You might want to add: > > Static_assert(is_constexpr(count), "DEFINE_FLEX: non-constant count " #count); > > That would be nice, though can Static_assert()s live in the middle of > variable definitions? I checked and it is fine. (I double-checked by adding a statement and getting an error.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 8/18/23 09:14, David Laight wrote: > From: Kees Cook >> Sent: Thursday, August 17, 2023 6:00 PM >> >> On Thu, Aug 17, 2023 at 02:35:23PM +0000, David Laight wrote: >>> From: Przemek Kitszel >>>> Sent: Wednesday, August 16, 2023 3:06 PM > ... >>>> +#define DEFINE_FLEX(type, name, member, count) \ >>>> + union { \ >>>> + u8 bytes[struct_size_t(type, member, count)]; \ >>>> + type obj; \ >>>> + } name##_u __aligned(_Alignof(type)) = {}; \ >>> >>> You shouldn't need the _Alignof() it is the default. >> >> In the sense that since "type" is in the union, it's okay? > > The alignment of the union is the larger of the alignments > of all its members. > Which is what you want. > >>> I'm not sure you should be forcing the memset() either. >> >> This already got discussed: better to fail safe. > > Perhaps call it DEFINE_FLEX_Z() to make this clear and > give the option for a non-zeroing version later. > Not everyone wants the expense of zeroing everything. per Kees, zeroing should be removed by compiler when not needed: https://lore.kernel.org/intel-wired-lan/202308101128.C4F0FA235@keescook/ Thanks for education on alignment! > > .. >>> You might want to add: >>> Static_assert(is_constexpr(count), "DEFINE_FLEX: non-constant count " #count); >> >> That would be nice, though can Static_assert()s live in the middle of >> variable definitions? > > I checked and it is fine. > (I double-checked by adding a statement and getting an error.) Static_assert with nice wording definitively would make it better, thanks again! > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > >
On 8/16/23 18:35, kernel test robot wrote: > Hi Przemek, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on 479b322ee6feaff612285a0e7f22c022e8cd84eb] > > url: https://github.com/intel-lab-lkp/linux/commits/Przemek-Kitszel/overflow-add-DEFINE_FLEX-for-on-stack-allocs/20230816-221402 > base: 479b322ee6feaff612285a0e7f22c022e8cd84eb > patch link: https://lore.kernel.org/r/20230816140623.452869-2-przemyslaw.kitszel%40intel.com > patch subject: [PATCH net-next v3 1/7] overflow: add DEFINE_FLEX() for on-stack allocs > config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20230817/202308170000.YqabIR9D-lkp@intel.com/config) Rust folks, could you please tell me if this is something I should fix, or I just uncovered some existing bug in "unstable" thing? Perhaps it is worth to mention, diff of v3 vs v2 is: move dummy implementation of __has_builtin() macro to the top of compiler_types.h, just before `#ifndef ASSEMBLY` Przemek > compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) > reproduce: (https://download.01.org/0day-ci/archive/20230817/202308170000.YqabIR9D-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202308170000.YqabIR9D-lkp@intel.com/ > > All errors (new ones prefixed by >>): > >>> thread 'main' panicked at '"ftrace_branch_data_union_(anonymous_at_include/linux/compiler_types_h_144_2)" is not a valid Ident', /opt/cross/rustc-1.68.2-bindgen-0.56.0/cargo/registry/src/github.com-1ecc6299db9ec823/proc-macro2-1.0.24/src/fallback.rs:693:9 > stack backtrace: > 0: rust_begin_unwind > at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/std/src/panicking.rs:575:5 > 1: core::panicking::panic_fmt > at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/core/src/panicking.rs:64:14 > 2: proc_macro2::fallback::Ident::_new > 3: proc_macro2::Ident::new > 4: bindgen::ir::context::BindgenContext::rust_ident > 5: <bindgen::ir::comp::CompInfo as bindgen::codegen::CodeGenerator>::codegen > 6: <bindgen::ir::ty::Type as bindgen::codegen::CodeGenerator>::codegen > 7: <bindgen::ir::item::Item as bindgen::codegen::CodeGenerator>::codegen > 8: <bindgen::ir::comp::CompInfo as bindgen::codegen::CodeGenerator>::codegen > 9: <bindgen::ir::ty::Type as bindgen::codegen::CodeGenerator>::codegen > 10: <bindgen::ir::item::Item as bindgen::codegen::CodeGenerator>::codegen > 11: <bindgen::ir::module::Module as bindgen::codegen::CodeGenerator>::codegen > 12: <bindgen::ir::item::Item as bindgen::codegen::CodeGenerator>::codegen > 13: bindgen::ir::context::BindgenContext::gen > 14: bindgen::Builder::generate > 15: bindgen::main > note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. > make[3]: *** [rust/Makefile:316: rust/uapi/uapi_generated.rs] Error 1 > make[3]: *** Deleting file 'rust/uapi/uapi_generated.rs' >>> thread 'main' panicked at '"ftrace_branch_data_union_(anonymous_at_include/linux/compiler_types_h_144_2)" is not a valid Ident', /opt/cross/rustc-1.68.2-bindgen-0.56.0/cargo/registry/src/github.com-1ecc6299db9ec823/proc-macro2-1.0.24/src/fallback.rs:693:9 > stack backtrace: > 0: rust_begin_unwind > at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/std/src/panicking.rs:575:5 > 1: core::panicking::panic_fmt > at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/core/src/panicking.rs:64:14 > 2: proc_macro2::fallback::Ident::_new > 3: proc_macro2::Ident::new > 4: bindgen::ir::context::BindgenContext::rust_ident > 5: <bindgen::ir::comp::CompInfo as bindgen::codegen::CodeGenerator>::codegen > 6: <bindgen::ir::ty::Type as bindgen::codegen::CodeGenerator>::codegen > 7: <bindgen::ir::item::Item as bindgen::codegen::CodeGenerator>::codegen > 8: <bindgen::ir::comp::CompInfo as bindgen::codegen::CodeGenerator>::codegen > 9: <bindgen::ir::ty::Type as bindgen::codegen::CodeGenerator>::codegen > 10: <bindgen::ir::item::Item as bindgen::codegen::CodeGenerator>::codegen > 11: <bindgen::ir::module::Module as bindgen::codegen::CodeGenerator>::codegen > 12: <bindgen::ir::item::Item as bindgen::codegen::CodeGenerator>::codegen > 13: bindgen::ir::context::BindgenContext::gen > 14: bindgen::Builder::generate > 15: bindgen::main > note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. > make[3]: *** [rust/Makefile:310: rust/bindings/bindings_generated.rs] Error 1 > make[3]: *** Deleting file 'rust/bindings/bindings_generated.rs' > make[3]: Target 'rust/' not remade because of errors. > make[2]: *** [Makefile:1293: prepare] Error 2 > make[1]: *** [Makefile:234: __sub-make] Error 2 > make[1]: Target 'prepare' not remade because of errors. > make: *** [Makefile:234: __sub-make] Error 2 > make: Target 'prepare' not remade because of errors. >
From: Przemek Kitszel > Sent: Friday, August 18, 2023 11:28 AM ... > >>> I'm not sure you should be forcing the memset() either. > >> > >> This already got discussed: better to fail safe. > > > > Perhaps call it DEFINE_FLEX_Z() to make this clear and > > give the option for a non-zeroing version later. > > Not everyone wants the expense of zeroing everything. > > per Kees, zeroing should be removed by compiler when not needed: > https://lore.kernel.org/intel-wired-lan/202308101128.C4F0FA235@keescook/ Expect in the most trivial cases the compiler is pretty much never going to remove the zeroing of the data[] part. I'm also not at all sure what happens if there is a function call between the initialisation and any assignments. With a bit of effort you should be able to pass the '= {}' through into an inner #define. Possibly with the alternative of a caller-provider '= { .obj = call_supplied_initialiser }' The 'not _Z' form would pass an empty argument. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, Aug 18, 2023 at 12:38 PM Przemek Kitszel <przemyslaw.kitszel@intel.com> wrote: > > Rust folks, could you please tell me if this is something I should fix, > or I just uncovered some existing bug in "unstable" thing? > > Perhaps it is worth to mention, diff of v3 vs v2 is: > move dummy implementation of __has_builtin() macro to the top of > compiler_types.h, just before `#ifndef ASSEMBLY` Nothing you need to worry about, it is an issue with old `bindgen` and LLVM >= 16, fixed in commit 08ab786556ff ("rust: bindgen: upgrade to 0.65.1") which is in `rust-next` at the moment. Sorry about that, and thanks for pinging us! LKP / Yujie / Philip: since we got a few reports on this, would it be possible to avoid LLVM >= 16 for Rust-enabled builds for any branch that does not include the new `bindgen` or at least 08ab786556ff? Or, if Greg is OK with that, I guess we could also backport the upgrade, but perhaps it is a bit too much for stable? Cheers, Miguel
On Fri, Aug 18, 2023 at 01:10:07PM +0200, Miguel Ojeda wrote: > On Fri, Aug 18, 2023 at 12:38 PM Przemek Kitszel > <przemyslaw.kitszel@intel.com> wrote: > > > > Rust folks, could you please tell me if this is something I should fix, > > or I just uncovered some existing bug in "unstable" thing? > > > > Perhaps it is worth to mention, diff of v3 vs v2 is: > > move dummy implementation of __has_builtin() macro to the top of > > compiler_types.h, just before `#ifndef ASSEMBLY` > > Nothing you need to worry about, it is an issue with old `bindgen` and > LLVM >= 16, fixed in commit 08ab786556ff ("rust: bindgen: upgrade to > 0.65.1") which is in `rust-next` at the moment. Sorry about that, and > thanks for pinging us! > > LKP / Yujie / Philip: since we got a few reports on this, would it be > possible to avoid LLVM >= 16 for Rust-enabled builds for any branch > that does not include the new `bindgen` or at least 08ab786556ff? Or, Got it, we will update the bot to handle this to avoid further false positive. > if Greg is OK with that, I guess we could also backport the upgrade, > but perhaps it is a bit too much for stable? > > Cheers, > Miguel
On Fri, Aug 18, 2023 at 01:10:07PM +0200, Miguel Ojeda wrote: > On Fri, Aug 18, 2023 at 12:38 PM Przemek Kitszel > <przemyslaw.kitszel@intel.com> wrote: > > > > Rust folks, could you please tell me if this is something I should fix, > > or I just uncovered some existing bug in "unstable" thing? > > > > Perhaps it is worth to mention, diff of v3 vs v2 is: > > move dummy implementation of __has_builtin() macro to the top of > > compiler_types.h, just before `#ifndef ASSEMBLY` > > Nothing you need to worry about, it is an issue with old `bindgen` and > LLVM >= 16, fixed in commit 08ab786556ff ("rust: bindgen: upgrade to > 0.65.1") which is in `rust-next` at the moment. Sorry about that, and > thanks for pinging us! > > LKP / Yujie / Philip: since we got a few reports on this, would it be > possible to avoid LLVM >= 16 for Rust-enabled builds for any branch > that does not include the new `bindgen` or at least 08ab786556ff? Or, > if Greg is OK with that, I guess we could also backport the upgrade, > but perhaps it is a bit too much for stable? Commit is tiny enough for stable backports if it fixes a real issue that everyone needs to address, so I have no objection to taking it for stable releases once it hits Linus's tree. thanks, greg k-h
On 8/18/23 12:49, David Laight wrote: > From: Przemek Kitszel >> Sent: Friday, August 18, 2023 11:28 AM > ... >>>>> I'm not sure you should be forcing the memset() either. >>>> >>>> This already got discussed: better to fail safe. >>> >>> Perhaps call it DEFINE_FLEX_Z() to make this clear and >>> give the option for a non-zeroing version later. >>> Not everyone wants the expense of zeroing everything. >> >> per Kees, zeroing should be removed by compiler when not needed: >> https://lore.kernel.org/intel-wired-lan/202308101128.C4F0FA235@keescook/ > > Expect in the most trivial cases the compiler is pretty much never > going to remove the zeroing of the data[] part. > > I'm also not at all sure what happens if there is a function > call between the initialisation and any assignments. > > With a bit of effort you should be able to pass the '= {}' > through into an inner #define. > Possibly with the alternative of a caller-provider > '= { .obj = call_supplied_initialiser }' > The 'not _Z' form would pass an empty argument. > > David Thanks, makes sense, there could be also DEFINE_FLEX_COUNTED (or DEFINE_FLEX_BOUNDED) to cover Kees's __counted_by() cases. Would you like me to cover/convert any existing code/use cases (as with other patches in the series, to have some examples/actual usage of newly introduced macros)? > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
On 8/23/23 22:52, Przemek Kitszel wrote: > On 8/18/23 12:49, David Laight wrote: >> From: Przemek Kitszel >>> Sent: Friday, August 18, 2023 11:28 AM >> ... >>>>>> I'm not sure you should be forcing the memset() either. >>>>> >>>>> This already got discussed: better to fail safe. >>>> >>>> Perhaps call it DEFINE_FLEX_Z() to make this clear and >>>> give the option for a non-zeroing version later. >>>> Not everyone wants the expense of zeroing everything. >>> >>> per Kees, zeroing should be removed by compiler when not needed: >>> https://lore.kernel.org/intel-wired-lan/202308101128.C4F0FA235@keescook/ >> >> Expect in the most trivial cases the compiler is pretty much never >> going to remove the zeroing of the data[] part. >> >> I'm also not at all sure what happens if there is a function >> call between the initialisation and any assignments. >> >> With a bit of effort you should be able to pass the '= {}' >> through into an inner #define. >> Possibly with the alternative of a caller-provider >> '= { .obj = call_supplied_initialiser }' >> The 'not _Z' form would pass an empty argument. >> >> David > > Thanks, makes sense, there could be also DEFINE_FLEX_COUNTED > (or DEFINE_FLEX_BOUNDED) to cover Kees's __counted_by() cases. > > Would you like me to cover/convert any existing code/use cases (as with > other patches in the series, to have some examples/actual usage of newly > introduced macros)? I did some manual searches and found no obvious candidate :/ will post next version/RFC without _NOINIT() variant. > >> >> - >> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, >> MK1 1PT, UK >> Registration No: 1397386 (Wales) > >
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 547ea1ff806e..f706691fc5b9 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -2,6 +2,15 @@ #ifndef __LINUX_COMPILER_TYPES_H #define __LINUX_COMPILER_TYPES_H +/* + * __has_builtin is supported on gcc >= 10, clang >= 3 and icc >= 21. + * In the meantime, to support gcc < 10, we implement __has_builtin + * by hand. + */ +#ifndef __has_builtin +#define __has_builtin(x) (0) +#endif + #ifndef __ASSEMBLY__ /* @@ -106,17 +115,6 @@ static inline void __chk_io_ptr(const volatile void __iomem *ptr) { } #define __cold #endif -/* Builtins */ - -/* - * __has_builtin is supported on gcc >= 10, clang >= 3 and icc >= 21. - * In the meantime, to support gcc < 10, we implement __has_builtin - * by hand. - */ -#ifndef __has_builtin -#define __has_builtin(x) (0) -#endif - /* Compiler specific macros. */ #ifdef __clang__ #include <linux/compiler-clang.h> @@ -324,6 +322,18 @@ struct ftrace_likely_data { # define __realloc_size(x, ...) #endif +/* + * When the size of an allocated object is needed, use the best available + * mechanism to find it. (For cases where sizeof() cannot be used.) + */ +#if __has_builtin(__builtin_dynamic_object_size) +#define __struct_size(p) __builtin_dynamic_object_size(p, 0) +#define __member_size(p) __builtin_dynamic_object_size(p, 1) +#else +#define __struct_size(p) __builtin_object_size(p, 0) +#define __member_size(p) __builtin_object_size(p, 1) +#endif + #ifndef asm_volatile_goto #define asm_volatile_goto(x...) asm goto(x) #endif diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index da51a83b2829..1e7711185ec6 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -93,13 +93,9 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size) #if __has_builtin(__builtin_dynamic_object_size) #define POS __pass_dynamic_object_size(1) #define POS0 __pass_dynamic_object_size(0) -#define __struct_size(p) __builtin_dynamic_object_size(p, 0) -#define __member_size(p) __builtin_dynamic_object_size(p, 1) #else #define POS __pass_object_size(1) #define POS0 __pass_object_size(0) -#define __struct_size(p) __builtin_object_size(p, 0) -#define __member_size(p) __builtin_object_size(p, 1) #endif #define __compiletime_lessthan(bounds, length) ( \ diff --git a/include/linux/overflow.h b/include/linux/overflow.h index f9b60313eaea..34de97ea9e8e 100644 --- a/include/linux/overflow.h +++ b/include/linux/overflow.h @@ -309,4 +309,24 @@ static inline size_t __must_check size_sub(size_t minuend, size_t subtrahend) #define struct_size_t(type, member, count) \ struct_size((type *)NULL, member, count) +/** + * DEFINE_FLEX() - Define an on-stack instance of structure with a trailing + * flexible array member. + * + * @type: structure type name, including "struct" keyword. + * @name: Name for a variable to define. + * @member: Name of the array member. + * @count: Number of elements in the array; must be compile-time const. + * + * Define a zeroed, on-stack, instance of @type structure with a trailing + * flexible array member. + * Use __struct_size(@name) to get compile-time size of it afterwards. + */ +#define DEFINE_FLEX(type, name, member, count) \ + union { \ + u8 bytes[struct_size_t(type, member, count)]; \ + type obj; \ + } name##_u __aligned(_Alignof(type)) = {}; \ + type *name = (type *)&name##_u + #endif /* __LINUX_OVERFLOW_H */
Add DEFINE_FLEX() macro for on-stack allocations of structs with flexible array member. Expose __struct_size() macro outside of fortify-string.h, as it could be used to read size of structs allocated by DEFINE_FLEX(). Move __member_size() alongside it. -Kees Using underlying array for on-stack storage lets us to declare known-at-compile-time structures without kzalloc(). Actual usage for ice driver is in following patches of the series. Missing __has_builtin() workaround is moved up to serve also assembly compilation with m68k-linux-gcc, see [1]. Error was (note the .S file extension): In file included from ../include/linux/linkage.h:5, from ../arch/m68k/fpsp040/skeleton.S:40: ../include/linux/compiler_types.h:331:5: warning: "__has_builtin" is not defined, evaluates to 0 [-Wundef] 331 | #if __has_builtin(__builtin_dynamic_object_size) | ^~~~~~~~~~~~~ ../include/linux/compiler_types.h:331:18: error: missing binary operator before token "(" 331 | #if __has_builtin(__builtin_dynamic_object_size) | ^ [1] https://lore.kernel.org/netdev/202308112122.OuF0YZqL-lkp@intel.com/ Co-developed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Kees Cook <keescook@chromium.org> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> --- v3: remove old macro needlessly kept in v2; fix build warning; reword doc comment; v2: Kees: reuse __struct_size() instead of adding new macro (adding Kees as Co-dev here); v1: change macro name; add macro for size read; accept struct type instead of ptr to it; change alignment; --- include/linux/compiler_types.h | 32 +++++++++++++++++++++----------- include/linux/fortify-string.h | 4 ---- include/linux/overflow.h | 20 ++++++++++++++++++++ 3 files changed, 41 insertions(+), 15 deletions(-)