linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Change judgment len position
@ 2018-10-24 15:47 Wang Hai
  2018-10-24 15:57 ` Willy Tarreau
                   ` (3 more replies)
  0 siblings, 4 replies; 24+ messages in thread
From: Wang Hai @ 2018-10-24 15:47 UTC (permalink / raw)
  To: edumazet; +Cc: davem, kuznet, yoshfuji, netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 15:47 [PATCH] Change judgment len position Wang Hai
@ 2018-10-24 15:57 ` Willy Tarreau
  2018-10-24 16:23   ` Joe Perches
  2018-10-24 15:59 ` Eric Dumazet
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 24+ messages in thread
From: Willy Tarreau @ 2018-10-24 15:57 UTC (permalink / raw)
  To: Wang Hai; +Cc: edumazet, davem, kuznet, yoshfuji, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 15:47 [PATCH] Change judgment len position Wang Hai
  2018-10-24 15:57 ` Willy Tarreau
@ 2018-10-24 15:59 ` Eric Dumazet
  2018-10-24 16:01   ` Willy Tarreau
  2018-10-24 17:10 ` David Miller
  2018-10-24 17:34 ` kbuild test robot
  3 siblings, 1 reply; 24+ messages in thread
From: Eric Dumazet @ 2018-10-24 15:59 UTC (permalink / raw)
  To: wanghaifine
  Cc: David Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev, LKML

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.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 15:59 ` Eric Dumazet
@ 2018-10-24 16:01   ` Willy Tarreau
  0 siblings, 0 replies; 24+ messages in thread
From: Willy Tarreau @ 2018-10-24 16:01 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: wanghaifine, David Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
	netdev, LKML

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 15:57 ` Willy Tarreau
@ 2018-10-24 16:23   ` Joe Perches
  2018-10-24 16:32     ` Willy Tarreau
  0 siblings, 1 reply; 24+ messages in thread
From: Joe Perches @ 2018-10-24 16:23 UTC (permalink / raw)
  To: Willy Tarreau, Wang Hai
  Cc: edumazet, davem, kuznet, yoshfuji, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 16:23   ` Joe Perches
@ 2018-10-24 16:32     ` Willy Tarreau
  2018-10-24 16:54       ` Joe Perches
  0 siblings, 1 reply; 24+ messages in thread
From: Willy Tarreau @ 2018-10-24 16:32 UTC (permalink / raw)
  To: Joe Perches
  Cc: Wang Hai, edumazet, davem, kuznet, yoshfuji, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 16:32     ` Willy Tarreau
@ 2018-10-24 16:54       ` Joe Perches
  2018-10-24 17:03         ` Eric Dumazet
  0 siblings, 1 reply; 24+ messages in thread
From: Joe Perches @ 2018-10-24 16:54 UTC (permalink / raw)
  To: Willy Tarreau
  Cc: Wang Hai, edumazet, davem, kuznet, yoshfuji, netdev, linux-kernel

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




^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 16:54       ` Joe Perches
@ 2018-10-24 17:03         ` Eric Dumazet
  2018-10-24 17:18           ` Joe Perches
  2018-10-24 20:09           ` Willy Tarreau
  0 siblings, 2 replies; 24+ messages in thread
From: Eric Dumazet @ 2018-10-24 17:03 UTC (permalink / raw)
  To: joe
  Cc: Willy Tarreau, wanghaifine, David Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, netdev, LKML

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 15:47 [PATCH] Change judgment len position Wang Hai
  2018-10-24 15:57 ` Willy Tarreau
  2018-10-24 15:59 ` Eric Dumazet
@ 2018-10-24 17:10 ` David Miller
  2018-10-24 18:28   ` Joe Perches
  2018-10-24 17:34 ` kbuild test robot
  3 siblings, 1 reply; 24+ messages in thread
