Re: + net-ipv4-tcpc-fix-warning.patch added to -mm tree

Re: + net-ipv4-tcpc-fix-warning.patch added to -mm tree

[William Allen Simpson]

akpm@linux-foundation.org wrote:
> The patch titled
>      net/ipv4/tcp.c: fix warning
> has been added to the -mm tree.  Its filename is
>      net-ipv4-tcpc-fix-warning.patch
> 
> Before you just go and hit "reply", please:
>    a) Consider who else should be cc'ed
>    b) Prefer to cc a suitable mailing list as well
>    c) Ideally: find the original patch on the mailing list and do a
>       reply-to-all to that, adding suitable additional cc's
> 
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
> 
> See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
> out what to do about this
> 
> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
> 
> ------------------------------------------------------
> Subject: net/ipv4/tcp.c: fix warning
> From: Andrew Morton <akpm@linux-foundation.org>
> 
> x86_64 allmodconfig:
> 
> net/ipv4/tcp.c: In function 'do_tcp_getsockopt':
> net/ipv4/tcp.c:2544: warning: comparison is always false due to limited range of data type
> 
> sizeof(tcpct_value) is 536, which can never be less than a value which is held
> in a u8.
> 
> This fix is stupid - just delete the code?
> 
I didn't have this warning, how in the world do you get it?

This is what David calls my "anal" coding style.  I check everything,
because someday somebody else might make a change, and it's important
that such problems be detected.  In this case, I assume the compiler
will compile them out or it would have no effect, and commented that
it should be impossible.

Your test should probably be TCP_MSS_DEFAULT < TCP_COOKIE_PAIR_SIZE


> btw, what does "return as nonce" try to mean?
> 
The internet-draft specifies returning a nonce.


> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Acked-by: William Allen Simpson <william.allen.simpson@gmail.com>

