Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1218441 > unrolled thread

Re: [GIT] Networking

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [GIT] Networking Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-03 18:50 +0200
    Re: [GIT] Networking David Miller <davem@davemloft.net> - 2015-09-03 19:50 +0200
      Re: [GIT] Networking Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-03 20:30 +0200
        Re: [GIT] Networking Joe Perches <joe@perches.com> - 2015-09-03 20:40 +0200
          Re: [GIT] Networking Julia Lawall <julia.lawall@lip6.fr> - 2015-09-03 21:40 +0200
            Re: [GIT] Networking Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-03 21:50 +0200
              Re: [GIT] Networking Julia Lawall <julia.lawall@lip6.fr> - 2015-09-03 23:00 +0200
                Re: [GIT] Networking Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-03 23:10 +0200
                  Re: [GIT] Networking Julia Lawall <julia.lawall@lip6.fr> - 2015-09-03 23:30 +0200
        Re: [GIT] Networking Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-03 20:40 +0200
          Re: [GIT] Networking Marcel Holtmann <marcel@holtmann.org> - 2015-09-03 23:10 +0200
            RE: [GIT] Networking David Laight <David.Laight@ACULAB.COM> - 2015-09-04 11:10 +0200
              Re: [GIT] Networking "Rustad, Mark D" <mark.d.rustad@intel.com> - 2015-09-04 19:40 +0200
        Re: [GIT] Networking David Miller <davem@davemloft.net> - 2015-09-03 20:50 +0200
    Re: [GIT] Networking Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> - 2015-09-05 18:20 +0200

#1218441 — Re: [GIT] Networking

On Wed, Sep 2, 2015 at 10:35 PM, David Miller <davem@davemloft.net> wrote:
>
> Another merge window, another set of networking changes.  I've heard
> rumblings that the lightweight tunnels infrastructure has been voted
> networking change of the year.

.. and others say that the most notable feature is the idiotic bugs
that it introduces, and the compiler even complains about.

Christ, people. Learn C, instead of just stringing random characters
together until it compiles (with warnings).

This:

  static bool rate_control_cap_mask(struct ieee80211_sub_if_data *sdata,
                                   struct ieee80211_supported_band *sband,
                                   struct ieee80211_sta *sta, u32 *mask,
                                   u8 mcs_mask[IEEE80211_HT_MCS_MASK_LEN])

is horribly broken to begin with, because array arguments in C don't
actually exist. Sadly, compilers accept it for various bad historical
reasons, and silently turn it into just a pointer argument. There are
arguments for them, but they are from weak minds.

But happily gcc has a really really valid warning (kudos - I often end
up ragging on the bad warnings gcc has, but this one is a keeper),
because a few lines down the mistake then turns into pure and utter
garbage.

It's garbage that was basically encouraged by the first mistake
(thinking that C allows array arguments), namely:

                  for (i = 0; i < sizeof(mcs_mask); i++)

the "sizeof(mcs_mask)" is _shit_. Since array arguments don't actually
exist in C, it is the size of the pointer, not the array. The first
mistake makes the bug look like reasonable code. Although I'd argue
that the code would actually be bad regardless, since "sizeof" is the
size in bytes, and the code actually wants the number of entries (and
we do have ARRAY_SIZE() for that).

Sure, in this case the entries are just one byte each, so it would
have *worked* had it not been for the array argument issue, but it's
misleading and the code is just fundamentally buggy and nonsensical in
two entirely different ways that fed on each other.

That line should read

                  for (i = 0; i < IEEE80211_HT_MCS_MASK_LEN; i++)

and the argument should just have been declared as the pointer it actually is.

A later patch then added onto the pile of manure by adding *another*
broken array argument, but at least that one then used the proper loop
for traversal of that array.

The fact that I notice this bug from a very basic "let's just compile
each pull request and make sure it isn't complete crap" is sad.

