Change judgment len position

PATCH NAK Wang Hai 9 participants 24 messages Oct 24, 2018 – Oct 25, 2018

Subsystems (3)

Networking Network drivers TCP

lore

Share & tools

lore

Wang Hai

author

2018-10-24 15:47 UTC #0 lore raw

PATCH 1/1 Change judgment len position

To determine whether len is less than zero, it should be put before the function min_t, because the return value of min_t is not likely to be less than zero.

Signed-off-by Wang Hai <[email protected]>

---
 net/ipv4/tcp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/ipv4/tcp.c +2 -2

index 10c624639..49af9fdc3 100644
@@ -3301,11 +3301,11 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
 	struct net *net = sock_net(sk);
 	int val, len;
 
+	len = min_t(unsigned int, len, sizeof(int));
+
 	if (get_user(len, optlen))
 		return -EFAULT;
 
-	len = min_t(unsigned int, len, sizeof(int));
-
 	if (len < 0)
 		return -EINVAL;

Signature · 1 line

2.17.1

Willy Tarreau

2018-10-24 15:57 UTC #1 lore raw

Quoted 3 lines · Wang Hai

To determine whether len is less than zero, it should be put before the function min_t, because the return value of min_t is not likely to be less than zero.

Huh? First, the <0 test is made on "len", not "min_t", so it still is signed. Second, you're in fact completely removing the test here, look :

net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3305

Quoted 5 lines

net/ipv4/tcp.c +2 -0

 	struct net *net = sock_net(sk);
 	int val, len;
 
+	len = min_t(unsigned int, len, sizeof(int));
+

len is used uninitialized here, so the result is undefined.

net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3307

Quoted 2 lines

net/ipv4/tcp.c +0 -0

 	if (get_user(len, optlen))
 		return -EFAULT;

Then it gets overridden by get_user()

net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3308

Quoted 2 lines

net/ipv4/tcp.c +0 -2

-	len = min_t(unsigned int, len, sizeof(int));
-

Then its positive values are not bounded anymore since you moved the test.

net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3310

Quoted 2 lines

net/ipv4/tcp.c +0 -0

 	if (len < 0)
 		return -EINVAL;

Then only negative values are dropped. So unless I'm missing something obvious, you're just allowing len to be as large as 2GB-1 based on the user's fed optlen.

Am I wrong ?

Willy

Joe Perches

2018-10-24 16:23 UTC #2 lore raw

Quoted 20 lines · Willy Tarreau

Quoted 3 lines · Wang Hai
To determine whether len is less than zero, it should be put before the function min_t, because the return value of min_t is not likely to be less than zero.
Huh? First, the <0 test is made on "len", not "min_t", so it still is signed. Second, you're in fact completely removing the test here, look :
net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3305
Quoted 5 lines
net/ipv4/tcp.c +2 -0
 	struct net *net = sock_net(sk);
 	int val, len;
 
+	len = min_t(unsigned int, len, sizeof(int));
+
len is used uninitialized here, so the result is undefined.
net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3307
Quoted 2 lines
net/ipv4/tcp.c +0 -0
 	if (get_user(len, optlen))
 		return -EFAULT;

Quoted 6 lines

Then it gets overridden by get_user()
net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3308
Quoted 2 lines
net/ipv4/tcp.c +0 -2
-	len = min_t(unsigned int, len, sizeof(int));
-
Then its positive values are not bounded anymore since you moved the test.

Not quite.

Problem here is negative values are tested as
large positive values and limited to 4

ie:

	ien len = -1,
	len = min_t(unsigned int, len, sizeof(int));

len is now 4

Quoted 2 lines

net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3310
Quoted 2 lines
net/ipv4/tcp.c +0 -0
 	if (len < 0)
 		return -EINVAL;

So this test len < 0 could be moved up above min_t

Quoted 7 lines

Then only negative values are dropped. So unless I'm missing something obvious, you're just allowing len to be as large as 2GB-1 based on the user's fed optlen.
Am I wrong ?
Willychee

Willy Tarreau

2018-10-24 16:32 UTC #3 lore raw

Quoted 37 lines · Joe Perches

