[PATCH 0/3] Fixes to network device group configuration via rtnetlink

[PATCH 0/3] Fixes to network device group configuration via rtnetlink

[Sergey Popovich]

In this small patch series I present few fixes to the network device
group configuration via rtnetlink interface.

For more information on problems addressed, please see description
of each patch in the thread.

Sergey Popovich (3):
  rtnetlink: walk through all devs in netns safely on link params change
  rtnetlink: fix potential NULL pointer dereference
  rtnetlink: add IFLA_GROUP to ifla_policy

 net/core/rtnetlink.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

-- 
1.8.3.4

[PATCH 1/3] rtnetlink: walk through all devs in netns safely on link params change

[Sergey Popovich]

Changing parameters of group of network interfaces may lead to
system hang during change, caused by unsafe walking through
list of network interfaces in current network namespace.

This could be easily reproduced with following configuration:

  # ip netns add pc10
  # ip link add dev dummy128 group 128 type dummy
  # ip link set group 128 netns pc10

Fixes: e7ed828f10bd netlink: support setting devgroup parameters
Signed-off-by: Sergey Popovich <popovich_sergei@mail.ru>
---
 net/core/rtnetlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d4ff417..95258e2 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1782,10 +1782,10 @@ static int rtnl_group_changelink(struct net *net, int group,
 		struct ifinfomsg *ifm,
 		struct nlattr **tb)
 {
-	struct net_device *dev;
+	struct net_device *dev, *aux;
 	int err;
 
-	for_each_netdev(net, dev) {
+	for_each_netdev_safe(net, dev, aux) {
 		if (dev->group == group) {
 			err = do_setlink(dev, ifm, tb, NULL, 0);
 			if (err < 0)
-- 
1.8.3.4

[PATCH 2/3] rtnetlink: fix potential NULL pointer dereference

[Sergey Popovich]

Now since commit fbdc1e8c6b79 (rtnetlink: walk through all devs in
netns safely on link params change) group of network devices can
be moved into another network namespace, there is NULL pointer
dereference in the do_setlink() as it get's called from
rtnl_group_changelink() with ifname == NULL.

Fixes: e7ed828f10bd netlink: support setting devgroup parameters
Signed-off-by: Sergey Popovich <popovich_sergei@mail.ru>
---
 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 95258e2..188c060 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1484,7 +1484,7 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 	 * name provided implies that a name change has been
 	 * requested.
 	 */
-	if (ifm->ifi_index > 0 && ifname[0]) {
+	if (ifname && ifname[0] && ifm->ifi_index > 0) {
 		err = dev_change_name(dev, ifname);
 		if (err < 0)
 			goto errout;
-- 
1.8.3.4

[PATCH 3/3] rtnetlink: add IFLA_GROUP to ifla_policy

[Sergey Popovich]

Network interface groups support added while ago, however
there is no IFLA_GROUP attribute description in policy
until now.

Add IFLA_GROUP attribute to the policy.

Fixes: cbda10fa97d7 net_device: add support for network device groups

Signed-off-by: Sergey Popovich <popovich_sergei@mail.ru>
---
 net/core/rtnetlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 188c060..7ee8565 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -823,6 +823,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + nla_total_size(1) /* IFLA_OPERSTATE */
 	       + nla_total_size(1) /* IFLA_LINKMODE */
 	       + nla_total_size(4) /* IFLA_CARRIER_CHANGES */
+	       + nla_total_size(4) /* IFLA_GROUP */
 	       + nla_total_size(ext_filter_mask
 			        & RTEXT_FILTER_VF ? 4 : 0) /* IFLA_NUM_VF */
 	       + rtnl_vfinfo_size(dev, ext_filter_mask) /* IFLA_VFINFO_LIST */
@@ -1151,6 +1152,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_NUM_RX_QUEUES]	= { .type = NLA_U32 },
 	[IFLA_PHYS_PORT_ID]	= { .type = NLA_BINARY, .len = MAX_PHYS_PORT_ID_LEN },
 	[IFLA_CARRIER_CHANGES]	= { .type = NLA_U32 },  /* ignored */
+	[IFLA_GROUP]		= { .type = NLA_U32 },
 };
 
 static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
-- 
1.8.3.4

Re: [PATCH 2/3] rtnetlink: fix potential NULL pointer dereference

[Sergey Popovich]

В письме от 6 мая 2014 18:13:16 пользователь Sergey Popovich написал:
> Now since commit fbdc1e8c6b79 (rtnetlink: walk through all devs in
> netns safely on link params change) group of network devices can
> be moved into another network namespace, there is NULL pointer
> dereference in the do_setlink() as it get's called from
> rtnl_group_changelink() with ifname == NULL.
> 
> Fixes: e7ed828f10bd netlink: support setting devgroup parameters
> Signed-off-by: Sergey Popovich <popovich_sergei@mail.ru>

Not needed, self-rejected patch, ifm->ifi_index is == 0 when
calling from rtnl_group_changelink().

Sorry for noise.

> ---
>  net/core/rtnetlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 95258e2..188c060 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1484,7 +1484,7 @@ static int do_setlink(struct net_device *dev, struct
> ifinfomsg *ifm, * name provided implies that a name change has been
>  	 * requested.
>  	 */
> -	if (ifm->ifi_index > 0 && ifname[0]) {
> +	if (ifname && ifname[0] && ifm->ifi_index > 0) {
>  		err = dev_change_name(dev, ifname);
>  		if (err < 0)
>  			goto errout;

-- 
SP5474-RIPE
Sergey Popovich

Re: [PATCH 2/3] rtnetlink: fix potential NULL pointer dereference

[David Miller]

From: Sergey Popovich <popovich_sergei@mail.ru>
Date: Tue,  6 May 2014 18:13:16 +0300

> Now since commit fbdc1e8c6b79 (rtnetlink: walk through all devs in
> netns safely on link params change) group of network devices can

This is not how you do this, first of all that SHA ID doesn't exist
in any tree other than your's.  It's not an existing commit, but
rather one you are adding in this series.

And secondly, and even more importantly, your patch in this series
is adding this new NULL pointer case.

Therefore you should adjust your first patch to handle this properly
since it introduced the possibility of it happening in the first
place.