[PATCH] net: ioctl SIOCSIFADDR minor cleanup

[PATCH] net: ioctl SIOCSIFADDR minor cleanup

[yuan linyu]

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

1. set interface address label to ioctl request device name is enough
2. when address pass inet_abc_len check, prefixlen < 31 is always true

Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
---
 net/ipv4/devinet.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 062a67c..d491a7a 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1063,10 +1063,7 @@ int devinet_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 			if (!ifa)
 				break;
 			INIT_HLIST_NODE(&ifa->hash);
-			if (colon)
-				memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
-			else
-				memcpy(ifa->ifa_label, dev->name, IFNAMSIZ);
+			memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
 		} else {
 			ret = 0;
 			if (ifa->ifa_local == sin->sin_addr.s_addr)
@@ -1081,8 +1078,7 @@ int devinet_ioctl(struct net *net, unsigned int cmd, void __user *arg)
 		if (!(dev->flags & IFF_POINTOPOINT)) {
 			ifa->ifa_prefixlen = inet_abc_len(ifa->ifa_address);
 			ifa->ifa_mask = inet_make_mask(ifa->ifa_prefixlen);
-			if ((dev->flags & IFF_BROADCAST) &&
-			    ifa->ifa_prefixlen < 31)
+			if (dev->flags & IFF_BROADCAST)
 				ifa->ifa_broadcast = ifa->ifa_address |
 						     ~ifa->ifa_mask;
 		} else {
-- 
2.7.4

Re: [PATCH] net: ioctl SIOCSIFADDR minor cleanup

[David Miller]

From: yuan linyu <cugyly@163.com>
Date: Tue, 15 Nov 2016 20:44:59 +0800

> @@ -1063,10 +1063,7 @@ int devinet_ioctl(struct net *net, unsigned int cmd, void __user *arg)
>  			if (!ifa)
>  				break;
>  			INIT_HLIST_NODE(&ifa->hash);
> -			if (colon)
> -				memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
> -			else
> -				memcpy(ifa->ifa_label, dev->name, IFNAMSIZ);
> +			memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);

This is an incorrect and untested change.

If there is a colon character in ifr.ifr_name, earlier in this function
we will:

1) First replace that colon with a NULL character.

2) Perform __dev_get_by_name() on the result.

3) Put the ":" colon character back into ifr.ifr_name

So the two strings ifr.ifr_name and dev->name can in fact be different
here.  Therefore the code has to be left as it is.

RE: [PATCH] net: ioctl SIOCSIFADDR minor cleanup

[YUAN Linyu]

hi david,

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of David Miller
> Sent: Wednesday, November 16, 2016 11:01 AM
> To: cugyly@163.com
> Cc: netdev@vger.kernel.org; YUAN Linyu
> Subject: Re: [PATCH] net: ioctl SIOCSIFADDR minor cleanup
> 
> From: yuan linyu <cugyly@163.com>
> Date: Tue, 15 Nov 2016 20:44:59 +0800
> 
> > @@ -1063,10 +1063,7 @@ int devinet_ioctl(struct net *net, unsigned int
> cmd, void __user *arg)
> >  			if (!ifa)
> >  				break;
> >  			INIT_HLIST_NODE(&ifa->hash);
> > -			if (colon)
> > -				memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
> > -			else
> > -				memcpy(ifa->ifa_label, dev->name, IFNAMSIZ);
> > +			memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
> 
> This is an incorrect and untested change.
> 
> If there is a colon character in ifr.ifr_name, earlier in this function
> we will:
> 
> 1) First replace that colon with a NULL character.
> 
> 2) Perform __dev_get_by_name() on the result.
> 
> 3) Put the ":" colon character back into ifr.ifr_name
> 
> So the two strings ifr.ifr_name and dev->name can in fact be different
> here.  Therefore the code has to be left as it is.
Here we assign value to ifa->ifa_label. 
orginal code means when reqest name have colon, then label name will have colon.
If request name have no colon, label name will same as dev-name, it's also have no colon.
So assign label to request name will do same thing as original code.
thanks

Re: [PATCH] net: ioctl SIOCSIFADDR minor cleanup

[David Miller]

From: YUAN Linyu <Linyu.Yuan@alcatel-sbell.com.cn>
Date: Wed, 16 Nov 2016 03:13:31 +0000

> orginal code means when reqest name have colon, then label name will have colon.

And that is intentional.

Re: [PATCH] net: ioctl SIOCSIFADDR minor cleanup

[David Miller]

From: YUAN Linyu <Linyu.Yuan@alcatel-sbell.com.cn>
Date: Wed, 16 Nov 2016 03:13:31 +0000

> So assign label to request name will do same thing as original code.

Nope.  dev->name does not have the colon, it was trimmed from the
string for the device lookup.  So the found device's dev->name does
not have the colon character, even if it was in ifr.ifr_name

This was my entire point.

You are changing the behvaior of the code in an invalid way.

RE: [PATCH] net: ioctl SIOCSIFADDR minor cleanup

[YUAN Linyu]