Quoted 20 lines · Willy Tarreau
Quoted 3 lines · Wang Hai
To determine whether len is less than zero, it should be put before the function min_t, because the return value of min_t is not likely to be less than zero.
Huh? First, the <0 test is made on "len", not "min_t", so it still is signed. Second, you're in fact completely removing the test here, look :
net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3305
Quoted 5 lines
net/ipv4/tcp.c +2 -0
 	struct net *net = sock_net(sk);
 	int val, len;
 
+	len = min_t(unsigned int, len, sizeof(int));
+
len is used uninitialized here, so the result is undefined.
net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3307
Quoted 2 lines
net/ipv4/tcp.c +0 -0
 	if (get_user(len, optlen))
 		return -EFAULT;
Quoted 6 lines
Then it gets overridden by get_user()
net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3308
Quoted 2 lines
net/ipv4/tcp.c +0 -2
-	len = min_t(unsigned int, len, sizeof(int));
-
Then its positive values are not bounded anymore since you moved the test.
Not quite.
Problem here is negative values are tested as
large positive values and limited to 4
ie:
	ien len = -1,
	len = min_t(unsigned int, len, sizeof(int));

Quoted 6 lines

len is now 4
Quoted 2 lines
net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3310
Quoted 2 lines
net/ipv4/tcp.c +0 -0
 	if (len < 0)
 		return -EINVAL;
So this test len < 0 could be moved up above min_t

It could indeed, or we could also have min_t() done on an int instead of an unsigned int and this would avoid the need to shuffle the code around and open a huge hole like this one.

Willy

Joe Perches

2018-10-24 16:54 UTC #4 lore raw

Quoted 40 lines · Willy Tarreau

Quoted 39 lines · Joe Perches
Quoted 20 lines · Willy Tarreau
Quoted 3 lines · Wang Hai
To determine whether len is less than zero, it should be put before the function min_t, because the return value of min_t is not likely to be less than zero.
Huh? First, the <0 test is made on "len", not "min_t", so it still is signed. Second, you're in fact completely removing the test here, look :
net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3305
Quoted 5 lines
net/ipv4/tcp.c +2 -0
 	struct net *net = sock_net(sk);
 	int val, len;
 
+	len = min_t(unsigned int, len, sizeof(int));
+
len is used uninitialized here, so the result is undefined.
net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3307
Quoted 2 lines
net/ipv4/tcp.c +0 -0
 	if (get_user(len, optlen))
 		return -EFAULT;
Quoted 6 lines
Then it gets overridden by get_user()
net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3308
Quoted 2 lines
net/ipv4/tcp.c +0 -2
-	len = min_t(unsigned int, len, sizeof(int));
-
Then its positive values are not bounded anymore since you moved the test.
Not quite.
Problem here is negative values are tested as
large positive values and limited to 4
ie:
	ien len = -1,
	len = min_t(unsigned int, len, sizeof(int));
len is now 4

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3310
Quoted 2 lines
net/ipv4/tcp.c +0 -0
 	if (len < 0)
 		return -EINVAL;
So this test len < 0 could be moved up above min_t
It could indeed, or we could also have min_t() done on an int instead of an unsigned int and this would avoid the need to shuffle the code around and open a huge hole like this one.

I think if the point is to test for negative numbers, it's clearer to do that before using min_t.and it's probably clearer not to use min_t at all.

	if (get_user(len, optlen))
		return -EFAULT;

	if (len < 0)
		return -EINVAL;

	if (len > sizeof(int))
		len = sizeof(int);

Eric Dumazet

2018-10-24 17:03 UTC #5 lore raw

Quoted 4 lines · Joe Perches

I think if the point is to test for negative numbers, it's clearer to do that before using min_t.and it's probably clearer not to use min_t at all.

...

Quoted 3 lines

if (len > sizeof(int))
len = sizeof(int);

It is a matter of taste really, I know some people (like me) sometimes mixes min() and max()

I would suggest that if someones wants to change the current code, a corresponding test
would be added in tools/testing/selftests/net

Joe Perches

2018-10-24 17:18 UTC #6 lore raw

Quoted 7 lines · Eric Dumazet

Quoted 4 lines · Joe Perches
I think if the point is to test for negative numbers, it's clearer to do that before using min_t.and it's probably clearer not to use min_t at all.

Quoted 6 lines

...
Quoted 2 lines
if (len > sizeof(int))
len = sizeof(int);
It is a matter of taste really,

