* remaining flexible-array conversions @ 2020-04-22 18:26 Gustavo A. R. Silva 2020-04-23 19:15 ` Linus Torvalds 2020-04-24 3:47 ` Nathan Chancellor 0 siblings, 2 replies; 10+ messages in thread From: Gustavo A. R. Silva @ 2020-04-22 18:26 UTC (permalink / raw) To: Linus Torvalds; +Cc: Kees Cook, Greg KH, Linux Kernel Mailing List Hi Linus, Just wanted to ask you if you would agree on pulling the remaining flexible-array conversions all at once, after they bake for a couple of weeks in linux-next[1] This is not a disruptive change and there are no code generation differences. So, I think it would make better use of everyone's time if you pull this treewide patch[2] from my tree (after sending you a proper pull-request, of course) sometime in the next couple of weeks. Notice that the treewide patch I mention here has been successfully built (on top of v5.7-rc1) for multiple architectures (arm, arm64, sparc, powerpc, ia64, s390, i386, nios2, c6x, xtensa, openrisc, mips, parisc, x86_64, riscv, sh, sparc64) and 82 different configurations with the help of the 0-day CI guys[3]. What do you think? [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d496496793ff69c4a6b1262a0001eb5cd0a56544 [2] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=for-next/kspp&id=d783301058f3d3605f9ad34f0192692ef572d663 [3] https://github.com/GustavoARSilva/linux-hardening/blob/master/cii/kernel-ci/kspp-fam0-20200420.md Thanks -- Gustavo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: remaining flexible-array conversions 2020-04-22 18:26 remaining flexible-array conversions Gustavo A. R. Silva @ 2020-04-23 19:15 ` Linus Torvalds 2020-04-24 14:53 ` Gustavo A. R. Silva 2020-04-24 3:47 ` Nathan Chancellor 1 sibling, 1 reply; 10+ messages in thread From: Linus Torvalds @ 2020-04-23 19:15 UTC (permalink / raw) To: Gustavo A. R. Silva; +Cc: Kees Cook, Greg KH, Linux Kernel Mailing List On Wed, Apr 22, 2020 at 11:23 AM Gustavo A. R. Silva <gustavo@embeddedor.com> wrote: > > Just wanted to ask you if you would agree on pulling the remaining > flexible-array conversions all at once, after they bake for a couple > of weeks in linux-next[1] > > This is not a disruptive change and there are no code generation > differences. The "no code generation differnces" is a good thing, but how was that tested? I assume just one configuration or architecture? Also, it bothers me a bit that some of the diff is unrelated whitespace cleanup. I'd actually be happier with a pure scripted patch (maybe coccinelle, maybe something else), than something that looks like it's at least partly manual. In particular, if I can re-create the diff with a script, I'd not have to worry about verifying it other ways.. Linus ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: remaining flexible-array conversions 2020-04-23 19:15 ` Linus Torvalds @ 2020-04-24 14:53 ` Gustavo A. R. Silva 0 siblings, 0 replies; 10+ messages in thread From: Gustavo A. R. Silva @ 2020-04-24 14:53 UTC (permalink / raw) To: Linus Torvalds; +Cc: Kees Cook, Greg KH, Linux Kernel Mailing List On 4/23/20 14:15, Linus Torvalds wrote: > On Wed, Apr 22, 2020 at 11:23 AM Gustavo A. R. Silva > <gustavo@embeddedor.com> wrote: >> >> Just wanted to ask you if you would agree on pulling the remaining >> flexible-array conversions all at once, after they bake for a couple >> of weeks in linux-next[1] >> >> This is not a disruptive change and there are no code generation >> differences. > > The "no code generation differnces" is a good thing, but how was that > tested? I assume just one configuration or architecture? > That's correct. I used pahole to test it (x86_64, allyesconfig). I will test other archs (arm, arm64, powerpc, mips, etc...). > Also, it bothers me a bit that some of the diff is unrelated > whitespace cleanup. I'd actually be happier with a pure scripted patch > (maybe coccinelle, maybe something else), than something that looks > like it's at least partly manual. In particular, if I can re-create > the diff with a script, I'd not have to worry about verifying it other > ways.. > Yeah. I fixed up some "tabs and spaces" checkpatch warnings --I can omit this step in the future. I also had to drop changes from files that were causing warnings --with the idea of fixing those warnings later, after landing all the "non-problematic" conversions, first. I will generate an ad-hoc bash script for this and will send it to you. Thanks -- Gustavo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: remaining flexible-array conversions 2020-04-22 18:26 remaining flexible-array conversions Gustavo A. R. Silva 2020-04-23 19:15 ` Linus Torvalds @ 2020-04-24 3:47 ` Nathan Chancellor 2020-04-24 12:15 ` Jason Gunthorpe 2020-04-24 15:17 ` Gustavo A. R. Silva 1 sibling, 2 replies; 10+ messages in thread From: Nathan Chancellor @ 2020-04-24 3:47 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Linus Torvalds, Kees Cook, Greg KH, Linux Kernel Mailing List, linux-rdma, clang-built-linux Hi Gustavo, On Wed, Apr 22, 2020 at 01:26:02PM -0500, Gustavo A. R. Silva wrote: > Hi Linus, > > Just wanted to ask you if you would agree on pulling the remaining > flexible-array conversions all at once, after they bake for a couple > of weeks in linux-next[1] > > This is not a disruptive change and there are no code generation > differences. So, I think it would make better use of everyone's time > if you pull this treewide patch[2] from my tree (after sending you a > proper pull-request, of course) sometime in the next couple of weeks. > > Notice that the treewide patch I mention here has been successfully > built (on top of v5.7-rc1) for multiple architectures (arm, arm64, > sparc, powerpc, ia64, s390, i386, nios2, c6x, xtensa, openrisc, mips, > parisc, x86_64, riscv, sh, sparc64) and 82 different configurations > with the help of the 0-day CI guys[3]. > > What do you think? > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d496496793ff69c4a6b1262a0001eb5cd0a56544 > [2] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=for-next/kspp&id=d783301058f3d3605f9ad34f0192692ef572d663 > [3] https://github.com/GustavoARSilva/linux-hardening/blob/master/cii/kernel-ci/kspp-fam0-20200420.md > > Thanks > -- > Gustavo That patch in -next appears to introduce some warnings with clang when CONFIG_UAPI_HEADER_TEST is enabled (allyesconfig/allmodconfig exposed it for us with KernelCI [1]): ./usr/include/rdma/ib_user_verbs.h:436:34: warning: field 'base' with variable sized type 'struct ib_uverbs_create_cq_resp' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end] struct ib_uverbs_create_cq_resp base; ^ ./usr/include/rdma/ib_user_verbs.h:647:34: warning: field 'base' with variable sized type 'struct ib_uverbs_create_qp_resp' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end] struct ib_uverbs_create_qp_resp base; ^ ./usr/include/rdma/ib_user_verbs.h:743:29: warning: field 'base' with variable sized type 'struct ib_uverbs_modify_qp' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end] struct ib_uverbs_modify_qp base; ^ 3 warnings generated. I presume this is part of the point of the conversion since you mention a compiler warning when the flexible member is not at the end of a struct. How should they be fixed? That should probably happen before the patch gets merged. [1]: https://kernelci.org/build/id/5ea17b1b77113098348ec6db/logs/ Cheers, Nathan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: remaining flexible-array conversions 2020-04-24 3:47 ` Nathan Chancellor @ 2020-04-24 12:15 ` Jason Gunthorpe 2020-04-24 15:24 ` Kees Cook 2020-10-08 15:24 ` Jan Engelhardt 2020-04-24 15:17 ` Gustavo A. R. Silva 1 sibling, 2 replies; 10+ messages in thread From: Jason Gunthorpe @ 2020-04-24 12:15 UTC (permalink / raw) To: Nathan Chancellor Cc: Gustavo A. R. Silva, Linus Torvalds, Kees Cook, Greg KH, Linux Kernel Mailing List, linux-rdma, clang-built-linux On Thu, Apr 23, 2020 at 08:47:04PM -0700, Nathan Chancellor wrote: > Hi Gustavo, > > On Wed, Apr 22, 2020 at 01:26:02PM -0500, Gustavo A. R. Silva wrote: > > Hi Linus, > > > > Just wanted to ask you if you would agree on pulling the remaining > > flexible-array conversions all at once, after they bake for a couple > > of weeks in linux-next[1] > > > > This is not a disruptive change and there are no code generation > > differences. So, I think it would make better use of everyone's time > > if you pull this treewide patch[2] from my tree (after sending you a > > proper pull-request, of course) sometime in the next couple of weeks. > > > > Notice that the treewide patch I mention here has been successfully > > built (on top of v5.7-rc1) for multiple architectures (arm, arm64, > > sparc, powerpc, ia64, s390, i386, nios2, c6x, xtensa, openrisc, mips, > > parisc, x86_64, riscv, sh, sparc64) and 82 different configurations > > with the help of the 0-day CI guys[3]. > > > > What do you think? > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d496496793ff69c4a6b1262a0001eb5cd0a56544 > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=for-next/kspp&id=d783301058f3d3605f9ad34f0192692ef572d663 > > [3] https://github.com/GustavoARSilva/linux-hardening/blob/master/cii/kernel-ci/kspp-fam0-20200420.md > > > > Thanks > > That patch in -next appears to introduce some warnings with clang when > CONFIG_UAPI_HEADER_TEST is enabled (allyesconfig/allmodconfig exposed it > for us with KernelCI [1]): Indeed, I've tried these conversions before and run into problems like this, and more. Particularly in userspace these structs also get embedded in other structs and the warnings explode. Please drop changes to ib_user_verbs.h from your series > ./usr/include/rdma/ib_user_verbs.h:436:34: warning: field 'base' with > variable sized type 'struct ib_uverbs_create_cq_resp' not at the end of > a struct or class is a GNU extension > [-Wgnu-variable-sized-type-not-at-end] > struct ib_uverbs_create_cq_resp base; > ^ > ./usr/include/rdma/ib_user_verbs.h:647:34: warning: field 'base' with > variable sized type 'struct ib_uverbs_create_qp_resp' not at the end of > a struct or class is a GNU extension > [-Wgnu-variable-sized-type-not-at-end] > struct ib_uverbs_create_qp_resp base; > ^ > ./usr/include/rdma/ib_user_verbs.h:743:29: warning: field 'base' with > variable sized type 'struct ib_uverbs_modify_qp' not at the end of a > struct or class is a GNU extension > [-Wgnu-variable-sized-type-not-at-end] > struct ib_uverbs_modify_qp base; > ^ > 3 warnings generated. > > I presume this is part of the point of the conversion since you mention > a compiler warning when the flexible member is not at the end of a > struct. How should they be fixed? That should probably happen before the > patch gets merged. The flexible member IS at the end of the struct and is often intended to cover the memory in the enclosing struct. I think clang is being overzealous with its warnings here. Jason ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: remaining flexible-array conversions 2020-04-24 12:15 ` Jason Gunthorpe @ 2020-04-24 15:24 ` Kees Cook 2020-04-24 15:36 ` Gustavo A. R. Silva 2020-10-08 15:24 ` Jan Engelhardt 1 sibling, 1 reply; 10+ messages in thread From: Kees Cook @ 2020-04-24 15:24 UTC (permalink / raw) To: Jason Gunthorpe Cc: Nathan Chancellor, Gustavo A. R. Silva, Linus Torvalds, Greg KH, Linux Kernel Mailing List, linux-rdma, clang-built-linux On Fri, Apr 24, 2020 at 09:15:53AM -0300, Jason Gunthorpe wrote: > On Thu, Apr 23, 2020 at 08:47:04PM -0700, Nathan Chancellor wrote: > > Hi Gustavo, > > > > On Wed, Apr 22, 2020 at 01:26:02PM -0500, Gustavo A. R. Silva wrote: > > > Hi Linus, > > > > > > Just wanted to ask you if you would agree on pulling the remaining > > > flexible-array conversions all at once, after they bake for a couple > > > of weeks in linux-next[1] > > > > > > This is not a disruptive change and there are no code generation > > > differences. So, I think it would make better use of everyone's time > > > if you pull this treewide patch[2] from my tree (after sending you a > > > proper pull-request, of course) sometime in the next couple of weeks. > > > > > > Notice that the treewide patch I mention here has been successfully > > > built (on top of v5.7-rc1) for multiple architectures (arm, arm64, > > > sparc, powerpc, ia64, s390, i386, nios2, c6x, xtensa, openrisc, mips, > > > parisc, x86_64, riscv, sh, sparc64) and 82 different configurations > > > with the help of the 0-day CI guys[3]. > > > > > > What do you think? > > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d496496793ff69c4a6b1262a0001eb5cd0a56544 > > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=for-next/kspp&id=d783301058f3d3605f9ad34f0192692ef572d663 > > > [3] https://github.com/GustavoARSilva/linux-hardening/blob/master/cii/kernel-ci/kspp-fam0-20200420.md > > > > > > Thanks > > > > That patch in -next appears to introduce some warnings with clang when > > CONFIG_UAPI_HEADER_TEST is enabled (allyesconfig/allmodconfig exposed it > > for us with KernelCI [1]): > > Indeed, I've tried these conversions before and run into problems like > this, and more. Particularly in userspace these structs also get > embedded in other structs and the warnings explode. > > Please drop changes to ib_user_verbs.h from your series We might need to make the UAPI changes separately (or not at all). -- Kees Cook ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: remaining flexible-array conversions 2020-04-24 15:24 ` Kees Cook @ 2020-04-24 15:36 ` Gustavo A. R. Silva 0 siblings, 0 replies; 10+ messages in thread From: Gustavo A. R. Silva @ 2020-04-24 15:36 UTC (permalink / raw) To: Kees Cook, Jason Gunthorpe Cc: Nathan Chancellor, Linus Torvalds, Greg KH, Linux Kernel Mailing List, linux-rdma, clang-built-linux On 4/24/20 10:24, Kees Cook wrote: > On Fri, Apr 24, 2020 at 09:15:53AM -0300, Jason Gunthorpe wrote: >> On Thu, Apr 23, 2020 at 08:47:04PM -0700, Nathan Chancellor wrote: >>> Hi Gustavo, >>> >>> On Wed, Apr 22, 2020 at 01:26:02PM -0500, Gustavo A. R. Silva wrote: >>>> Hi Linus, >>>> >>>> Just wanted to ask you if you would agree on pulling the remaining >>>> flexible-array conversions all at once, after they bake for a couple >>>> of weeks in linux-next[1] >>>> >>>> This is not a disruptive change and there are no code generation >>>> differences. So, I think it would make better use of everyone's time >>>> if you pull this treewide patch[2] from my tree (after sending you a >>>> proper pull-request, of course) sometime in the next couple of weeks. >>>> >>>> Notice that the treewide patch I mention here has been successfully >>>> built (on top of v5.7-rc1) for multiple architectures (arm, arm64, >>>> sparc, powerpc, ia64, s390, i386, nios2, c6x, xtensa, openrisc, mips, >>>> parisc, x86_64, riscv, sh, sparc64) and 82 different configurations >>>> with the help of the 0-day CI guys[3]. >>>> >>>> What do you think? >>>> >>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d496496793ff69c4a6b1262a0001eb5cd0a56544 >>>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=for-next/kspp&id=d783301058f3d3605f9ad34f0192692ef572d663 >>>> [3] https://github.com/GustavoARSilva/linux-hardening/blob/master/cii/kernel-ci/kspp-fam0-20200420.md >>>> >>>> Thanks >>> >>> That patch in -next appears to introduce some warnings with clang when >>> CONFIG_UAPI_HEADER_TEST is enabled (allyesconfig/allmodconfig exposed it >>> for us with KernelCI [1]): >> >> Indeed, I've tried these conversions before and run into problems like >> this, and more. Particularly in userspace these structs also get >> embedded in other structs and the warnings explode. >> >> Please drop changes to ib_user_verbs.h from your series > > We might need to make the UAPI changes separately (or not at all). > I agree. In the meantime I've dropped the changes for ib_user_verbs.h and will do the same for all the UAPI files. -- Gustavo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: remaining flexible-array conversions 2020-04-24 12:15 ` Jason Gunthorpe 2020-04-24 15:24 ` Kees Cook @ 2020-10-08 15:24 ` Jan Engelhardt 2020-10-08 15:54 ` Jason Gunthorpe 1 sibling, 1 reply; 10+ messages in thread From: Jan Engelhardt @ 2020-10-08 15:24 UTC (permalink / raw) To: Jason Gunthorpe Cc: Nathan Chancellor, Gustavo A. R. Silva, Linus Torvalds, Kees Cook, Greg KH, Linux Kernel Mailing List, linux-rdma, clang-built-linux On Friday 2020-04-24 14:15, Jason Gunthorpe wrote: >> ./usr/include/rdma/ib_user_verbs.h:436:34: warning: field 'base' with >> variable sized type 'struct ib_uverbs_create_cq_resp' not at the end of >> a struct or class is a GNU extension >> [-Wgnu-variable-sized-type-not-at-end] >> struct ib_uverbs_create_cq_resp base; >> ^ >> I presume this is part of the point of the conversion since you mention >> a compiler warning when the flexible member is not at the end of a >> struct. How should they be fixed? That should probably happen before the >> patch gets merged. > >The flexible member IS at the end of the struct and is often intended >to cover the memory in the enclosing struct. There is no guarantee for the presence of such a struct. In the case of the RDMA headers, even if we assume best conditions -- a block of malloc(sizeof(struct ib_uverbs_create_cq_resp) + sizeof(u64)*N) and not some struct -- it smells a lot like undefined behavior, because ib_uverbs_create_cq_resp::driver_data accesses data as u64 while ib_uverbs_ex_create_cq_resp::comp_mask and friends are u32. There has got to be some aliasing rule in C that causes RDMA's purported use-case to be UB. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: remaining flexible-array conversions 2020-10-08 15:24 ` Jan Engelhardt @ 2020-10-08 15:54 ` Jason Gunthorpe 0 siblings, 0 replies; 10+ messages in thread From: Jason Gunthorpe @ 2020-10-08 15:54 UTC (permalink / raw) To: Jan Engelhardt Cc: Nathan Chancellor, Gustavo A. R. Silva, Linus Torvalds, Kees Cook, Greg KH, Linux Kernel Mailing List, linux-rdma, clang-built-linux On Thu, Oct 08, 2020 at 05:24:03PM +0200, Jan Engelhardt wrote: > In the case of the RDMA headers, even if we assume best conditions -- > a block of malloc(sizeof(struct ib_uverbs_create_cq_resp) + > sizeof(u64)*N) and not some struct -- it smells a lot like undefined > behavior, because ib_uverbs_create_cq_resp::driver_data accesses data > as u64 driver_data[] is never accessed, it is only used as a pointer. Aliasing should be OK because all accesses to those bytes consistently use u32. Fixing the compiler warning requires significant edits of kernel and user code, not entirely sure it is worthwhile. If someone wants to do it let me know and I'll give some guidance. Jason ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: remaining flexible-array conversions 2020-04-24 3:47 ` Nathan Chancellor 2020-04-24 12:15 ` Jason Gunthorpe @ 2020-04-24 15:17 ` Gustavo A. R. Silva 1 sibling, 0 replies; 10+ messages in thread From: Gustavo A. R. Silva @ 2020-04-24 15:17 UTC (permalink / raw) To: Nathan Chancellor Cc: Linus Torvalds, Kees Cook, Greg KH, Linux Kernel Mailing List, linux-rdma, clang-built-linux Hi Nathan, On 4/23/20 22:47, Nathan Chancellor wrote: > Hi Gustavo, > > That patch in -next appears to introduce some warnings with clang when > CONFIG_UAPI_HEADER_TEST is enabled (allyesconfig/allmodconfig exposed it > for us with KernelCI [1]): > Thanks a lot for reporting this. > ./usr/include/rdma/ib_user_verbs.h:436:34: warning: field 'base' with > variable sized type 'struct ib_uverbs_create_cq_resp' not at the end of > a struct or class is a GNU extension > [-Wgnu-variable-sized-type-not-at-end] > struct ib_uverbs_create_cq_resp base; > ^ > ./usr/include/rdma/ib_user_verbs.h:647:34: warning: field 'base' with > variable sized type 'struct ib_uverbs_create_qp_resp' not at the end of > a struct or class is a GNU extension > [-Wgnu-variable-sized-type-not-at-end] > struct ib_uverbs_create_qp_resp base; > ^ > ./usr/include/rdma/ib_user_verbs.h:743:29: warning: field 'base' with > variable sized type 'struct ib_uverbs_modify_qp' not at the end of a > struct or class is a GNU extension > [-Wgnu-variable-sized-type-not-at-end] > struct ib_uverbs_modify_qp base; > ^ > 3 warnings generated. > > I presume this is part of the point of the conversion since you mention > a compiler warning when the flexible member is not at the end of a > struct. How should they be fixed? That should probably happen before the > patch gets merged. > For all the cases above, the solution seems to be to move the declaration of the "base" member to the end of the corresponding structure, as below: diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h index a390a667b3f3..e05538be8b30 100644 --- a/include/uapi/rdma/ib_user_verbs.h +++ b/include/uapi/rdma/ib_user_verbs.h @@ -644,9 +644,9 @@ struct ib_uverbs_create_qp_resp { }; struct ib_uverbs_ex_create_qp_resp { - struct ib_uverbs_create_qp_resp base; __u32 comp_mask; __u32 response_length; + struct ib_uverbs_create_qp_resp base; }; /* but I guess this will change the ABI? Also, notice that: "A structure containing a flexible array member, or a union containing such a structure (possibly recursively), may not be a member of a structure or an element of an array. (However, these uses are permitted by GCC as extensions.)"[1] [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html Thanks -- Gustavo ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-08 15:54 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-22 18:26 remaining flexible-array conversions Gustavo A. R. Silva 2020-04-23 19:15 ` Linus Torvalds 2020-04-24 14:53 ` Gustavo A. R. Silva 2020-04-24 3:47 ` Nathan Chancellor 2020-04-24 12:15 ` Jason Gunthorpe 2020-04-24 15:24 ` Kees Cook 2020-04-24 15:36 ` Gustavo A. R. Silva 2020-10-08 15:24 ` Jan Engelhardt 2020-10-08 15:54 ` Jason Gunthorpe 2020-04-24 15:17 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).