[PATCH] docs: deprecated.rst: Add note to the use of struct_size() helper

[PATCH] docs: deprecated.rst: Add note to the use of struct_size() helper

[Gustavo A. R. Silva]

Add a note to educate people about the proper use of struct_size() when
the trailing array in the enclosing structure is a one-element array.

Zero-length and one-element arrays will soon be removed from the kernel,
but in the meantime, it's worth letting people know how to correctly
use struct_size() together with such constructs.

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---

Kees,
 
This is not substitute for the patch I'll write about flexible-arrays
and the deprecation of zero-lenght and one-element arrays.

Thanks
--
Gustavo

 Documentation/process/deprecated.rst | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
index 652e2aa02a66c..0b7b37718bf96 100644
--- a/Documentation/process/deprecated.rst
+++ b/Documentation/process/deprecated.rst
@@ -85,6 +85,17 @@ Instead, use the helper::
 
 	header = kzalloc(struct_size(header, item, count), GFP_KERNEL);
 
+NOTE: You might want to use the following form in case the trailing array
+is a one-element array, as unlike zero-length arrays and flexible-array
+members, `one-element arrays occupy at least as much space as a single
+object of the type <https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html>`_,
+hence they contribute to the size of the enclosing structure::
+
+	header = kzalloc(struct_size(header, item, count - 1), GFP_KERNEL);
+
+It's also worth noting that one-element arrays --together with zero-length
+arrays-- will soon be completely removed from the codebase and deprecated.
+
 See array_size(), array3_size(), and struct_size(),
 for more details as well as the related check_add_overflow() and
 check_mul_overflow() family of functions.
-- 
2.27.0

Re: [PATCH] docs: deprecated.rst: Add note to the use of struct_size() helper

[Kees Cook]

On Thu, Jun 04, 2020 at 12:21:38PM -0500, Gustavo A. R. Silva wrote:
> Add a note to educate people about the proper use of struct_size() when
> the trailing array in the enclosing structure is a one-element array.
> 
> Zero-length and one-element arrays will soon be removed from the kernel,
> but in the meantime, it's worth letting people know how to correctly
> use struct_size() together with such constructs.
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> 
> Kees,
>  
> This is not substitute for the patch I'll write about flexible-arrays
> and the deprecation of zero-lenght and one-element arrays.

Hm, hm. I think I'd rather just get the 0/1-array docs written, since
that will mean this paragraph isn't needed at all. (Or rather, it can be
modified to say "if you're using struct_size() on a 1-array, stop using
a 1-array, see [link]". If someone is going to switch around their code,
they need to switch to flex at the same time, IMO.

-Kees

> 
> Thanks
> --
> Gustavo
> 
>  Documentation/process/deprecated.rst | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
> index 652e2aa02a66c..0b7b37718bf96 100644
> --- a/Documentation/process/deprecated.rst
> +++ b/Documentation/process/deprecated.rst
> @@ -85,6 +85,17 @@ Instead, use the helper::
>  
>  	header = kzalloc(struct_size(header, item, count), GFP_KERNEL);
>  
> +NOTE: You might want to use the following form in case the trailing array
> +is a one-element array, as unlike zero-length arrays and flexible-array
> +members, `one-element arrays occupy at least as much space as a single
> +object of the type <https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html>`_,
> +hence they contribute to the size of the enclosing structure::
> +
> +	header = kzalloc(struct_size(header, item, count - 1), GFP_KERNEL);
> +
> +It's also worth noting that one-element arrays --together with zero-length
> +arrays-- will soon be completely removed from the codebase and deprecated.
> +
>  See array_size(), array3_size(), and struct_size(),
>  for more details as well as the related check_add_overflow() and
>  check_mul_overflow() family of functions.
> -- 
> 2.27.0
> 

-- 
Kees Cook

Re: [PATCH] docs: deprecated.rst: Add note to the use of struct_size() helper

[Gustavo A. R. Silva]