Agree and hence my use of 'I think' above.

Quoted 2 lines

I know some people (like me) sometimes
mixes min() and max()

Not quite sure what you mean here by mixes. mix up? If so, the < > inversions probably have about the same error rate.

And I suppose there are cases where the always set of len in uses like

	len = min(len, 4);

are more costly (len being in a slow write speed area of memory or some such) than the other style of

	if (len < 4)
		len = 4;

I think that min() is easier to read in most cases.

Quoted 2 lines

I would suggest that if someones wants to change the current code, a corresponding test would be added in tools/testing/selftests/net?

Willy Tarreau

2018-10-24 20:09 UTC #7 lore raw

In reply to Eric Dumazet

Quoted 7 lines · Eric Dumazet

Quoted 4 lines · Joe Perches
I think if the point is to test for negative numbers, it's clearer to do that before using min_t.and it's probably clearer not to use min_t at all.

Quoted 8 lines

...
Quoted 3 lines
if (len > sizeof(int))
len = sizeof(int);
It is a matter of taste really, I know some people (like me) sometimes mixes min() and max()

I do mix them up a lot as well because I tend to read "x=min(y,4)" as "take y with a minimum value of 4" which in fact would be "max(y,4)".

Quoted 3 lines

I would suggest that if someones wants to change the current code, a corresponding test
would be added in tools/testing/selftests/net

In any case, what matters to me is that for now the only risk the existing code represents is to overwrite up to one int of some userspace if the size is negative, and we don't want that a wrong fix results in doing something worse by accident like reading 2GB of kernel memory. I agree that Joe's test with len<0 then len>sizeof(int) seems to work, but a test is probably useful at least to ensure that the next person who passes there and wants to turn this into min_t() again clearly catches all bad cases.

Regards,
Willy

Eric Dumazet

2018-10-24 15:59 UTC #8 lore raw

In reply to Wang Hai

Quoted 4 lines · Wang Hai

To determine whether len is less than zero, it should be put before the function min_t, because the return value of min_t is not likely to be less than zero.

????

I really wonder if you actually tested this patch.

Thanks.

Willy Tarreau

2018-10-24 16:01 UTC #9 lore raw

Quoted 1 line · Eric Dumazet

I really wonder if you actually tested this patch.

I'm even wondering if it's not done on purpose to retrieve up to 2 GB of kernel memory via a single getsockopt() call :-/

Willy

David Miller

maintainer

2018-10-24 17:10 UTC #10 lore raw

In reply to Wang Hai

From: Wang Hai <[email protected]>
Date: Wed, 24 Oct 2018 23:47:29 +0800

Quoted 14 lines

To determine whether len is less than zero, it should be put before the function min_t, because the return value of min_t is not likely to be less than zero.
Signed-off-by Wang Hai <[email protected]>
---
 net/ipv4/tcp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
net/ipv4/tcp.c +0 -0
index 10c624639..49af9fdc3 100644

net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3307

Quoted 7 lines

net/ipv4/tcp.c +2 -0

@@ -3301,11 +3301,11 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
 	struct net *net = sock_net(sk);
 	int val, len;
 
+	len = min_t(unsigned int, len, sizeof(int));
+
 	if (get_user(len, optlen))
 		return -EFAULT;

You can't be serious?

'len' has no value before the get_user() call.

Joe Perches

2018-10-24 18:28 UTC #11 lore raw

Quoted 6 lines · David Miller

From: Wang Hai <[email protected]>
Date: Wed, 24 Oct 2018 23:47:29 +0800
Quoted 3 lines
To determine whether len is less than zero, it should be put before the function min_t, because the return value of min_t is not likely to be less than zero.

[]

Quoted 1 line

net/ipv4/tcp.c
Quoted 1 line
net/ipv4/tcp.c +0 -0

[]

Quoted 10 lines

net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3307

Quoted 8 lines

net/ipv4/tcp.c +2 -0

@@ -3301,11 +3301,11 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
      struct net *net = sock_net(sk);
      int val, len;
 
+     len = min_t(unsigned int, len, sizeof(int));
+
      if (get_user(len, optlen))
              return -EFAULT;

You can't be serious?

I'm not personally taken aback by this but there is the new Code of
Conduct to consider.

