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(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 10c624639..49af9fdc3 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -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;
--
2.17.1
On Wed, Oct 24, 2018 at 11:47:29PM +0800, Wang Hai wrote:
> 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 :
> 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.
> if (get_user(len, optlen))
> return -EFAULT;
Then it gets overridden by get_user()
> - len = min_t(unsigned int, len, sizeof(int));
> -
Then its positive values are not bounded anymore since you moved the test.
> 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
On Wed, Oct 24, 2018 at 8:47 AM Wang Hai <[email protected]> wrote:
>
> 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.
On Wed, Oct 24, 2018 at 08:59:39AM -0700, Eric Dumazet wrote:
> 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
On Wed, 2018-10-24 at 17:57 +0200, Willy Tarreau wrote:
> On Wed, Oct 24, 2018 at 11:47:29PM +0800, Wang Hai wrote:
> > 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 :
>
> > 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.
>
> > if (get_user(len, optlen))
> > return -EFAULT;
>
> Then it gets overridden by get_user()
>
> > - 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
> > if (len < 0)
> > return -EINVAL;
So this test len < 0 could be moved up above min_t
> 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
On Wed, Oct 24, 2018 at 09:23:19AM -0700, Joe Perches wrote:
> On Wed, 2018-10-24 at 17:57 +0200, Willy Tarreau wrote:
> > On Wed, Oct 24, 2018 at 11:47:29PM +0800, Wang Hai wrote:
> > > 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 :
> >
> > > 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.
> >
> > > if (get_user(len, optlen))
> > > return -EFAULT;
> >
> > Then it gets overridden by get_user()
> >
> > > - 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
>
> > > 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
On Wed, 2018-10-24 at 18:32 +0200, Willy Tarreau wrote:
> On Wed, Oct 24, 2018 at 09:23:19AM -0700, Joe Perches wrote:
> > On Wed, 2018-10-24 at 17:57 +0200, Willy Tarreau wrote:
> > > On Wed, Oct 24, 2018 at 11:47:29PM +0800, Wang Hai wrote:
> > > > 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 :
> > >
> > > > 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.
> > >
> > > > if (get_user(len, optlen))
> > > > return -EFAULT;
> > >
> > > Then it gets overridden by get_user()
> > >
> > > > - 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
> >
> > > > 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);
On Wed, Oct 24, 2018 at 9:54 AM Joe Perches <[email protected]> wrote:
> 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 (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
From: Wang Hai <[email protected]>
Date: Wed, 24 Oct 2018 23:47:29 +0800
> 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(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 10c624639..49af9fdc3 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -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.
On Wed, 2018-10-24 at 10:03 -0700, Eric Dumazet wrote:
> On Wed, Oct 24, 2018 at 9:54 AM Joe Perches <[email protected]> wrote:
>
> > 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 (len > sizeof(int))
> > len = sizeof(int);
>
> It is a matter of taste really,
Agree and hence my use of 'I think' above.
> 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.
> I would suggest that if someones wants to change the current code, a
> corresponding test would be added in tools/testing/selftests/net?
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
On Wed, 2018-10-24 at 10:10 -0700, David Miller wrote:
> From: Wang Hai <[email protected]>
> Date: Wed, 24 Oct 2018 23:47:29 +0800
>
> > 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.
[]
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
[]
> > @@ -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.
On Wed, Oct 24, 2018 at 10:03:08AM -0700, Eric Dumazet wrote:
> On Wed, Oct 24, 2018 at 9:54 AM Joe Perches <[email protected]> wrote:
>
> > 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 (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)".
> 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
On Wed, Oct 24, 2018 at 11:28:11AM -0700, Joe Perches wrote:
> On Wed, 2018-10-24 at 10:10 -0700, David Miller wrote:
> > From: Wang Hai <[email protected]>
> > Date: Wed, 24 Oct 2018 23:47:29 +0800
> >
> > > 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.
> []
> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> []
> > > @@ -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.
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
(adding Fengguang Wu and lkp)
On Wed, 2018-10-24 at 22:48 +0200, Willy Tarreau wrote:
> 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.
> 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
From: Joe Perches <[email protected]>
Date: Wed, 24 Oct 2018 16:46:18 -0700
> 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.
On Wed, 2018-10-24 at 17:02 -0700, David Miller wrote:
> From: Joe Perches <[email protected]>
> Date: Wed, 24 Oct 2018 16:46:18 -0700
>
> > 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?
> 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.
From the new Code of Conduct (which I don't even approve)
* Gracefully accepting constructive criticism.
cheers, Joe
On 10/24/2018 05:23 PM, Joe Perches wrote:
>
> "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.
On Wed, Oct 24, 2018 at 11:28:11AM -0700, Joe Perches wrote:
> On Wed, 2018-10-24 at 10:10 -0700, David Miller wrote:
> > From: Wang Hai <[email protected]>
> > Date: Wed, 24 Oct 2018 23:47:29 +0800
> >
> > > 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.
> []
> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
[snip obviously broken patch]
> > 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.
Please tell me we are being Poe'd...
CC Philip and LKP team.
On Wed, Oct 24, 2018 at 04:46:18PM -0700, Joe Perches wrote:
>(adding Fengguang Wu and lkp)
>
>On Wed, 2018-10-24 at 22:48 +0200, Willy Tarreau wrote:
>> 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.
>
>> 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.
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
On Thu, 2018-10-25 at 09:11 +0800, Fengguang Wu wrote:
> CC Philip and LKP team.
> > 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.
>
> 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.
On Wed, Oct 24, 2018 at 06:16:31PM -0700, Joe Perches wrote:
> On Thu, 2018-10-25 at 09:11 +0800, Fengguang Wu wrote:
> > CC Philip and LKP team.
> > > 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.
> >
> > 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...
On Thu, 2018-10-25 at 03:20 +0100, Al Viro wrote:
> On Wed, Oct 24, 2018 at 06:16:31PM -0700, Joe Perches wrote:
> > On Thu, 2018-10-25 at 09:11 +0800, Fengguang Wu wrote:
> > > CC Philip and LKP team.
> > > > 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.
> > >
> > > 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.
> 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.
> *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.
> 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.
On Wed, Oct 24, 2018 at 07:41:52PM -0700, Joe Perches wrote:
> 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