On Thu, Jun 04, 2020 at 10:49:19AM -0700, Kees Cook wrote:
> On Thu, Jun 04, 2020 at 12:21:38PM -0500, Gustavo A. R. Silva wrote:
> > Add a note to educate people about the proper use of struct_size() when
> > the trailing array in the enclosing structure is a one-element array.
> > 
> > Zero-length and one-element arrays will soon be removed from the kernel,
> > but in the meantime, it's worth letting people know how to correctly
> > use struct_size() together with such constructs.
> > 
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > ---
> > 
> > Kees,
> >  
> > This is not substitute for the patch I'll write about flexible-arrays
> > and the deprecation of zero-lenght and one-element arrays.
> 
> Hm, hm. I think I'd rather just get the 0/1-array docs written, since
> that will mean this paragraph isn't needed at all. (Or rather, it can be

Yeah. My reasoning for is that it will take a while --at least one 
development cycle more-- to completely get rid of all the 0/1-arrays.
Also, this was motivated by the following comments from Christian
König:

"May I suggest that we add a section how to correctly do this to
Documentation/process/coding-style.rst or similar document?

I've seen a bunch of different approaches and some even doesn't work
with some gcc versions and result in a broken binary."[1]

> modified to say "if you're using struct_size() on a 1-array, stop using
> a 1-array, see [link]". If someone is going to switch around their code,
> they need to switch to flex at the same time, IMO.
> 

I agree with this. I can add the comments in quotes you suggest to this
patch.

But I think we can add this note while I continue working on the flexible-array
conversions. Once that work is complete, I can go back and update the
documentation. :)

What do you think?

Thanks
--
Gustavo

[1] https://lore.kernel.org/lkml/1065d63e-7959-e4b4-af4e-70607ba92296@amd.com/

> > 
> >  Documentation/process/deprecated.rst | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
> > index 652e2aa02a66c..0b7b37718bf96 100644
> > --- a/Documentation/process/deprecated.rst
> > +++ b/Documentation/process/deprecated.rst
> > @@ -85,6 +85,17 @@ Instead, use the helper::
> >  
> >  	header = kzalloc(struct_size(header, item, count), GFP_KERNEL);
> >  
> > +NOTE: You might want to use the following form in case the trailing array
> > +is a one-element array, as unlike zero-length arrays and flexible-array
> > +members, `one-element arrays occupy at least as much space as a single
> > +object of the type <https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html>`_,
> > +hence they contribute to the size of the enclosing structure::
> > +
> > +	header = kzalloc(struct_size(header, item, count - 1), GFP_KERNEL);
> > +
> > +It's also worth noting that one-element arrays --together with zero-length
> > +arrays-- will soon be completely removed from the codebase and deprecated.
> > +
> >  See array_size(), array3_size(), and struct_size(),
> >  for more details as well as the related check_add_overflow() and
> >  check_mul_overflow() family of functions.
> > -- 
> > 2.27.0
> > 
> 
> -- 
> Kees Cook

Re: [PATCH] docs: deprecated.rst: Add note to the use of struct_size() helper

[Kees Cook]

On Thu, Jun 04, 2020 at 01:21:23PM -0500, Gustavo A. R. Silva wrote:
> Yeah. My reasoning for is that it will take a while --at least one 
> development cycle more-- to completely get rid of all the 0/1-arrays.

Right -- but we need a place to point people when we tell them "please
don't use 0-byte and 1-byte arrays", and the deprecated.rst is the place
for that.

Having it in deprecated.rst once they're all gone only serves to explain
why various compiler flags are enabled, etc. But while they're being
removed, it serves as a single place to document the issue (as in, much
of the flex-array patch commit log "boilerplate" can actually be
repeated in deprecated.rst.

> But I think we can add this note while I continue working on the flexible-array
> conversions. Once that work is complete, I can go back and update the
> documentation. :)
> 
> What do you think?