John McEnroe earned quite a bit of his reputation as an 'enfant terrible' via a similar statement.

https://www.youtube.com/watch?v=t0hK1wyrrAU

Perhaps a different word choice next time in reply to submitters of ill-considered and/or defective patches could be useful.

Willy Tarreau

2018-10-24 20:48 UTC #12 lore raw

Quoted 29 lines · Joe Perches

Quoted 6 lines · David Miller
From: Wang Hai <[email protected]>
Date: Wed, 24 Oct 2018 23:47:29 +0800
Quoted 3 lines
To determine whether len is less than zero, it should be put before the function min_t, because the return value of min_t is not likely to be less than zero.
[]
Quoted 1 line
net/ipv4/tcp.c
Quoted 1 line
net/ipv4/tcp.c +0 -0
[]
Quoted 10 lines
net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 3307
Quoted 8 lines
net/ipv4/tcp.c +2 -0
@@ -3301,11 +3301,11 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
      struct net *net = sock_net(sk);
      int val, len;
 
+     len = min_t(unsigned int, len, sizeof(int));
+
      if (get_user(len, optlen))
              return -EFAULT;
You can't be serious?
I'm not personally taken aback by this but there is the new Code of
Conduct to consider.
John McEnroe earned quite a bit of his reputation as an 'enfant terrible' via a similar statement.

Quoted 5 lines

https://www.youtube.com/watch?v=t0hK1wyrrAU
Perhaps a different word choice next time in reply to submitters of ill-considered and/or defective patches could be useful.

Maybe but on this one I think we're really out of the scope of the CoC.

When you read this patch from an apparent first-time contributor (no trace in either LKML, netdev or even google), the level of assurance in the commit message is pretty good, showing that he's not at all a beginner, which doesn't match at all the type of error seen in the code, which doesn't even need to be compiled to see that it will emit a warning and not work as advertised. Moreover, the commit message is vague enough to seem it tries to cover the patch, and doesn't even match what's done in the patch.

When you factor in the fact that the code opens a big but very discrete vulnerability, I tend to think that in fact we're not facing a newbie at all but someone deliberately trying to inject a subtle backdoor into the kernel and disguise it as a vague bug fix, possibly even hoping that it would find its way to -stable. I would not be surprised if this e-mail address is a throw-away anonymous address created just for this occasion.

I could totally be wrong of course, but the clues are quite heavy here as I find it hard to argue for a series of beginner's mistakes. If this person really exists and can explain how we ended up there, I will of course happily retract my suspicion and apologize.

Willy

Joe Perches

NAK

2018-10-24 23:46 UTC #13 lore raw

(adding Fengguang Wu and lkp)

Quoted 6 lines · Willy Tarreau

When you read this patch from an apparent first-time contributor (no trace in either LKML, netdev or even google), the level of assurance in the commit message is pretty good, showing that he's not at all a beginner, which doesn't match at all the type of error seen in the code, which doesn't even need to be compiled to see that it will emit a warning and not work as advertised.

Which to me is more of an indication of beginner-ness.

Quoted 4 lines

When you factor in the fact that the code opens a big but very discrete vulnerability, I tend to think that in fact we're not facing a newbie at all but someone deliberately trying to inject a subtle backdoor into the kernel and disguise it as a vague bug fix,

That seems unlikely as it introduces a compilation warning.

A real intentional backdoor from a nefarious source would likely be completely correct about compilation and use the typical commit subject style.

Anyway, who know for certain right now.

I would have suggested David reply with only his second sentence and maybe a pointer to kernelnewbies or staging.

Just something like:

nack: 'len' has no value before the get_user() call.

Please try to make your first patch in drivers/staging to get familiar with submitting patches to the kernel.
https://kernelnewbies.org/FirstKernelPatch

Maybe there's utility in creating a filtering and auto-reply tool for first time patch submitters for all the vger mailing lists using some combination of previously known submitters and the 0-day robot to point those first time submitters of defective patches to kernelnewbies and staging.

cheers, Joe

David Miller

maintainer

2018-10-25 00:02 UTC #14 lore raw

From: Joe Perches <[email protected]>
Date: Wed, 24 Oct 2018 16:46:18 -0700

Quoted 2 lines

I would have suggested David reply with only his second sentence and maybe a pointer to kernelnewbies or staging.