From: David Miller @ 2018-10-24 17:10 UTC (permalink / raw)
  To: wanghaifine; +Cc: edumazet, kuznet, yoshfuji, netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 17:03         ` Eric Dumazet
@ 2018-10-24 17:18           ` Joe Perches
  2018-10-24 20:09           ` Willy Tarreau
  1 sibling, 0 replies; 24+ messages in thread
From: Joe Perches @ 2018-10-24 17:18 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Willy Tarreau, wanghaifine, David Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, netdev, LKML

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?


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 15:47 [PATCH] Change judgment len position Wang Hai
                   ` (2 preceding siblings ...)
  2018-10-24 17:10 ` David Miller
@ 2018-10-24 17:34 ` kbuild test robot
  3 siblings, 0 replies; 24+ messages in thread
From: kbuild test robot @ 2018-10-24 17:34 UTC (permalink / raw)
  To: Wang Hai
  Cc: kbuild-all, edumazet, davem, kuznet, yoshfuji, netdev,
	linux-kernel, Wang Hai

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 17:10 ` David Miller
@ 2018-10-24 18:28   ` Joe Perches
  2018-10-24 20:48     ` Willy Tarreau
  2018-10-25  1:03     ` Al Viro
  0 siblings, 2 replies; 24+ messages in thread
From: Joe Perches @ 2018-10-24 18:28 UTC (permalink / raw)
  To: David Miller, wanghaifine; +Cc: netdev, LKML

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.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 17:03         ` Eric Dumazet
  2018-10-24 17:18           ` Joe Perches
@ 2018-10-24 20:09           ` Willy Tarreau
  1 sibling, 0 replies; 24+ messages in thread
From: Willy Tarreau @ 2018-10-24 20:09 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: joe, wanghaifine, David Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, netdev, LKML

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 18:28   ` Joe Perches
@ 2018-10-24 20:48     ` Willy Tarreau
  2018-10-24 23:46       ` Joe Perches
  2018-10-25  1:03     ` Al Viro
  1 sibling, 1 reply; 24+ messages in thread
From: Willy Tarreau @ 2018-10-24 20:48 UTC (permalink / raw)
  To: Joe Perches; +Cc: David Miller, wanghaifine, netdev, LKML

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 20:48     ` Willy Tarreau
@ 2018-10-24 23:46       ` Joe Perches
  2018-10-25  0:02         ` David Miller
  2018-10-25  1:11         ` Fengguang Wu
  0 siblings, 2 replies; 24+ messages in thread
From: Joe Perches @ 2018-10-24 23:46 UTC (permalink / raw)
  To: Willy Tarreau; +Cc: David Miller, wanghaifine, netdev, LKML, Fengguang Wu, lkp

(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


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 23:46       ` Joe Perches
@ 2018-10-25  0:02         ` David Miller
  2018-10-25  0:23           ` Joe Perches
  2018-10-25  1:11         ` Fengguang Wu
  1 sibling, 1 reply; 24+ messages in thread
From: David Miller @ 2018-10-25  0:02 UTC (permalink / raw)
  To: joe; +Cc: w, wanghaifine, netdev, linux-kernel, fengguang.wu, lkp

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.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-25  0:02         ` David Miller
@ 2018-10-25  0:23           ` Joe Perches
  2018-10-25  0:50             ` Eric Dumazet
  0 siblings, 1 reply; 24+ messages in thread
From: Joe Perches @ 2018-10-25  0:23 UTC (permalink / raw)
  To: David Miller; +Cc: w, wanghaifine, netdev, linux-kernel, fengguang.wu, lkp

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


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-25  0:23           ` Joe Perches
@ 2018-10-25  0:50             ` Eric Dumazet
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Dumazet @ 2018-10-25  0:50 UTC (permalink / raw)
  To: Joe Perches, David Miller
  Cc: w, wanghaifine, netdev, linux-kernel, fengguang.wu, lkp



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.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 18:28   ` Joe Perches
  2018-10-24 20:48     ` Willy Tarreau
