[net-next] net: assign err to 0 at begin in do_setlink() function

Message ID 1510669859-12055-1-git-send-email-cugyly@163.com
State Rejected
Delegated to: David Miller
Headers show
Series
  • [net-next] net: assign err to 0 at begin in do_setlink() function
Related show

Commit Message

yuan linyu Nov. 14, 2017, 2:30 p.m.
From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>

each netlink attribute have proper process when error happen,
when exit on attribute process, it implies that no error,
so err = 0; is useless.

assign err = 0; at beginning if all attributes not set.

Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
---
 net/core/rtnetlink.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Comments

David Miller Nov. 15, 2017, 5:08 a.m. | #1
From: yuan linyu <cugyly@163.com>
Date: Tue, 14 Nov 2017 22:30:59 +0800

> From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> 
> each netlink attribute have proper process when error happen,
> when exit on attribute process, it implies that no error,
> so err = 0; is useless.
> 
> assign err = 0; at beginning if all attributes not set.
> 
> Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>

This change is not correct.

The IFLA_VF_PORTS code block can finish with err set non-zero.
The assignment to zero there is really necessary.
Yuan, Linyu (NSB - CN/Shanghai) Nov. 15, 2017, 5:15 a.m. | #2
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of David Miller
> Sent: Wednesday, November 15, 2017 1:08 PM
> To: cugyly@163.com
> Cc: netdev@vger.kernel.org; Yuan, Linyu (NSB - CN/Shanghai)
> Subject: Re: [PATCH net-next] net: assign err to 0 at begin in do_setlink()
> function
> 
> From: yuan linyu <cugyly@163.com>
> Date: Tue, 14 Nov 2017 22:30:59 +0800
> 
> > From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> >
> > each netlink attribute have proper process when error happen,
> > when exit on attribute process, it implies that no error,
> > so err = 0; is useless.
> >
> > assign err = 0; at beginning if all attributes not set.
> >
> > Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> 
> This change is not correct.
> 
> The IFLA_VF_PORTS code block can finish with err set non-zero.

Do you mean this block can return err = -EOPNOTSUPP;  ?
It means nla_for_each_nested() in this block will never enter, right ?

> The assignment to zero there is really necessary.
David Miller Nov. 15, 2017, 5:19 a.m. | #3
From: "Yuan, Linyu (NSB - CN/Shanghai)" <linyu.yuan@nokia-sbell.com>
Date: Wed, 15 Nov 2017 05:15:35 +0000

> 
> 
>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
>> On Behalf Of David Miller
>> Sent: Wednesday, November 15, 2017 1:08 PM
>> To: cugyly@163.com
>> Cc: netdev@vger.kernel.org; Yuan, Linyu (NSB - CN/Shanghai)
>> Subject: Re: [PATCH net-next] net: assign err to 0 at begin in do_setlink()
>> function
>> 
>> From: yuan linyu <cugyly@163.com>
>> Date: Tue, 14 Nov 2017 22:30:59 +0800
>> 
>> > From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
>> >
>> > each netlink attribute have proper process when error happen,
>> > when exit on attribute process, it implies that no error,
>> > so err = 0; is useless.
>> >
>> > assign err = 0; at beginning if all attributes not set.
>> >
>> > Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
>> 
>> This change is not correct.
>> 
>> The IFLA_VF_PORTS code block can finish with err set non-zero.
> 
> Do you mean this block can return err = -EOPNOTSUPP;  ?
> It means nla_for_each_nested() in this block will never enter, right ?

Yes, that is what I mean.
Yuan, Linyu (NSB - CN/Shanghai) Nov. 15, 2017, 5:22 a.m. | #4
Thanks, I will provide a new patch

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of David Miller
> Sent: Wednesday, November 15, 2017 1:20 PM
> To: Yuan, Linyu (NSB - CN/Shanghai)
> Cc: cugyly@163.com; netdev@vger.kernel.org
> Subject: Re: [PATCH net-next] net: assign err to 0 at begin in do_setlink()
> function
> 
> From: "Yuan, Linyu (NSB - CN/Shanghai)" <linyu.yuan@nokia-sbell.com>
> Date: Wed, 15 Nov 2017 05:15:35 +0000
> 
> >
> >
> >> -----Original Message-----
> >> From: netdev-owner@vger.kernel.org
> [mailto:netdev-owner@vger.kernel.org]
> >> On Behalf Of David Miller
> >> Sent: Wednesday, November 15, 2017 1:08 PM
> >> To: cugyly@163.com
> >> Cc: netdev@vger.kernel.org; Yuan, Linyu (NSB - CN/Shanghai)
> >> Subject: Re: [PATCH net-next] net: assign err to 0 at begin in do_setlink()
> >> function
> >>
> >> From: yuan linyu <cugyly@163.com>
> >> Date: Tue, 14 Nov 2017 22:30:59 +0800
> >>
> >> > From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> >> >
> >> > each netlink attribute have proper process when error happen,
> >> > when exit on attribute process, it implies that no error,
> >> > so err = 0; is useless.
> >> >
> >> > assign err = 0; at beginning if all attributes not set.
> >> >
> >> > Signed-off-by: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
> >>
> >> This change is not correct.
> >>
> >> The IFLA_VF_PORTS code block can finish with err set non-zero.
> >
> > Do you mean this block can return err = -EOPNOTSUPP;  ?
> > It means nla_for_each_nested() in this block will never enter, right ?
> 
> Yes, that is what I mean.

Patch

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index dabba2a..4760249 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2075,7 +2075,7 @@  static int do_setlink(const struct sk_buff *skb,
 		      struct nlattr **tb, char *ifname, int status)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
-	int err;
+	int err = 0;
 
 	if (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD]) {
 		struct net *net = rtnl_link_get_net(dev_net(dev), tb);
@@ -2253,7 +2253,6 @@  static int do_setlink(const struct sk_buff *skb,
 			status |= DO_SETLINK_NOTIFY;
 		}
 	}
-	err = 0;
 
 	if (tb[IFLA_VF_PORTS]) {
 		struct nlattr *port[IFLA_PORT_MAX+1];
@@ -2286,7 +2285,6 @@  static int do_setlink(const struct sk_buff *skb,
 			status |= DO_SETLINK_NOTIFY;
 		}
 	}
-	err = 0;
 
 	if (tb[IFLA_PORT_SELF]) {
 		struct nlattr *port[IFLA_PORT_MAX+1];
@@ -2326,7 +2324,6 @@  static int do_setlink(const struct sk_buff *skb,
 			status |= DO_SETLINK_NOTIFY;
 		}
 	}
-	err = 0;
 
 	if (tb[IFLA_PROTO_DOWN]) {
 		err = dev_change_proto_down(dev,