I maintain that I was not out of line with my comment.

I sought a second opinion from Greg KH and others, and they agree with me that I was still kind with my choice of words.

I think you're taking things too far, and I will simply not stand for this overreaching judgement upon my behavior on this list which is completely not justified.

Thank you.

Joe Perches

2018-10-25 00:23 UTC #15 lore raw

Quoted 10 lines · David Miller

From: Joe Perches <[email protected]>
Date: Wed, 24 Oct 2018 16:46:18 -0700
Quoted 2 lines
I would have suggested David reply with only his second sentence and maybe a pointer to kernelnewbies or staging.is
I maintain that I was not out of line with my comment.
I sought a second opinion from Greg KH and others, and they agree with me that I was still kind with my choice of words.

"You can't be serious?" is kind?

Quoted 3 lines

I think you're taking things too far, and I will simply not stand for this overreaching judgement upon my behavior on this list which is completely not justified.

Even above here, concision is generally better.

overreaching and completely are probably not reasonable.

openness and defensiveness are somewhat antithetic.

Quoted 1 line

From the new Code of Conduct (which I don't even approve)

* Gracefully accepting constructive criticism.

cheers, Joe

Eric Dumazet

2018-10-25 00:50 UTC #16 lore raw

Quoted 2 lines · Joe Perches

"You can't be serious?" is kind?

Context maybe ? As a non native, I do not see why it is an offense.

I would like very much we discuss about patches here, not about whatever interpretation you or anyone could make from any answers.

Recipe for disaster in linux community :

1) Write a bot, sending one wrong patch every hour, with a random "From:" name and email address.

Yeah, can you believe a bot can actually 'Signed-off-by:' a patch ???

2) Hundred of emails sent, from reviewers, annoyed maintainers, and flames because the proper words for newbies were not _carefully_ chosen.

So just wait for an answer from the supposed patch author, and see what happens next.

Thank you.

Fengguang Wu

2018-10-25 01:11 UTC #17 lore raw

In reply to Joe Perches

CC Philip and LKP team.

Quoted 36 lines · Joe Perches

(adding Fengguang Wu and lkp)
Quoted 6 lines · Willy Tarreau
When you read this patch from an apparent first-time contributor (no trace in either LKML, netdev or even google), the level of assurance in the commit message is pretty good, showing that he's not at all a beginner, which doesn't match at all the type of error seen in the code, which doesn't even need to be compiled to see that it will emit a warning and not work as advertised.
Which to me is more of an indication of beginner-ness.
Quoted 4 lines
When you factor in the fact that the code opens a big but very discrete vulnerability, I tend to think that in fact we're not facing a newbie at all but someone deliberately trying to inject a subtle backdoor into the kernel and disguise it as a vague bug fix,
That seems unlikely as it introduces a compilation warning.
A real intentional backdoor from a nefarious source would likely be completely correct about compilation and use the typical commit subject style.
Anyway, who know for certain right now.
I would have suggested David reply with only his second sentence and maybe a pointer to kernelnewbies or staging.
Just something like:
nack: 'len' has no value before the get_user() call.
Please try to make your first patch in drivers/staging to get familiar with submitting patches to the kernel.
https://kernelnewbies.org/FirstKernelPatch

Quoted 5 lines

Maybe there's utility in creating a filtering and auto-reply tool for first time patch submitters for all the vger mailing lists using some combination of previously known submitters and the 0-day robot to point those first time submitters of defective patches to kernelnewbies and staging.

Yeah good idea. That feature can be broken into 2 parts:

- an email script, which could be added to Linux scripts/ dir - maintain records for telling whether someone is first-time patch submitters

Thanks,
Fengguang

Joe Perches

2018-10-25 01:16 UTC #18 lore raw

Quoted 11 lines · Fengguang Wu

CC Philip and LKP team.
Quoted 9 lines
Please try to make your first patch in drivers/staging to get familiar with submitting patches to the kernel.
https://kernelnewbies.org/FirstKernelPatch
Maybe there's utility in creating a filtering and auto-reply tool for first time patch submitters for all the vger mailing lists using some combination of previously known submitters and the 0-day robot to point those first time submitters of defective patches to kernelnewbies and staging.

Quoted 4 lines

