remaining flexible-array conversions

remaining flexible-array conversions

[Gustavo A. R. Silva]

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

Re: remaining flexible-array conversions

[Linus Torvalds]

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

Re: remaining flexible-array conversions

[Gustavo A. R. Silva]

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

Re: remaining flexible-array conversions

[Nathan Chancellor]

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

Re: remaining flexible-array conversions

[Jason Gunthorpe]

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

Re: remaining flexible-array conversions

[Kees Cook]

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

Re: remaining flexible-array conversions

[Gustavo A. R. Silva]

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

Re: remaining flexible-array conversions

[Jan Engelhardt]

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.

Re: remaining flexible-array conversions

[Jason Gunthorpe]

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

Re: remaining flexible-array conversions

[Gustavo A. R. Silva]

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