I think we need to document it at the beginning of the work (and I
should have asked for this earlier). So let's just add a new section on
dynamic array usage, etc. It can include a note about struct_size() as
an example for why 1-byte arrays are so weird. :)

-- 
Kees Cook

Re: [PATCH] docs: deprecated.rst: Add note to the use of struct_size() helper

[Gustavo A. R. Silva]

On Thu, Jun 04, 2020 at 01:25:26PM -0700, Kees Cook wrote:
> On Thu, Jun 04, 2020 at 01:21:23PM -0500, Gustavo A. R. Silva wrote:
> > Yeah. My reasoning for is that it will take a while --at least one 
> > development cycle more-- to completely get rid of all the 0/1-arrays.
> 
> Right -- but we need a place to point people when we tell them "please
> don't use 0-byte and 1-byte arrays", and the deprecated.rst is the place
> for that.
> 
> Having it in deprecated.rst once they're all gone only serves to explain
> why various compiler flags are enabled, etc. But while they're being
> removed, it serves as a single place to document the issue (as in, much
> of the flex-array patch commit log "boilerplate" can actually be
> repeated in deprecated.rst.
> 
> > But I think we can add this note while I continue working on the flexible-array
> > conversions. Once that work is complete, I can go back and update the
> > documentation. :)
> > 
> > What do you think?
> 
> I think we need to document it at the beginning of the work (and I
> should have asked for this earlier). So let's just add a new section on
> dynamic array usage, etc. It can include a note about struct_size() as
> an example for why 1-byte arrays are so weird. :)
> 

Got ya. :)

One last thing... I was thinking on adding such section (dynamic array
usage) to coding-style.rst, explaining how to use struct_size() and
transform the different open-coded versions we currently have in the
kernel, e.g. I have seen people use offsetof() --and sometimes open-coded
versions of sizeof_field()-- and its open-coded version to do arithmetic
in allocator arguments.

Thanks
--
Gustavo

Re: [PATCH] docs: deprecated.rst: Add note to the use of struct_size() helper

[Kees Cook]

On Thu, Jun 04, 2020 at 04:19:03PM -0500, Gustavo A. R. Silva wrote:
> On Thu, Jun 04, 2020 at 01:25:26PM -0700, Kees Cook wrote:
> > On Thu, Jun 04, 2020 at 01:21:23PM -0500, Gustavo A. R. Silva wrote:
> > > Yeah. My reasoning for is that it will take a while --at least one 
> > > development cycle more-- to completely get rid of all the 0/1-arrays.
> > 
> > Right -- but we need a place to point people when we tell them "please
> > don't use 0-byte and 1-byte arrays", and the deprecated.rst is the place
> > for that.
> > 
> > Having it in deprecated.rst once they're all gone only serves to explain
> > why various compiler flags are enabled, etc. But while they're being
> > removed, it serves as a single place to document the issue (as in, much
> > of the flex-array patch commit log "boilerplate" can actually be
> > repeated in deprecated.rst.
> > 
> > > But I think we can add this note while I continue working on the flexible-array
> > > conversions. Once that work is complete, I can go back and update the
> > > documentation. :)
> > > 
> > > What do you think?
> > 
> > I think we need to document it at the beginning of the work (and I
> > should have asked for this earlier). So let's just add a new section on
> > dynamic array usage, etc. It can include a note about struct_size() as
> > an example for why 1-byte arrays are so weird. :)
> > 
> 
> Got ya. :)
> 
> One last thing... I was thinking on adding such section (dynamic array
> usage) to coding-style.rst, explaining how to use struct_size() and
> transform the different open-coded versions we currently have in the
> kernel, e.g. I have seen people use offsetof() --and sometimes open-coded
> versions of sizeof_field()-- and its open-coded version to do arithmetic
> in allocator arguments.

Yeah, that sounds good to me!

-Kees

> 
> Thanks
> --
> Gustavo
> 

-- 
Kees Cook