Yeah good idea. That feature can be broken into 2 parts:
- an email script, which could be added to Linux scripts/ dir
- maintain records for telling whether someone is first-time patch submitters

Maybe run checkpatch on those first-time submitter patches too.

Al Viro

2018-10-25 02:20 UTC #19 lore raw

Quoted 12 lines · Joe Perches

Quoted 11 lines · Fengguang Wu
CC Philip and LKP team.
Quoted 9 lines
Please try to make your first patch in drivers/staging to get familiar with submitting patches to the kernel.
https://kernelnewbies.org/FirstKernelPatch
Maybe there's utility in creating a filtering and auto-reply tool for first time patch submitters for all the vger mailing lists using some combination of previously known submitters and the 0-day robot to point those first time submitters of defective patches to kernelnewbies and staging.

Quoted 6 lines

Quoted 4 lines
Yeah good idea. That feature can be broken into 2 parts:
- an email script, which could be added to Linux scripts/ dir
- maintain records for telling whether someone is first-time patch submitters
Maybe run checkpatch on those first-time submitter patches too.

OK, now I'm certain that you are trolling...

Joe, what really pisses me off is that it's actually at the expense of original poster (who had nothing to do with the CoCup) *and* an invitation for a certain variety of kooks. In probably vain hope of heading that off, here's the summary of what happened _before_ Joe started to stir the shit:

