[PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()

[PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()

[yuan linyu]

From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>

Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
---
 net/ipv6/ndisc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index d310dc4..414e929 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -148,17 +148,18 @@ void __ndisc_fill_addr_option(struct sk_buff *skb, int type, void *data,
 
 	opt[0] = type;
 	opt[1] = space>>3;
+	opt   += 2;
+	space -= 2;
 
-	memset(opt + 2, 0, pad);
+	memset(opt, 0, pad);
 	opt   += pad;
 	space -= pad;
 
-	memcpy(opt+2, data, data_len);
-	data_len += 2;
+	memcpy(opt, data, data_len);
 	opt += data_len;
 	space -= data_len;
-	if (space > 0)
-		memset(opt, 0, space);
+
+	memset(opt, 0, space);
 }
 EXPORT_SYMBOL_GPL(__ndisc_fill_addr_option);
 
-- 
2.7.4

Re: [PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()

[Alexey Dobriyan]

> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -148,17 +148,18 @@ void __ndisc_fill_addr_option(struct sk_buff *skb, int type, void *data,

>  	space -= data_len;
> -	if (space > 0)
> -		memset(opt, 0, space);
> +
> +	memset(opt, 0, space);

This can't be right.

And what size are you reducing?

Re: [PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()

[David Miller]

From: yuan linyu <cugyly@163.com>
Date: Sat, 27 May 2017 06:00:52 +0800

> From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> 
> Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>

Applied, thanks.

Re: [PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()

[David Miller]

From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Sat, 27 May 2017 18:15:14 +0300

>> --- a/net/ipv6/ndisc.c
>> +++ b/net/ipv6/ndisc.c
>> @@ -148,17 +148,18 @@ void __ndisc_fill_addr_option(struct sk_buff *skb, int type, void *data,
> 
>>  	space -= data_len;
>> -	if (space > 0)
>> -		memset(opt, 0, space);
>> +
>> +	memset(opt, 0, space);
> 
> This can't be right.
> 
> And what size are you reducing?

It is right, space equals the same thing it would have equaled
before his changes, and a memset() of zero length will do the
right thing.

Finally, if space can be negative here, we have real problems.

Re: [PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()

[Joe Perches]

On Mon, 2017-05-29 at 23:30 -0400, David Miller wrote:
> From: yuan linyu <cugyly@163.com>
> Date: Sat, 27 May 2017 06:00:52 +0800
> 
> > From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> > 
> > Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> 
> Applied, thanks.

OK, but is it really safe though?

Could "space" (an int) ever be negative after
subtracting "pad" and "data_len"?

Re: [PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()

[David Ahern]

On 5/29/17 9:41 PM, Joe Perches wrote:
> On Mon, 2017-05-29 at 23:30 -0400, David Miller wrote:
>> From: yuan linyu <cugyly@163.com>
>> Date: Sat, 27 May 2017 06:00:52 +0800
>>
>>> From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
>>>
>>> Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
>> Applied, thanks.
> OK, but is it really safe though?
> 
> Could "space" (an int) ever be negative after
> subtracting "pad" and "data_len"?
> 

that function should be converted to skb_put_zero once it hits net-next.

Re: [PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()

[Joe Perches]

On Mon, 2017-05-29 at 23:31 -0400, David Miller wrote:
> From: Alexey Dobriyan <adobriyan@gmail.com>
> Date: Sat, 27 May 2017 18:15:14 +0300
> 
> >> --- a/net/ipv6/ndisc.c
> >> +++ b/net/ipv6/ndisc.c
> >> @@ -148,17 +148,18 @@ void __ndisc_fill_addr_option(struct sk_buff *skb, int type, void *data,
> > 
> >>      space -= data_len;
> >> -    if (space > 0)
> >> -            memset(opt, 0, space);
> >> +
> >> +    memset(opt, 0, space);
> > 
> > This can't be right.
> > 
> > And what size are you reducing?
> 
> It is right, space equals the same thing it would have equaled
> before his changes, and a memset() of zero length will do the
> right thing.
> 
> Finally, if space can be negative here, we have real problems.

__ndisc_fill_addr_option is exported so yeah.

RE: [PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()

[YUAN Linyu]

Hi joe,

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Joe Perches
> Sent: Tuesday, May 30, 2017 11:41 AM
> To: David Miller; cugyly@163.com
> Cc: netdev@vger.kernel.org; dsahern@gmail.com; YUAN Linyu
> Subject: Re: [PATCH net-next] net: ndisc.c: reduce size of
> __ndisc_fill_addr_option()
> 
> On Mon, 2017-05-29 at 23:30 -0400, David Miller wrote:
> > From: yuan linyu <cugyly@163.com>
> > Date: Sat, 27 May 2017 06:00:52 +0800
> >
> > > From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> > >
> > > Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> >
> > Applied, thanks.
> 
> OK, but is it really safe though?
> 
> Could "space" (an int) ever be negative after
> subtracting "pad" and "data_len"?
It's safe, check
int space = __ndisc_opt_addr_space(data_len, pad);
space is maximum aligned value.
And I check current pad, the maximum value is 2.

RE: [PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()

[YUAN Linyu]

> -----Original Message-----
> From: David Ahern [mailto:dsahern@gmail.com]
> Sent: Tuesday, May 30, 2017 11:42 AM
> To: Joe Perches; David Miller; cugyly@163.com
> Cc: netdev@vger.kernel.org; YUAN Linyu
> Subject: Re: [PATCH net-next] net: ndisc.c: reduce size of
> __ndisc_fill_addr_option()
> 
> On 5/29/17 9:41 PM, Joe Perches wrote:
> > On Mon, 2017-05-29 at 23:30 -0400, David Miller wrote:
> >> From: yuan linyu <cugyly@163.com>
> >> Date: Sat, 27 May 2017 06:00:52 +0800
> >>
> >>> From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> >>>
> >>> Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> >> Applied, thanks.
> > OK, but is it really safe though?
> >
> > Could "space" (an int) ever be negative after
> > subtracting "pad" and "data_len"?
> >
> 
> that function should be converted to skb_put_zero once it hits net-next.
I will check it

RE: [PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()

[YUAN Linyu]

> > -----Original Message-----
> > From: David Ahern [mailto:dsahern@gmail.com]
> > Sent: Tuesday, May 30, 2017 11:42 AM
> > To: Joe Perches; David Miller; cugyly@163.com
> > Cc: netdev@vger.kernel.org; YUAN Linyu
> > Subject: Re: [PATCH net-next] net: ndisc.c: reduce size of
> > __ndisc_fill_addr_option()
> >
> > On 5/29/17 9:41 PM, Joe Perches wrote:
> > > On Mon, 2017-05-29 at 23:30 -0400, David Miller wrote:
> > >> From: yuan linyu <cugyly@163.com>
> > >> Date: Sat, 27 May 2017 06:00:52 +0800
> > >>
> > >>> From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> > >>>
> > >>> Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> > >> Applied, thanks.
> > > OK, but is it really safe though?
> > >
> > > Could "space" (an int) ever be negative after
> > > subtracting "pad" and "data_len"?
> > >
> >
> > that function should be converted to skb_put_zero once it hits net-next.
> I will check it
I can't find skb_put_zero

Re: [PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()

[David Ahern]

On 5/30/17 6:29 PM, YUAN Linyu wrote:
>>> that function should be converted to skb_put_zero once it hits net-next.
>> I will check it
> I can't find skb_put_zero

I believe the decision was to put it in Johannes' tree and it will make
its way to DaveM's tree. Give some time.

RE: [PATCH net-next] net: ndisc.c: reduce size of __ndisc_fill_addr_option()

[YUAN Linyu]

Ok, I will send v2 later

> -----Original Message-----
> From: David Ahern [mailto:dsahern@gmail.com]
> Sent: Wednesday, May 31, 2017 8:40 AM
> To: YUAN Linyu; David Ahern; Joe Perches; David Miller; cugyly@163.com
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH net-next] net: ndisc.c: reduce size of
> __ndisc_fill_addr_option()
> 
> On 5/30/17 6:29 PM, YUAN Linyu wrote:
> >>> that function should be converted to skb_put_zero once it hits net-next.
> >> I will check it
> > I can't find skb_put_zero
> 
> I believe the decision was to put it in Johannes' tree and it will make
> its way to DaveM's tree. Give some time.