@ 2018-10-25  1:03     ` Al Viro
  1 sibling, 0 replies; 24+ messages in thread
From: Al Viro @ 2018-10-25  1:03 UTC (permalink / raw)
  To: Joe Perches; +Cc: David Miller, wanghaifine, netdev, LKML

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-24 23:46       ` Joe Perches
  2018-10-25  0:02         ` David Miller
@ 2018-10-25  1:11         ` Fengguang Wu
  2018-10-25  1:16           ` Joe Perches
  1 sibling, 1 reply; 24+ messages in thread
From: Fengguang Wu @ 2018-10-25  1:11 UTC (permalink / raw)
  To: Joe Perches
  Cc: Willy Tarreau, David Miller, wanghaifine, netdev, LKML, LKP, Philip Li

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-25  1:11         ` Fengguang Wu
@ 2018-10-25  1:16           ` Joe Perches
  2018-10-25  2:20             ` Al Viro
  0 siblings, 1 reply; 24+ messages in thread
From: Joe Perches @ 2018-10-25  1:16 UTC (permalink / raw)
  To: Fengguang Wu
  Cc: Willy Tarreau, David Miller, wanghaifine, netdev, LKML, LKP, Philip Li

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.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-25  1:16           ` Joe Perches
@ 2018-10-25  2:20             ` Al Viro
  2018-10-25  2:41               ` Joe Perches
  0 siblings, 1 reply; 24+ messages in thread
From: Al Viro @ 2018-10-25  2:20 UTC (permalink / raw)
  To: Joe Perches
  Cc: Fengguang Wu, Willy Tarreau, David Miller, wanghaifine, netdev,
	LKML, LKP, Philip Li

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-25  2:20             ` Al Viro
@ 2018-10-25  2:41               ` Joe Perches
  2018-10-25  3:20                 ` Willy Tarreau
  0 siblings, 1 reply; 24+ messages in thread
From: Joe Perches @ 2018-10-25  2:41 UTC (permalink / raw)
  To: Al Viro
  Cc: Fengguang Wu, Willy Tarreau, David Miller, wanghaifine, netdev,
	LKML, LKP, Philip Li

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.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] Change judgment len position
  2018-10-25  2:41               ` Joe Perches
@ 2018-10-25  3:20                 ` Willy Tarreau
  0 siblings, 0 replies; 24+ messages in thread
From: Willy Tarreau @ 2018-10-25  3:20 UTC (permalink / raw)
  To: Joe Perches
  Cc: Al Viro, Fengguang Wu, David Miller, wanghaifine, netdev, LKML,
	LKP, Philip Li

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2018-10-25  3:20 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-24 15:47 [PATCH] Change judgment len position Wang Hai
2018-10-24 15:57 ` Willy Tarreau
2018-10-24 16:23   ` Joe Perches
2018-10-24 16:32     ` Willy Tarreau
2018-10-24 16:54       ` Joe Perches
2018-10-24 17:03         ` Eric Dumazet
2018-10-24 17:18           ` Joe Perches
2018-10-24 20:09           ` Willy Tarreau
2018-10-24 15:59 ` Eric Dumazet
2018-10-24 16:01   ` Willy Tarreau
2018-10-24 17:10 ` David Miller
2018-10-24 18:28   ` Joe Perches
2018-10-24 20:48     ` Willy Tarreau
2018-10-24 23:46       ` Joe Perches
2018-10-25  0:02         ` David Miller
2018-10-25  0:23           ` Joe Perches
2018-10-25  0:50             ` Eric Dumazet
2018-10-25  1:11         ` Fengguang Wu
2018-10-25  1:16           ` Joe Perches
2018-10-25  2:20             ` Al Viro
2018-10-25  2:41               ` Joe Perches
2018-10-25  3:20                 ` Willy Tarreau
2018-10-25  1:03     ` Al Viro
2018-10-24 17:34 ` kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).