* code in question is, indeed, (slightly) bogus in mainline.
It reads as "reject negative values for length, truncate positive ones to 4", but in reality it's "treat everything outside of 0..4 as 4". It's not broken per se, but it's certainly misleading.
* one possible fix would be to drop the "reject negative values"
completely, another - to move checking for negatives before the truncation. Patch tried to do the latter.
* the author of the patch has moved the check *too* early -
before the value being tested is even obtained. It's obviously wrong - kernel
newbie or not.
* I sincerely doubt that it was an attempt to introduce a backdoor,
albeit one would've been created if that patch went in as-is. Genuine braino is much more likely, and we'd all done such.
* such brainos can be surprisingly hard to spot in one's code.
It's too obviously wrong, in a sense - you know what you've meant to write and you keep seeing that instead of what you've actually written. If you are really unlucky, that might end up with a few days worth of debugging, with eventual embarrassed "how the fuck have I managed not to notice that previous twenty times I went over this function today?" Been there, done that, and so has everyone who'd actually written anything (other than the worthless screeds, that is).
* one thing the author of that patch could be blamed for (and most
of us have fucked up that way at one point or another) is not testing the effect of the damn thing. Modification is local, the change of behaviour - simple and triggering that code is also trivial. Checking that the patch has done what you expect it to do would be simple and would've caught the problem.
`
Code of Conduct is garbage, but neither Dave nor the author
of this patch had anything to do with that mess. If you want to make a point, do so without shit splatter hitting innocent bystanders - people tend to get very annoyed by that kind of thing, and with a damn good reason. Sheesh...

Joe Perches

2018-10-25 02:41 UTC #20 lore raw

Quoted 13 lines · Al Viro

Quoted 12 lines · Joe Perches
Quoted 11 lines · Fengguang Wu
CC Philip and LKP team.
Quoted 9 lines
Please try to make your first patch in drivers/staging to get familiar with submitting patches to the kernel.
https://kernelnewbies.org/FirstKernelPatch
Maybe there's utility in creating a filtering and auto-reply tool for first time patch submitters for all the vger mailing lists using some combination of previously known submitters and the 0-day robot to point those first time submitters of defective patches to kernelnewbies and staging.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Yeah good idea. That feature can be broken into 2 parts:
- an email script, which could be added to Linux scripts/ dir
- maintain records for telling whether someone is first-time patch submitters
Maybe run checkpatch on those first-time submitter patches too.
OK, now I'm certain that you are trolling...

Nope, the process suggestions above are sincere.

Quoted 2 lines

Joe, what really pisses me off is that it's actually at the expense of original poster (who had nothing to do with the CoCup)

CoCup? No doubt pronounced cock-up.

Quoted 11 lines

*and* an invitation for a certain
variety of kooks. In probably vain hope of heading that off, here's the summary of what happened _before_ Joe started to stir the shit:
* code in question is, indeed, (slightly) bogus in mainline.
It reads as "reject negative values for length, truncate positive ones to 4", but in reality it's "treat everything outside of 0..4 as 4". It's not broken per se, but it's certainly misleading.
* one possible fix would be to drop the "reject negative values"
completely, another - to move checking for negatives before the truncation. Patch tried to do the latter.

Umm, I suggested an appropriate mechanism to fix the patch in this thread immediately after reading it.

Quoted 4 lines

Code of Conduct is garbage, but neither Dave nor the author
of this patch had anything to do with that mess. If you want to make a point, do so without shit splatter hitting innocent bystanders - people tend to get very annoyed by that kind of thing, and with a damn good reason.

The Code of Conduct, if it exists at all, should apply to all of the kernel.

And no, as I have previously posted, I don't agree with it nor the method that was used to introduce it.

But it does exist.
Its splatter affects us all.

Willy Tarreau

2018-10-25 03:20 UTC #21 lore raw

Quoted 8 lines · Joe Perches

The Code of Conduct, if it exists at all, should apply to all of the kernel.
And no, as I have previously posted, I don't agree with it nor the method that was used to introduce it.
But it does exist.
Its splatter affects us all.

Then just like any rule, start to use it as a guideline and not as something extremely strict to apply to others. If someone feels offended he can complain, there's no reason for suggesting that maybe someone else could have felt offended, otherwise we'll end up with a new code of thinking to explain how to think what others could feel like...

Willy

Al Viro

2018-10-25 01:03 UTC #22 lore raw

In reply to Joe Perches

Quoted 9 lines · Joe Perches

Quoted 6 lines · David Miller
From: Wang Hai <[email protected]>
Date: Wed, 24 Oct 2018 23:47:29 +0800
Quoted 3 lines
To determine whether len is less than zero, it should be put before the function min_t, because the return value of min_t is not likely to be less than zero.
[]
Quoted 1 line
net/ipv4/tcp.c
Quoted 1 line
net/ipv4/tcp.c +0 -0

[snip obviously broken patch]

Quoted 10 lines

Quoted 1 line
You can't be serious?
I'm not personally taken aback by this but there is the new Code of
Conduct to consider.
John McEnroe earned quite a bit of his reputation as an 'enfant terrible' via a similar statement.

Quoted 5 lines

https://www.youtube.com/watch?v=t0hK1wyrrAU
Perhaps a different word choice next time in reply to submitters of ill-considered and/or defective patches could be useful.

Please tell me we are being Poe'd...

kbuild test robot

build bot

2018-10-24 17:34 UTC #23 lore raw

In reply to Wang Hai

Thank you for the patch! Perhaps something to improve:

Hi Wang,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v4.19]
[also build test WARNING on next-20181019]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Wang-Hai/Change-judgment-len-position/20181025-010812
config: i386-randconfig-x002-201842 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce:

        # save the attached .config to linux build tree
        make ARCH=i386

All warnings (new ones prefixed by >>):

In file included from include/linux/compiler_types.h:64:0,

                    from <command-line>:0:
   net/ipv4/tcp.c: In function 'do_tcp_getsockopt':
>> include/linux/compiler-gcc.h:82:45: warning: 'len' is used uninitialized in this function [-Wuninitialized]
    #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
                                                ^~~~~~~~~~~~
   net/ipv4/tcp.c:3302:11: note: 'len' was declared here
     int val, len;
              ^~~
--
   In file included from include/linux/compiler_types.h:64:0,
                    from <command-line>:0:
   net//ipv4/tcp.c: In function 'do_tcp_getsockopt':
>> include/linux/compiler-gcc.h:82:45: warning: 'len' is used uninitialized in this function [-Wuninitialized]
    #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
                                                ^~~~~~~~~~~~
   net//ipv4/tcp.c:3302:11: note: 'len' was declared here
     int val, len;
              ^~~

vim +/len +82 include/linux/compiler-gcc.h

87358710 David Woodhouse 2018-02-19 81
cb984d10 Joe Perches 2015-06-25 @82 #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
cb984d10 Joe Perches 2015-06-25 83

:::::: The code at line 82 was first introduced by commit
:::::: cb984d101b30eb7478d32df56a0023e4603cba7f compiler-gcc: integrate the various compiler-gcc[345].h files

:::::: TO: Joe Perches <[email protected]>
:::::: CC: Linus Torvalds <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation