[PATCH] Change judgment len position

[PATCH] Change judgment len position

[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.

Signed-off-by: Wang Hai <wanghaifine@gmail.com>
---
 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

Re: [PATCH] Change judgment len position

[Willy Tarreau]

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

Re: [PATCH] Change judgment len position

[Eric Dumazet]

On Wed, Oct 24, 2018 at 8:47 AM Wang Hai <wanghaifine@gmail.com> 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.

Re: [PATCH] Change judgment len position

[Willy Tarreau]

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

Re: [PATCH] Change judgment len position

[Joe Perches]

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

Re: [PATCH] Change judgment len position

[Willy Tarreau]

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

Re: [PATCH] Change judgment len position

[Joe Perches]

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);

Re: [PATCH] Change judgment len position

[Eric Dumazet]

On Wed, Oct 24, 2018 at 9:54 AM Joe Perches <joe@perches.com> 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

Re: [PATCH] Change judgment len position

[David Miller]

From: Wang Hai <wanghaifine@gmail.com>
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 <wanghaifine@gmail.com>
> ---
>  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.

Re: [PATCH] Change judgment len position

[Joe Perches]

On Wed, 2018-10-24 at 10:03 -0700, Eric Dumazet wrote:
> On Wed, Oct 24, 2018 at 9:54 AM Joe Perches <joe@perches.com> 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?

Re: [PATCH] Change judgment len position

[kbuild test robot]

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

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 <joe@perches.com>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26475 bytes --]

Re: [PATCH] Change judgment len position

[Joe Perches]

On Wed, 2018-10-24 at 10:10 -0700, David Miller wrote:
> From: Wang Hai <wanghaifine@gmail.com>
> 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.

Re: [PATCH] Change judgment len position

[Willy Tarreau]

On Wed, Oct 24, 2018 at 10:03:08AM -0700, Eric Dumazet wrote:
> On Wed, Oct 24, 2018 at 9:54 AM Joe Perches <joe@perches.com> 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

Re: [PATCH] Change judgment len position

[Willy Tarreau]

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 <wanghaifine@gmail.com>
> > 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

Re: [PATCH] Change judgment len position

[Joe Perches]

(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

Re: [PATCH] Change judgment len position

[David Miller]

From: Joe Perches <joe@perches.com>
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.

Re: [PATCH] Change judgment len position

[Joe Perches]

On Wed, 2018-10-24 at 17:02 -0700, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> 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

Re: [PATCH] Change judgment len position

[Eric Dumazet]

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.

Re: [PATCH] Change judgment len position

[Al Viro]

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 <wanghaifine@gmail.com>
> > 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...

Re: [PATCH] Change judgment len position

[Fengguang Wu]

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

Re: [PATCH] Change judgment len position

[Joe Perches]

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.

Re: [PATCH] Change judgment len position

[Al Viro]

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...

Re: [PATCH] Change judgment len position

[Joe Perches]

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.

Re: [PATCH] Change judgment len position

[Willy Tarreau]

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