No, this patch will not change dev->name,
It's care about ifa->ifa_label.
> -			if (colon)
> -				memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
> -			else
> -				memcpy(ifa->ifa_label, dev->name, IFNAMSIZ);
When ifr.ifr_name have no colon, dev->name must equal to ifr.ifr_name.
So we change to 
> -			else
> -				memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
Then if and else do same thing. Just one line is enough,
memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, November 16, 2016 11:31 AM
> To: YUAN Linyu
> Cc: cugyly@163.com; netdev@vger.kernel.org
> Subject: Re: [PATCH] net: ioctl SIOCSIFADDR minor cleanup
> 
> From: YUAN Linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> Date: Wed, 16 Nov 2016 03:13:31 +0000
> 
> > So assign label to request name will do same thing as original code.
> 
> Nope.  dev->name does not have the colon, it was trimmed from the
> string for the device lookup.  So the found device's dev->name does
> not have the colon character, even if it was in ifr.ifr_name
> 
> This was my entire point.
> 
> You are changing the behvaior of the code in an invalid way.

Re: [PATCH] net: ioctl SIOCSIFADDR minor cleanup

[Cong Wang]

On Tue, Nov 15, 2016 at 7:57 PM, YUAN Linyu
<Linyu.Yuan@alcatel-sbell.com.cn> wrote:
> No, this patch will not change dev->name,
> It's care about ifa->ifa_label.
>> -                     if (colon)
>> -                             memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
>> -                     else
>> -                             memcpy(ifa->ifa_label, dev->name, IFNAMSIZ);
> When ifr.ifr_name have no colon, dev->name must equal to ifr.ifr_name.
> So we change to
>> -                     else
>> -                             memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
> Then if and else do same thing. Just one line is enough,
> memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
>

I don't understand why you try to optimize a slow path and some kind of
obsolete code.

RE: [PATCH] net: ioctl SIOCSIFADDR minor cleanup

[YUAN Linyu]

I think there are newbie include me still use ifconfig utility. 
So when I check this code, it can be optimized.

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Cong Wang
> Sent: Tuesday, November 22, 2016 12:20 PM
> To: YUAN Linyu
> Cc: David Miller; cugyly@163.com; netdev@vger.kernel.org
> Subject: Re: [PATCH] net: ioctl SIOCSIFADDR minor cleanup
> 
> On Tue, Nov 15, 2016 at 7:57 PM, YUAN Linyu
> <Linyu.Yuan@alcatel-sbell.com.cn> wrote:
> > No, this patch will not change dev->name,
> > It's care about ifa->ifa_label.
> >> -                     if (colon)
> >> -                             memcpy(ifa->ifa_label, ifr.ifr_name,
> IFNAMSIZ);
> >> -                     else
> >> -                             memcpy(ifa->ifa_label, dev->name,
> IFNAMSIZ);
> > When ifr.ifr_name have no colon, dev->name must equal to ifr.ifr_name.
> > So we change to
> >> -                     else
> >> -                             memcpy(ifa->ifa_label, ifr.ifr_name,
> IFNAMSIZ);
> > Then if and else do same thing. Just one line is enough,
> > memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
> >
> 
> I don't understand why you try to optimize a slow path and some kind of
> obsolete code.

Re: [PATCH] net: ioctl SIOCSIFADDR minor cleanup

[Cong Wang]

On Mon, Nov 21, 2016 at 8:30 PM, YUAN Linyu
<Linyu.Yuan@alcatel-sbell.com.cn> wrote:
> I think there are newbie include me still use ifconfig utility.
> So when I check this code, it can be optimized.
>

Working on Linux kernel is always welcome.

But I don't think this can convince DaveM to take your patch even
if it is really correct (I never check).

[PATCH] net: ioctl SIOCSIFADDR minor cleanup

[yuan linyu]

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

1. set interface address label to ioctl request device name is enough
2. when address pass inet_abc_len check, prefixlen less than 31 is always true

Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
---
 net/ipv4/devinet.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 062a67c..d491a7a 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1063,10 +1063,7 @@ int devinet_ioctl(struct net *net, unsigned int cmd, void
__user *arg)
 			if (!ifa)
 				break;
 			INIT_HLIST_NODE(&ifa->hash);
-			if (colon)
-				memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
-			else
-				memcpy(ifa->ifa_label, dev->name, IFNAMSIZ);
+			memcpy(ifa->ifa_label, ifr.ifr_name, IFNAMSIZ);
 		} else {
 			ret = 0;
 			if (ifa->ifa_local == sin->sin_addr.s_addr)
@@ -1081,8 +1078,7 @@ int devinet_ioctl(struct net *net, unsigned int cmd, void
__user *arg)
 		if (!(dev->flags & IFF_POINTOPOINT)) {
 			ifa->ifa_prefixlen = inet_abc_len(ifa->ifa_address);
 			ifa->ifa_mask = inet_make_mask(ifa->ifa_prefixlen);
-			if ((dev->flags & IFF_BROADCAST) &&
-			    ifa->ifa_prefixlen < 31)
+			if (dev->flags & IFF_BROADCAST)
 				ifa->ifa_broadcast = ifa->ifa_address |
 						     ~ifa->ifa_mask;
 		} else {
-- 
2.7.4

Re: [PATCH] net: ioctl SIOCSIFADDR minor cleanup

[David Miller]

Your patch was mangled by your email client, chopping up long lines
and turning TAB characters into spaces.

Please fix this, email a test patch to yourself, and do not resubmit
your change until you can successfully apply the patch you receive
in a test email.  Also, do not try using attachments to fix this
problem, patches must be inline.