Now, it *looks* like the code was just moved, and the "sizeof()" was
initially correct (because it was a size of an actual array). Well, it
was "correct" in the sense that it generated the right code, even if
the whole confusion between "number of entries" and "size in bytes"
was still there. Then it got moved and turned from "confused but
happens to generate correct code" into "buggy pile of bovine manure".
See commit 90c66bd2232a ("mac80211: remove ieee80211_tx_rate
dependency in rate mask code").

So I can see how this bug happened, and I am only slightly upset with
Lorenzo who is the author of that commit.

What I can't see is why the code has existed in at least two
maintainer trees (Johannes' and David's) for a couple of weeks, and
nobody cared about the new compiler warnings? And it was sent to me
despite that new warning?

I realy want people to take a really hard look at functions that use
arrays as arguments. It really is very misleading, even if it can look
"prettier", and some people will argue that it's "documentation" about
how the pointer is a particular size. But it's neither. It's basically
just lying about what is going on, and the only thing it documents is
"I don't know how to C". Misleading documentation isn't documentation,
it's a mistake.

I see it in that file for at least the functions rate_idx_match_mask()
and rate_control_cap_mask(). I tried - and failed - to come up with a
reasonable grep pattern to try to see how common it is, and I'm too
lazy to add some sparse check for it.

Please people. When I see these kinds of obviously bogus code
problems, that just makes me very upset. Because it makes me worry
about all the non-obvious stuff that I miss.  Sadly, this time I had
pushed out the merge early (because I wanted to test the wireless
changes on my laptop), so now the bug is out there.

I'm not sure what the practical *impact* of the bug is. Yes, it only
traverses four or eight rate entries (depending on 32-bit or
64-bitness of the kernel) out of the ten that it should. But maybe in
practice one of the first entries are always good enough matches. So
maybe _testing_ doesn't actually show this bug, but I sure wish people
just took compiler warnings more seriously (and were a lot more
careful about moving things to functions, and never ever used the
"function argument is an array" model).

               Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1218468

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 3 Sep 2015 09:45:44 -0700

> But happily gcc has a really really valid warning (kudos - I often end
> up ragging on the bad warnings gcc has, but this one is a keeper),
> because a few lines down the mistake then turns into pure and utter
> garbage.

I really wish my GCC had emitted a warning for this, I'm on 4.9.2 here:

[davem@localhost linux]$ make net/mac80211/rate.o 
  CHK     include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CHK     include/generated/bounds.h
  CHK     include/generated/timeconst.h
  CHK     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CC [M]  net/mac80211/rate.o
[davem@localhost linux]$

Linus, what GCC version are you using and what does the warning look
like?

Anyways, Johannes please get this fixed, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218500

On Thu, Sep 3, 2015 at 10:40 AM, David Miller <davem@davemloft.net> wrote:
>
> Linus, what GCC version are you using and what does the warning look
> like?

I'm on whatever is in F22. gcc -v says

   gcc version 5.1.1 20150618 (Red Hat 5.1.1-4) (GCC)

and the warning looks like so:

  net/mac80211/rate.c: In function ‘rate_control_cap_mask’:
  net/mac80211/rate.c:719:25: warning: ‘sizeof’ on array function
parameter ‘mcs_mask’ will return size of ‘u8 * {aka unsigned char *}’
[-Wsizeof-array-argument]
     for (i = 0; i < sizeof(mcs_mask); i++)
                           ^

(note the lack of warning about the use of an array in the function
definition parameter list - I tried to find if there's any way to
enable such a warning, but couldn't find anything. Maybe my google-fu
is weak, but more probably that just doesn't exist).

                      Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218503

On Thu, 2015-09-03 at 11:22 -0700, Linus Torvalds wrote:
> On Thu, Sep 3, 2015 at 10:40 AM, David Miller <davem@davemloft.net> wrote:
> >
> > Linus, what GCC version are you using and what does the warning look
> > like?
> 
> I'm on whatever is in F22. gcc -v says
> 
>    gcc version 5.1.1 20150618 (Red Hat 5.1.1-4) (GCC)
> 
> and the warning looks like so:
> 
>   net/mac80211/rate.c: In function ‘rate_control_cap_mask’:
>   net/mac80211/rate.c:719:25: warning: ‘sizeof’ on array function
> parameter ‘mcs_mask’ will return size of ‘u8 * {aka unsigned char *}’
> [-Wsizeof-array-argument]
>      for (i = 0; i < sizeof(mcs_mask); i++)
>                            ^
> 
> (note the lack of warning about the use of an array in the function
> definition parameter list - I tried to find if there's any way to
> enable such a warning, but couldn't find anything. Maybe my google-fu
> is weak, but more probably that just doesn't exist).

Coccinelle might be a better tool for this but
a possible checkpatch patch is below:

It produces output like:

$ ./scripts/checkpatch.pl -f net/iucv/iucv.c --types=sized_array_argument
WARNING: Avoid sized array arguments
#716: FILE: net/iucv/iucv.c:716:
+static int iucv_sever_pathid(u16 pathid, u8 userdata[16])
+{

WARNING: Avoid sized array arguments
#878: FILE: net/iucv/iucv.c:878:
+int iucv_path_accept(struct iucv_path *path, struct iucv_handler *handler,
+		     u8 userdata[16], void *private)
+{

WARNING: Avoid sized array arguments
#925: FILE: net/iucv/iucv.c:925:
+int iucv_path_connect(struct iucv_path *path, struct iucv_handler *handler,
+		      u8 userid[8], u8 system[8], u8 userdata[16],
+		      void *private)
+{

WARNING: Avoid sized array arguments
#988: FILE: net/iucv/iucv.c:988:
+int iucv_path_quiesce(struct iucv_path *path, u8 userdata[16])
+{

WARNING: Avoid sized array arguments
#1020: FILE: net/iucv/iucv.c:1020:
+int iucv_path_resume(struct iucv_path *path, u8 userdata[16])
+{

WARNING: Avoid sized array arguments
#1050: FILE: net/iucv/iucv.c:1050:
+int iucv_path_sever(struct iucv_path *path, u8 userdata[16])
+{

total: 0 errors, 6 warnings, 0 checks, 2119 lines checked
---
 scripts/checkpatch.pl | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e14dcdb..747b164 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5422,6 +5422,24 @@ sub process {
 			     "externs should be avoided in .c files\n" .  $herecurr);
 		}
 
+# check for function arguments using arg[SIZE]
+		if ($^V && $^V ge 5.10.0 &&
+		    defined $stat &&
+		    $stat =~ /^.\s*(?:$Declare|$DeclareMisordered)\s*$Ident\s*($balanced_parens)\s*\{/s) {
+			my $func_args = $1;
+			if ($func_args =~ /(.*)\[\s*(?:$Constant|[A-Z0-9_]+)\s*\]/ && (!defined($1) || $1 !~ /\[\s*\]\s*$/)) {
+				my $ctx = '';
+				my $herectx = $here . "\n";
+				my $cnt = statement_rawlines($stat);
+				for (my $n = 0; $n < $cnt; $n++) {
+					$herectx .= raw_line($linenr, $n) . "\n";
+					$n = $cnt if ($herectx =~ /{/);
+				}
+				WARN("SIZED_ARRAY_ARGUMENT",
+				     "Avoid sized array arguments\n" . $herectx);
+			}
+		}
+
 # checks for new __setup's
 		if ($rawline =~ /\b__setup\("([^"]*)"/) {
 			my $name = $1;


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218537

[Multipart message — attachments visible in raw view] — view raw

On Thu, 3 Sep 2015, Joe Perches wrote:

> On Thu, 2015-09-03 at 11:22 -0700, Linus Torvalds wrote:
> > On Thu, Sep 3, 2015 at 10:40 AM, David Miller <davem@davemloft.net> wrote:
> > >
> > > Linus, what GCC version are you using and what does the warning look
> > > like?
> > 
> > I'm on whatever is in F22. gcc -v says
> > 
> >    gcc version 5.1.1 20150618 (Red Hat 5.1.1-4) (GCC)
> > 
> > and the warning looks like so:
> > 
> >   net/mac80211/rate.c: In function ‘rate_control_cap_mask’:
> >   net/mac80211/rate.c:719:25: warning: ‘sizeof’ on array function
> > parameter ‘mcs_mask’ will return size of ‘u8 * {aka unsigned char *}’
> > [-Wsizeof-array-argument]
> >      for (i = 0; i < sizeof(mcs_mask); i++)
> >                            ^
> > 
> > (note the lack of warning about the use of an array in the function
> > definition parameter list - I tried to find if there's any way to
> > enable such a warning, but couldn't find anything. Maybe my google-fu
> > is weak, but more probably that just doesn't exist).

I find 518 occurrences of a function parameter declaration that contains 
an explicit size.  But only the sizeof(mcs_mask) where there is a sizeof 
on such a parameter.  I also checked for ARRAY_SIZE on such parameters, 
and didn't find any occurrences of that either.

julia


> Coccinelle might be a better tool for this but
> a possible checkpatch patch is below:
> 
> It produces output like:
> 
> $ ./scripts/checkpatch.pl -f net/iucv/iucv.c --types=sized_array_argument
> WARNING: Avoid sized array arguments
> #716: FILE: net/iucv/iucv.c:716:
> +static int iucv_sever_pathid(u16 pathid, u8 userdata[16])
> +{
> 
> WARNING: Avoid sized array arguments
> #878: FILE: net/iucv/iucv.c:878:
> +int iucv_path_accept(struct iucv_path *path, struct iucv_handler *handler,
> +		     u8 userdata[16], void *private)
> +{
> 
> WARNING: Avoid sized array arguments
> #925: FILE: net/iucv/iucv.c:925:
> +int iucv_path_connect(struct iucv_path *path, struct iucv_handler *handler,
> +		      u8 userid[8], u8 system[8], u8 userdata[16],
> +		      void *private)
> +{
> 
> WARNING: Avoid sized array arguments
> #988: FILE: net/iucv/iucv.c:988:
> +int iucv_path_quiesce(struct iucv_path *path, u8 userdata[16])
> +{
> 
> WARNING: Avoid sized array arguments
> #1020: FILE: net/iucv/iucv.c:1020:
> +int iucv_path_resume(struct iucv_path *path, u8 userdata[16])
> +{
> 
> WARNING: Avoid sized array arguments
> #1050: FILE: net/iucv/iucv.c:1050:
> +int iucv_path_sever(struct iucv_path *path, u8 userdata[16])
> +{
> 
> total: 0 errors, 6 warnings, 0 checks, 2119 lines checked
> ---
>  scripts/checkpatch.pl | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index e14dcdb..747b164 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5422,6 +5422,24 @@ sub process {
>  			     "externs should be avoided in .c files\n" .  $herecurr);
>  		}
>  
> +# check for function arguments using arg[SIZE]
> +		if ($^V && $^V ge 5.10.0 &&
> +		    defined $stat &&
> +		    $stat =~ /^.\s*(?:$Declare|$DeclareMisordered)\s*$Ident\s*($balanced_parens)\s*\{/s) {
> +			my $func_args = $1;
> +			if ($func_args =~ /(.*)\[\s*(?:$Constant|[A-Z0-9_]+)\s*\]/ && (!defined($1) || $1 !~ /\[\s*\]\s*$/)) {
> +				my $ctx = '';
> +				my $herectx = $here . "\n";
> +				my $cnt = statement_rawlines($stat);
> +				for (my $n = 0; $n < $cnt; $n++) {
> +					$herectx .= raw_line($linenr, $n) . "\n";
> +					$n = $cnt if ($herectx =~ /{/);
> +				}
> +				WARN("SIZED_ARRAY_ARGUMENT",
> +				     "Avoid sized array arguments\n" . $herectx);
> +			}
> +		}
> +
>  # checks for new __setup's
>  		if ($rawline =~ /\b__setup\("([^"]*)"/) {
>  			my $name = $1;
> 
> 
> 

[toc] | [prev] | [next] | [standalone]

#1218538

On Thu, Sep 3, 2015 at 12:32 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
> I find 518 occurrences of a function parameter declaration that contains
> an explicit size.  But only the sizeof(mcs_mask) where there is a sizeof
> on such a parameter.  I also checked for ARRAY_SIZE on such parameters,
> and didn't find any occurrences of that either.

Are there any cases of multi-dimensional arrays? Because those
actually have semantic meaning outside of sizeof(), just in things
like adding offsets.

Eg something like

     int fn(int a[][10])

ends up being equivalent to something like

     int fn(int (*a)[10])

and "a+1" is actually 40 bytes ahead of "a", so it does *not* act like
an "int *".

(And I might have screwed that up mightily - C multidimensional arrays
and the conversions to pointers are really easy to get confused about.
Which is why I hope we don't have them)

                  Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218571

On Thu, 3 Sep 2015, Linus Torvalds wrote:

> On Thu, Sep 3, 2015 at 12:32 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> > I find 518 occurrences of a function parameter declaration that contains
> > an explicit size.  But only the sizeof(mcs_mask) where there is a sizeof
> > on such a parameter.  I also checked for ARRAY_SIZE on such parameters,
> > and didn't find any occurrences of that either.
> 
> Are there any cases of multi-dimensional arrays? Because those
> actually have semantic meaning outside of sizeof(), just in things
> like adding offsets.
> 
> Eg something like
> 
>      int fn(int a[][10])
> 
> ends up being equivalent to something like
> 
>      int fn(int (*a)[10])
> 
> and "a+1" is actually 40 bytes ahead of "a", so it does *not* act like
> an "int *".
> 
> (And I might have screwed that up mightily - C multidimensional arrays
> and the conversions to pointers are really easy to get confused about.
> Which is why I hope we don't have them)

There are 32 2-dimensional arrays in function parameters, and 1 
3-dimensional array.  No 4-dimensional arrays.  I didn't check past that.  
None of these has a sizeof or ARRAY_SIZE.

The three dimensional array is here: drivers/media/dvb-frontends/stv0367.c

static int stv0367ter_filt_coeff_init(struct stv0367_state *state,
                                u16 CellsCoeffs[3][6][5], u32 DemodXtal)

It is used as follows:

                       stv0367_writereg(state,
                                (R367TER_IIRCX_COEFF1_MSB + 2 * (j - 1)),
                                MSB(CellsCoeffs[k][i-1][j-1]));
                        stv0367_writereg(state,
                                (R367TER_IIRCX_COEFF1_LSB + 2 * (j - 1)),
                                LSB(CellsCoeffs[k][i-1][j-1]));

The value of this parameter is one of three locally defined static global 
arrays.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218579

On Thu, Sep 3, 2015 at 1:55 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
> There are 32 2-dimensional arrays in function parameters, and 1
> 3-dimensional array.  No 4-dimensional arrays.  I didn't check past that.
> None of these has a sizeof or ARRAY_SIZE.
>
> The three dimensional array is here: drivers/media/dvb-frontends/stv0367.c

Ok. That actually looks like a valid use of the C function argument
array passing semantics. It's rather much simpler than exposing the
pointers.

So I guess we don't really end up wanting to disallow this, and the
new gcc array sizeof warning is good enough.

Thanks for running the analysis so that I didn't have to look at it ;)

                       Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218585

On Thu, 3 Sep 2015, Linus Torvalds wrote:

> On Thu, Sep 3, 2015 at 1:55 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> > There are 32 2-dimensional arrays in function parameters, and 1
> > 3-dimensional array.  No 4-dimensional arrays.  I didn't check past that.
> > None of these has a sizeof or ARRAY_SIZE.
> >
> > The three dimensional array is here: drivers/media/dvb-frontends/stv0367.c
> 
> Ok. That actually looks like a valid use of the C function argument
> array passing semantics. It's rather much simpler than exposing the
> pointers.
> 
> So I guess we don't really end up wanting to disallow this, and the
> new gcc array sizeof warning is good enough.
> 
> Thanks for running the analysis so that I didn't have to look at it ;)

The double arrays also look OK - the uses are also explicit double array 
references.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218505

On Thu, Sep 3, 2015 at 11:22 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> [-Wsizeof-array-argument]

Ahh. Google shows that it's an old clang warning that gcc has recently
picked up.

But even clang doesn't seem to have any way for a project to say
"please warn about arrays in function argument declaration". It *is*
very traditional idiomatic C, it's just that I personally think it's
one of those bad traditional C things exactly because it's so
misleading about what actually goes on. But I guess that in practice,
the only thing that it actually *affects* is "sizeof" (and assignment
to the variable name - something that would be invalid for a real
array, but works on argument arrays because they are really just
pointers).

The "array as function argument" syntax is occasionally useful
(particularly for the multi-dimensional array case), so I very much
understand why it exists, I just think that in the kernel we'd be
better off with the rule that it's against our coding practices.

                  Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218576

Hi Linus,

>> [-Wsizeof-array-argument]
> 
> Ahh. Google shows that it's an old clang warning that gcc has recently
> picked up.
> 
> But even clang doesn't seem to have any way for a project to say
> "please warn about arrays in function argument declaration". It *is*
> very traditional idiomatic C, it's just that I personally think it's
> one of those bad traditional C things exactly because it's so
> misleading about what actually goes on. But I guess that in practice,
> the only thing that it actually *affects* is "sizeof" (and assignment
> to the variable name - something that would be invalid for a real
> array, but works on argument arrays because they are really just
> pointers).
> 
> The "array as function argument" syntax is occasionally useful
> (particularly for the multi-dimensional array case), so I very much
> understand why it exists, I just think that in the kernel we'd be
> better off with the rule that it's against our coding practices.

I find them useful as syntactic sugar. We have not used them a lot, but there are cases in our crypto handling code where we have fixed size array inputs/outputs and there we opted to use them. They make it easy to remember what the expected sizes of input and output are without having to read through the implementation (of course we never even tried to use sizeof on these pointers).

static int smp_ah(struct crypto_blkcipher *tfm, const u8 irk[16],                
                  const u8 r[3], u8 res[3])

This is one of the simple crypto hashing for privacy keys we have.

	r' = padding || r
	ah(h, r) = e(k, r') mod 2^24

We are fully aware that const u8 r[3] is const u8 *r. As I said, it is syntactic sugar for us and nothing more.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218785

> I find them useful as syntactic sugar. We have not used them a lot, but there are cases in our crypto
> handling code where we have fixed size array inputs/outputs and there we opted to use them. They make
> it easy to remember what the expected sizes of input and output are without having to read through the
> implementation (of course we never even tried to use sizeof on these pointers).
> 
> static int smp_ah(struct crypto_blkcipher *tfm, const u8 irk[16],
>                   const u8 r[3], u8 res[3])

Expect that it looks like you are passing arrays by value,
but instead you are passing by reference.

Explicitly pass by reference and sizeof works.
The object code will be the same.

	David


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219223

[Multipart message — attachments visible in raw view] — view raw

> On Sep 4, 2015, at 2:07 AM, David Laight <David.Laight@ACULAB.COM> wrote:
> 
>> I find them useful as syntactic sugar. We have not used them a lot, but there are cases in our crypto
>> handling code where we have fixed size array inputs/outputs and there we opted to use them. They make
>> it easy to remember what the expected sizes of input and output are without having to read through the
>> implementation (of course we never even tried to use sizeof on these pointers).
>> 
>> static int smp_ah(struct crypto_blkcipher *tfm, const u8 irk[16],
>>                  const u8 r[3], u8 res[3])
> 
> Expect that it looks like you are passing arrays by value,
> but instead you are passing by reference.
> 
> Explicitly pass by reference and sizeof works.

It depends on what you mean by works. It at least doesn't look so misleading when passing by reference and so works more as expected. The sizeof in either case will never return the size of the array. To have sizeof return the size of the array would require a typedef of the array to pass by reference. In some cases that could be the right thing to do.

--
Mark Rustad, Networking Division, Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1218515

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Thu, 3 Sep 2015 11:22:10 -0700

> (note the lack of warning about the use of an array in the function
> definition parameter list - I tried to find if there's any way to
> enable such a warning, but couldn't find anything. Maybe my google-fu
> is weak, but more probably that just doesn't exist).

I would love to see such a warning if it doesn't exist.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1219576

Hi all,

> On Wed, Sep 2, 2015 at 10:35 PM, David Miller <davem@davemloft.net> wrote:
>>
>> Another merge window, another set of networking changes.  I've heard
>> rumblings that the lightweight tunnels infrastructure has been voted
>> networking change of the year.
>
> .. and others say that the most notable feature is the idiotic bugs
> that it introduces, and the compiler even complains about.
>
> Christ, people. Learn C, instead of just stringing random characters
> together until it compiles (with warnings).
>
> This:
>
>   static bool rate_control_cap_mask(struct ieee80211_sub_if_data *sdata,
>                                    struct ieee80211_supported_band *sband,
>                                    struct ieee80211_sta *sta, u32 *mask,
>                                    u8 mcs_mask[IEEE80211_HT_MCS_MASK_LEN])
>
> is horribly broken to begin with, because array arguments in C don't
> actually exist. Sadly, compilers accept it for various bad historical
> reasons, and silently turn it into just a pointer argument. There are
> arguments for them, but they are from weak minds.
>
> But happily gcc has a really really valid warning (kudos - I often end
> up ragging on the bad warnings gcc has, but this one is a keeper),
> because a few lines down the mistake then turns into pure and utter
> garbage.
>
> It's garbage that was basically encouraged by the first mistake
> (thinking that C allows array arguments), namely:
>
>                   for (i = 0; i < sizeof(mcs_mask); i++)
>

I moved rate_control_apply_mask() logic in rate_control_cap_mask() in
order to be applied in multiple code points (i.e.
 rate_control_apply_mask_ratetbl()). Since I was using gcc version
4.9.2, the warning did not show up. Sorry for that bug.

> the "sizeof(mcs_mask)" is _shit_. Since array arguments don't actually
> exist in C, it is the size of the pointer, not the array. The first
> mistake makes the bug look like reasonable code. Although I'd argue
> that the code would actually be bad regardless, since "sizeof" is the
> size in bytes, and the code actually wants the number of entries (and
> we do have ARRAY_SIZE() for that).
>
> Sure, in this case the entries are just one byte each, so it would
> have *worked* had it not been for the array argument issue, but it's
> misleading and the code is just fundamentally buggy and nonsensical in
> two entirely different ways that fed on each other.
>
> That line should read
>
>                   for (i = 0; i < IEEE80211_HT_MCS_MASK_LEN; i++)
>
> and the argument should just have been declared as the pointer it actually is.
>
> A later patch then added onto the pile of manure by adding *another*
> broken array argument, but at least that one then used the proper loop
> for traversal of that array.
>
> The fact that I notice this bug from a very basic "let's just compile
> each pull request and make sure it isn't complete crap" is sad.
>
> Now, it *looks* like the code was just moved, and the "sizeof()" was
> initially correct (because it was a size of an actual array). Well, it
> was "correct" in the sense that it generated the right code, even if
> the whole confusion between "number of entries" and "size in bytes"
> was still there. Then it got moved and turned from "confused but
> happens to generate correct code" into "buggy pile of bovine manure".
> See commit 90c66bd2232a ("mac80211: remove ieee80211_tx_rate
> dependency in rate mask code").
>
> So I can see how this bug happened, and I am only slightly upset with
> Lorenzo who is the author of that commit.
>
> What I can't see is why the code has existed in at least two
> maintainer trees (Johannes' and David's) for a couple of weeks, and
> nobody cared about the new compiler warnings? And it was sent to me
> despite that new warning?
>
> I realy want people to take a really hard look at functions that use
> arrays as arguments. It really is very misleading, even if it can look
> "prettier", and some people will argue that it's "documentation" about
> how the pointer is a particular size. But it's neither. It's basically
> just lying about what is going on, and the only thing it documents is
> "I don't know how to C". Misleading documentation isn't documentation,
> it's a mistake.
>
> I see it in that file for at least the functions rate_idx_match_mask()
> and rate_control_cap_mask(). I tried - and failed - to come up with a
> reasonable grep pattern to try to see how common it is, and I'm too
> lazy to add some sparse check for it.
>
> Please people. When I see these kinds of obviously bogus code
> problems, that just makes me very upset. Because it makes me worry
> about all the non-obvious stuff that I miss.  Sadly, this time I had
> pushed out the merge early (because I wanted to test the wireless
> changes on my laptop), so now the bug is out there.
>
> I'm not sure what the practical *impact* of the bug is. Yes, it only
> traverses four or eight rate entries (depending on 32-bit or
> 64-bitness of the kernel) out of the ten that it should. But maybe in
> practice one of the first entries are always good enough matches. So
> maybe _testing_ doesn't actually show this bug, but I sure wish people
> just took compiler warnings more seriously (and were a lot more
> careful about moving things to functions, and never ever used the
> "function argument is an array" model).
>
>                Linus

Best regards,
Lorenzo

-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web