> ---
> 
>  net/ipv4/tcp.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff -puN net/ipv4/tcp.c~net-ipv4-tcpc-fix-warning net/ipv4/tcp.c
> --- a/net/ipv4/tcp.c~net-ipv4-tcpc-fix-warning
> +++ a/net/ipv4/tcp.c
> @@ -2540,11 +2540,13 @@ static int do_tcp_getsockopt(struct sock
>  			ctd.tcpct_cookie_desired = cvp->cookie_desired;
>  			ctd.tcpct_s_data_desired = cvp->s_data_desired;
>  
> +#if TCP_MSS_DEFAULT < 256
>  			/* Cookie(s) saved, return as nonce */
>  			if (sizeof(ctd.tcpct_value) < cvp->cookie_pair_size) {
>  				/* impossible? */
>  				return -EINVAL;
>  			}
> +#endif
>  			memcpy(&ctd.tcpct_value[0], &cvp->cookie_pair[0],
>  			       cvp->cookie_pair_size);
>  			ctd.tcpct_used = cvp->cookie_pair_size;
> _

Re: + net-ipv4-tcpc-fix-warning.patch added to -mm tree

[Andrew Morton]

On Tue, 08 Dec 2009 20:45:53 -0500
William Allen Simpson <william.allen.simpson@gmail.com> wrote:

> > ------------------------------------------------------
> > Subject: net/ipv4/tcp.c: fix warning
> > From: Andrew Morton <akpm@linux-foundation.org>
> > 
> > x86_64 allmodconfig:
> > 
> > net/ipv4/tcp.c: In function 'do_tcp_getsockopt':
> > net/ipv4/tcp.c:2544: warning: comparison is always false due to limited range of data type
> > 
> > sizeof(tcpct_value) is 536, which can never be less than a value which is held
> > in a u8.
> > 
> > This fix is stupid - just delete the code?
> > 
> I didn't have this warning, how in the world do you get it?

Used gcc-4.0.2.  They re-randomise the warnings with each release I think.

> This is what David calls my "anal" coding style.  I check everything,
> because someday somebody else might make a change, and it's important
> that such problems be detected.  In this case, I assume the compiler
> will compile them out or it would have no effect, and commented that
> it should be impossible.
> 
> Your test should probably be TCP_MSS_DEFAULT < TCP_COOKIE_PAIR_SIZE

I'd say just delete it ;)

Re: + net-ipv4-tcpc-fix-warning.patch added to -mm tree

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Tue, 8 Dec 2009 18:08:01 -0800

> I'd say just delete it ;)

That's how I'll handle this, thanks Andrew.

Re: + net-ipv4-tcpc-fix-warning.patch added to -mm tree

[William Allen Simpson]

Andrew Morton wrote:
> On Tue, 08 Dec 2009 20:45:53 -0500
> William Allen Simpson <william.allen.simpson@gmail.com> wrote:
>> I didn't have this warning, how in the world do you get it?
> 
> Used gcc-4.0.2.  They re-randomise the warnings with each release I think.
> 
Using gcc-4.4.1 here, and the usual 'make -s -j4 vmlinux'.


>> This is what David calls my "anal" coding style.  I check everything,
>> because someday somebody else might make a change, and it's important
>> that such problems be detected.  In this case, I assume the compiler
>> will compile them out or it would have no effect, and commented that
>> it should be impossible.
>>
>> Your test should probably be TCP_MSS_DEFAULT < TCP_COOKIE_PAIR_SIZE
> 
> I'd say just delete it ;)
> 
Well, David said he would, but nothing seems to have happened, so I'll
send a short patch in my next message.

Re: [PATCH] net-ipv4-tcp.c-fix-warning-from-older-compilers.patch

[William Allen Simpson]

[-- Attachment #1: Type: text/plain, Size: 327 bytes --]

Replace + net-ipv4-tcpc-fix-warning.patch added to -mm tree

Remove the offending code, and replace with comments.  This is an
unlikely problem, so depend on human finding any future error.

Signed-off-by: William.Allen.Simpson@gmail.com
---
  net/ipv4/tcp.c |    9 ++++-----
  1 files changed, 4 insertions(+), 5 deletions(-)

[-- Attachment #2: net-ipv4-tcp.c-fix-warning-from-older-compilers.patch --]
[-- Type: text/plain, Size: 761 bytes --]

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index c8666b7..3782c17 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2540,11 +2540,10 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
 			ctd.tcpct_cookie_desired = cvp->cookie_desired;
 			ctd.tcpct_s_data_desired = cvp->s_data_desired;
 
-			/* Cookie(s) saved, return as nonce */
-			if (sizeof(ctd.tcpct_value) < cvp->cookie_pair_size) {
-				/* impossible? */
-				return -EINVAL;
-			}
+			/* Cookie(s) saved, return as nonce.
+			 * Assumes TCP_MSS_DEFAULT > TCP_COOKIE_PAIR_SIZE,
+			 * sizeof(ctd.tcpct_value) > cvp->cookie_pair_size
+			 */
 			memcpy(&ctd.tcpct_value[0], &cvp->cookie_pair[0],
 			       cvp->cookie_pair_size);
 			ctd.tcpct_used = cvp->cookie_pair_size;
-- 
1.6.3.3

Re: [PATCH] net-ipv4-tcp.c-fix-warning-from-older-compilers.patch

[Ben Hutchings]

On Thu, 2009-12-10 at 09:16 -0500, William Allen Simpson wrote:
> Replace + net-ipv4-tcpc-fix-warning.patch added to -mm tree
> 
> Remove the offending code, and replace with comments.  This is an
> unlikely problem, so depend on human finding any future error.
> 
> Signed-off-by: William.Allen.Simpson@gmail.com
> ---
>   net/ipv4/tcp.c |    9 ++++-----
>   1 files changed, 4 insertions(+), 5 deletions(-)
> plain text document attachment
> (net-ipv4-tcp.c-fix-warning-from-older-compilers.patch)
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index c8666b7..3782c17 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2540,11 +2540,10 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
>  			ctd.tcpct_cookie_desired = cvp->cookie_desired;
>  			ctd.tcpct_s_data_desired = cvp->s_data_desired;
>  
> -			/* Cookie(s) saved, return as nonce */
> -			if (sizeof(ctd.tcpct_value) < cvp->cookie_pair_size) {
> -				/* impossible? */
> -				return -EINVAL;
> -			}
> +			/* Cookie(s) saved, return as nonce.
> +			 * Assumes TCP_MSS_DEFAULT > TCP_COOKIE_PAIR_SIZE,
[...]

You can use BUILD_BUG_ON() to ensure that remains true.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] net-ipv4-tcp.c-fix-warning-from-older-compilers.patch

[Eric Dumazet]

Le 10/12/2009 15:16, William Allen Simpson a écrit :
> Replace + net-ipv4-tcpc-fix-warning.patch added to -mm tree
> 
> Remove the offending code, and replace with comments.  This is an
> unlikely problem, so depend on human finding any future error.
> 
> Signed-off-by: William.Allen.Simpson@gmail.com
> ---
>  net/ipv4/tcp.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)

This is not necessary, David already _did_ the thing, and _told_ so.


You probably missed fact we are in merge window, so fixes (and only fixes)
are pushed into net-2.6 tree.

When Linus closes this merge window (issuing a linux-2.6.33-rc1 tag),
then David re-opens net-next-2.6 tree for new stuff.

http://git2.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commitdiff;h=3dc789320e1b310cb505dcd94512c279abcd5e1c

commit 3dc789320e1b310cb505dcd94512c279abcd5e1c
Author: David S. Miller <davem@davemloft.net>
Date:   Tue Dec 8 20:07:54 2009 -0800

    tcp: Remove runtime check that can never be true.

    GCC even warns about it, as reported by Andrew Morton:

    net/ipv4/tcp.c: In function 'do_tcp_getsockopt':
    net/ipv4/tcp.c:2544: warning: comparison is always false due to limited range of data type

    Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] net-ipv4-tcp.c-fix-warning-from-older-compilers.patch

[David Miller]

This has already been taken care of in net-2.6 for more than a day.

Re: [PATCH] net-ipv4-tcp.c-fix-warning-from-older-compilers.patch

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 10 Dec 2009 16:31:30 +0100

> Le 10/12/2009 15:16, William Allen Simpson a ^[$(D+1^[(Bcrit :
>> Replace + net-ipv4-tcpc-fix-warning.patch added to -mm tree
>> 
>> Remove the offending code, and replace with comments.  This is an
>> unlikely problem, so depend on human finding any future error.
>> 
>> Signed-off-by: William.Allen.Simpson@gmail.com
>> ---
>>  net/ipv4/tcp.c |    9 ++++-----
>>  1 files changed, 4 insertions(+), 5 deletions(-)
> 
> This is not necessary, David already _did_ the thing, and _told_ so.

Right.

> You probably missed fact we are in merge window, so fixes (and only fixes)
> are pushed into net-2.6 tree.

I even announced this explicitly to the list:

	http://marc.info/?l=linux-netdev&m=126031591420846&w=2

Re: [PATCH] net-ipv4-tcp.c-fix-warning-from-older-compilers.patch

[Jarek Poplawski]

>>> Replace + net-ipv4-tcpc-fix-warning.patch added to -mm tree
>>>
>>> Remove the offending code, and replace with comments.  This is an
>>> unlikely problem, so depend on human finding any future error.
>>>
>>> Signed-off-by: William.Allen.Simpson@gmail.com
>>> ---
>>>  net/ipv4/tcp.c |    9 ++++-----
>>>  1 files changed, 4 insertions(+), 5 deletions(-)
>> This is not necessary, David already _did_ the thing, and _told_ so.
> 
> Right.

But David _did_ forget to _send_ the thing to the list. ;-)

Jarek P.

Re: [PATCH] net-ipv4-tcp.c-fix-warning-from-older-compilers.patch

[William Allen Simpson]

Ben Hutchings wrote:
> You can use BUILD_BUG_ON() to ensure that remains true.
> 
Another fine undocumented Linux Kernel feature.  In the absence of
standardized functions that check for buffer overruns, that's just
the ticket!

Kinda the opposite conditional of static assert, but I'm pretty sure
static assert wasn't working in gcc until a year or so ago.  That's
why I used the more old-fashioned test instead.  I'd no idea those
triggered a warning in some versions of gcc -- I never trusted 4.0.x,
and had plenty of problems with 4.1.x, too.

Thanks, I'll resubmit.

[PATCH v2] net-ipv4-tcp.c-fix-warning-for-older-compilers.patch

[William Allen Simpson]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]

Replace + net-ipv4-tcpc-fix-warning.patch added to -mm tree

Remove the offending code, and replace with BUILD_BUG_ON().

Signed-off-by: William.Allen.Simpson@gmail.com
---
  net/ipv4/tcp.c |    5 +----
  1 files changed, 1 insertions(+), 4 deletions(-)

[-- Attachment #2: net-ipv4-tcp.c-fix-warning-for-older-compilers.patch --]
[-- Type: text/plain, Size: 615 bytes --]

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index c8666b7..3d2229c 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2541,10 +2541,7 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
 			ctd.tcpct_s_data_desired = cvp->s_data_desired;
 
 			/* Cookie(s) saved, return as nonce */
-			if (sizeof(ctd.tcpct_value) < cvp->cookie_pair_size) {
-				/* impossible? */
-				return -EINVAL;
-			}
+			BUILD_BUG_ON(sizeof(ctd.tcpct_value) < TCP_COOKIE_PAIR_SIZE);
 			memcpy(&ctd.tcpct_value[0], &cvp->cookie_pair[0],
 			       cvp->cookie_pair_size);
 			ctd.tcpct_used = cvp->cookie_pair_size;
-- 
1.6.3.3

Re: [PATCH] net-ipv4-tcp.c-fix-warning-from-older-compilers.patch

[William Allen Simpson]

Eric Dumazet wrote:
> This is not necessary, David already _did_ the thing, and _told_ so.
> 
There doesn't seem to be a patch sent to the list, at least it didn't
show up in my mail, nor is it in the archive.  Such mistakes happen.


> You probably missed fact we are in merge window, so fixes (and only fixes)
> are pushed into net-2.6 tree.
> 
That's my understanding.  I'd asked a question earlier which tree to
patch against (which he didn't answer), but a later message was more
helpful and detailed.


> When Linus closes this merge window (issuing a linux-2.6.33-rc1 tag),
> then David re-opens net-next-2.6 tree for new stuff.
> 
That's my understanding, thanks again.  (This isn't new stuff.)


> http://git2.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commitdiff;h=3dc789320e1b310cb505dcd94512c279abcd5e1c
> 
Well, I'll be darned!  There must be some list that you're on that sends
out such notifications....

Not a good patch, as it accidentally removed the section comment.  And it
fails to describe or test against buffer overruns.

So, he'll have no problems backing out his patch and adding mine.

Re: [PATCH] net-ipv4-tcp.c-fix-warning-from-older-compilers.patch

[William Allen Simpson]

Jarek Poplawski wrote:
>>> This is not necessary, David already _did_ the thing, and _told_ so.
> 
> But David _did_ forget to _send_ the thing to the list. ;-)
> 
Aha, so it wasn't a local problem.  Thanks!

Re: [PATCH] net-ipv4-tcp.c-fix-warning-from-older-compilers.patch

[Jarek Poplawski]

On Fri, Dec 11, 2009 at 10:51:21AM -0500, William Allen Simpson wrote:
> Jarek Poplawski wrote:
> >>>This is not necessary, David already _did_ the thing, and _told_ so.
> >
> >But David _did_ forget to _send_ the thing to the list. ;-)
> >
> Aha, so it wasn't a local problem.  Thanks!

It's (linux) universal:
1. Nobody (even davem) is perfect :-(
2. Except akpm ;-)

Jarek P.

Re: [PATCH] net-ipv4-tcp.c-fix-warning-from-older-compilers.patch

[David Miller]

From: William Allen Simpson <william.allen.simpson@gmail.com>
Date: Fri, 11 Dec 2009 10:49:38 -0500

> So, he'll have no problems backing out his patch and adding mine.

Guess again.

Re: [PATCH] net-ipv4-tcp.c-fix-warning-from-older-compilers.patch

[David Miller]

From: William Allen Simpson <william.allen.simpson@gmail.com>
Date: Fri, 11 Dec 2009 10:51:21 -0500

> Jarek Poplawski wrote:
>>>> This is not necessary, David already _did_ the thing, and _told_ so.
>> But David _did_ forget to _send_ the thing to the list. ;-)
>> 
> Aha, so it wasn't a local problem.  Thanks!

I didn't necessarily have to, I stated exactly in the thread with
Andrew Morton what change I was going to make in my tree to resolve
this.

If you're really interested in following development and generating
correct current patches to the project, you should follow the
appropriate GIT tree.