[PATCH net-next 00/10] use appropriate APIs to get interfaces

[PATCH net-next 00/10] use appropriate APIs to get interfaces

[Ying Xue]

Under rtnl_lock protection, we should use __dev_get_name/index()
rather than dev_get_name()/index() to find interface handlers
because the former interfaces can help us avoid to change interface
reference counter.

Ying Xue (10):
  Drivers: Staging: cxt1e1: use __dev_get_name instead of dev_get_name
    to find interfaces
  bonding: use __dev_get_by_name instead of dev_get_by_name to find
    interface
  eql: use __dev_get_by_name instead of dev_get_by_name to find
    interface
  dcb: use __dev_get_by_name instead of dev_get_by_name to find
    interface
  decnet: use __dev_get_by_index instead of dev_get_by_index to find
    interface
  vxlan: use __dev_get_by_index instead of dev_get_by_index to find
    interface
  batman-adv: use __dev_get_by_index instead of dev_get_by_index to
    find interface
  caif: __dev_get_by_index instead of dev_get_by_index to find
    interface
  can: use __dev_get_by_index instead of dev_get_by_index to find
    interface
  net: nl80211: __dev_get_by_index instead of dev_get_by_index to find
    interface

 drivers/net/bonding/bond_main.c |   49 +++++++++----------
 drivers/net/eql.c               |   95 ++++++++++++++++---------------------
 drivers/net/vxlan.c             |    3 +-
 drivers/staging/cxt1e1/linux.c  |   15 +++---
 net/batman-adv/hard-interface.c |    4 +-
 net/caif/chnl_net.c             |    3 +-
 net/can/gw.c                    |   15 ++----
 net/dcb/dcbnl.c                 |   15 ++----
 net/decnet/dn_route.c           |    6 +--
 net/wireless/nl80211.c          |  100 ++++++++++++++-------------------------
 10 files changed, 123 insertions(+), 182 deletions(-)

-- 
1.7.9.5

[PATCH net-next 01/10] Drivers: Staging: cxt1e1: use __dev_get_name instead of dev_get_name to find interfaces

[Ying Xue]

The following call chain denotes that both do_reset() and do_del_chan()
are protected under rtnl_lock. If we use __dev_get_by_name() instead of
dev_get_by_name() to find interface handlers in them, this would help
us avoid to change interface reference counter.

dev_ioctl()
  rtnl_lock()
  dev_ifsioc()
    c4_ioctl()
      do_reset()
      do_del_chan()

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 drivers/staging/cxt1e1/linux.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/cxt1e1/linux.c b/drivers/staging/cxt1e1/linux.c
index 9b48373..4a08e16 100644
--- a/drivers/staging/cxt1e1/linux.c
+++ b/drivers/staging/cxt1e1/linux.c
@@ -770,9 +770,9 @@ do_del_chan (struct net_device *musycc_dev, void *data)
     if (cp.channum > 999)
         return -EINVAL;
     snprintf (buf, sizeof(buf), CHANNAME "%d", cp.channum);
-    if (!(dev = dev_get_by_name (&init_net, buf)))
-        return -ENOENT;
-    dev_put (dev);
+	dev = __dev_get_by_name(&init_net, buf);
+	if (!dev)
+		return -ENODEV;
     ret = do_deluser (dev, 1);
     if (ret)
         return ret;
@@ -792,19 +792,18 @@ do_reset (struct net_device *musycc_dev, void *data)
         char        buf[sizeof (CHANNAME) + 3];
 
         sprintf (buf, CHANNAME "%d", i);
-        if (!(ndev = dev_get_by_name(&init_net, buf)))
-            continue;
+	ndev = __dev_get_by_name(&init_net, buf);
+	if (!ndev)
+		continue;
         priv = dev_to_hdlc (ndev)->priv;
 
         if ((unsigned long) (priv->ci) ==
             (unsigned long) (netdev_priv(musycc_dev)))
         {
             ndev->flags &= ~IFF_UP;
-            dev_put (ndev);
             netif_stop_queue (ndev);
             do_deluser (ndev, 1);
-        } else
-            dev_put (ndev);
+	}
     }
     return 0;
 }
-- 
1.7.9.5

[PATCH net-next 02/10] bonding: use __dev_get_by_name instead of dev_get_by_name to find interface

[Ying Xue]

The following call chain indicates that bond_do_ioctl() is protected
under rtnl_lock. If we use __dev_get_by_name() instead of
dev_get_by_name() to find interface handler in it, this would
help us avoid to change reference counter of interface once.

dev_ioctl()
  rtnl_lock()
  dev_ifsioc()
    bond_do_ioctl()
  rtnl_unlock()

Additionally we also change the coding style in bond_do_ioctl(),
letting it more readable for us.

Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 drivers/net/bonding/bond_main.c |   49 ++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 26 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e06c445..a69afbf 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3213,37 +3213,34 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
 	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
 		return -EPERM;
 
-	slave_dev = dev_get_by_name(net, ifr->ifr_slave);
+	slave_dev = __dev_get_by_name(net, ifr->ifr_slave);
 
 	pr_debug("slave_dev=%p:\n", slave_dev);
 
 	if (!slave_dev)
-		res = -ENODEV;
-	else {
-		pr_debug("slave_dev->name=%s:\n", slave_dev->name);
-		switch (cmd) {
-		case BOND_ENSLAVE_OLD:
-		case SIOCBONDENSLAVE:
-			res = bond_enslave(bond_dev, slave_dev);
-			break;
-		case BOND_RELEASE_OLD:
-		case SIOCBONDRELEASE:
-			res = bond_release(bond_dev, slave_dev);
-			break;
-		case BOND_SETHWADDR_OLD:
-		case SIOCBONDSETHWADDR:
-			bond_set_dev_addr(bond_dev, slave_dev);
-			res = 0;
-			break;
-		case BOND_CHANGE_ACTIVE_OLD:
-		case SIOCBONDCHANGEACTIVE:
-			res = bond_option_active_slave_set(bond, slave_dev);
-			break;
-		default:
-			res = -EOPNOTSUPP;
-		}
+		return -ENODEV;
 
-		dev_put(slave_dev);
+	pr_debug("slave_dev->name=%s:\n", slave_dev->name);
+	switch (cmd) {
+	case BOND_ENSLAVE_OLD:
+	case SIOCBONDENSLAVE:
+		res = bond_enslave(bond_dev, slave_dev);
+		break;
+	case BOND_RELEASE_OLD:
+	case SIOCBONDRELEASE:
+		res = bond_release(bond_dev, slave_dev);
+		break;
+	case BOND_SETHWADDR_OLD:
+	case SIOCBONDSETHWADDR:
+		bond_set_dev_addr(bond_dev, slave_dev);
+		res = 0;
+		break;
+	case BOND_CHANGE_ACTIVE_OLD:
+	case SIOCBONDCHANGEACTIVE:
+		res = bond_option_active_slave_set(bond, slave_dev);
+		break;
+	default:
+		res = -EOPNOTSUPP;
 	}
 
 	return res;
-- 
1.7.9.5

Re: [PATCH net-next 02/10] bonding: use __dev_get_by_name instead of dev_get_by_name to find interface

[Veaceslav Falico]

On Tue, Jan 14, 2014 at 03:41:01PM +0800, Ying Xue wrote:
>The following call chain indicates that bond_do_ioctl() is protected
>under rtnl_lock. If we use __dev_get_by_name() instead of
>dev_get_by_name() to find interface handler in it, this would
>help us avoid to change reference counter of interface once.
>
>dev_ioctl()
>  rtnl_lock()
>  dev_ifsioc()
>    bond_do_ioctl()
>  rtnl_unlock()
>
>Additionally we also change the coding style in bond_do_ioctl(),
>letting it more readable for us.
>
>Cc: Jay Vosburgh <fubar@us.ibm.com>
>Cc: Veaceslav Falico <vfalico@redhat.com>

Acked-by: Veaceslav Falico <vfalico@redhat.com>

>Signed-off-by: Ying Xue <ying.xue@windriver.com>
>---
> drivers/net/bonding/bond_main.c |   49 ++++++++++++++++++---------------------
> 1 file changed, 23 insertions(+), 26 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index e06c445..a69afbf 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3213,37 +3213,34 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
> 	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> 		return -EPERM;
>
>-	slave_dev = dev_get_by_name(net, ifr->ifr_slave);
>+	slave_dev = __dev_get_by_name(net, ifr->ifr_slave);
>
> 	pr_debug("slave_dev=%p:\n", slave_dev);
>
> 	if (!slave_dev)
>-		res = -ENODEV;
>-	else {
>-		pr_debug("slave_dev->name=%s:\n", slave_dev->name);
>-		switch (cmd) {
>-		case BOND_ENSLAVE_OLD:
>-		case SIOCBONDENSLAVE:
>-			res = bond_enslave(bond_dev, slave_dev);
>-			break;
>-		case BOND_RELEASE_OLD:
>-		case SIOCBONDRELEASE:
>-			res = bond_release(bond_dev, slave_dev);
>-			break;
>-		case BOND_SETHWADDR_OLD:
>-		case SIOCBONDSETHWADDR:
>-			bond_set_dev_addr(bond_dev, slave_dev);
>-			res = 0;
>-			break;
>-		case BOND_CHANGE_ACTIVE_OLD:
>-		case SIOCBONDCHANGEACTIVE:
>-			res = bond_option_active_slave_set(bond, slave_dev);
>-			break;
>-		default:
>-			res = -EOPNOTSUPP;
>-		}
>+		return -ENODEV;
>
>-		dev_put(slave_dev);
>+	pr_debug("slave_dev->name=%s:\n", slave_dev->name);
>+	switch (cmd) {
>+	case BOND_ENSLAVE_OLD:
>+	case SIOCBONDENSLAVE:
>+		res = bond_enslave(bond_dev, slave_dev);
>+		break;
>+	case BOND_RELEASE_OLD:
>+	case SIOCBONDRELEASE:
>+		res = bond_release(bond_dev, slave_dev);
>+		break;
>+	case BOND_SETHWADDR_OLD:
>+	case SIOCBONDSETHWADDR:
>+		bond_set_dev_addr(bond_dev, slave_dev);
>+		res = 0;
>+		break;
>+	case BOND_CHANGE_ACTIVE_OLD:
>+	case SIOCBONDCHANGEACTIVE:
>+		res = bond_option_active_slave_set(bond, slave_dev);
>+		break;
>+	default:
>+		res = -EOPNOTSUPP;
> 	}
>
> 	return res;
>-- 
>1.7.9.5
>

[PATCH net-next 03/10] eql: use __dev_get_by_name instead of dev_get_by_name to find interface

[Ying Xue]

The following call chain indicates that eql_ioctl(), eql_enslave(),
eql_emancipate(), eql_g_slave_cfg() and eql_s_slave_cfg() are
protected under rtnl_lock. So if we use __dev_get_by_name() instead
of dev_get_by_name() to find interface handlers in them, this would
help us avoid to change interface reference counters.

dev_ioctl()
  rtnl_lock()
    dev_ifsioc()
      eql_ioctl()
        eql_enslave()
	eql_emancipate()
	eql_g_slave_cfg()
	eql_s_slave_cfg()
  rtnl_unlock()

Additionally we also change their return values from -EINVAL to
-ENODEV in case that interfaces are no found.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 drivers/net/eql.c |   95 +++++++++++++++++++++++------------------------------
 1 file changed, 42 insertions(+), 53 deletions(-)

diff --git a/drivers/net/eql.c b/drivers/net/eql.c
index f219d38..7a79b60 100644
--- a/drivers/net/eql.c
+++ b/drivers/net/eql.c
@@ -395,6 +395,7 @@ static int __eql_insert_slave(slave_queue_t *queue, slave_t *slave)
 		if (duplicate_slave)
 			eql_kill_one_slave(queue, duplicate_slave);
 
+		dev_hold(slave->dev);
 		list_add(&slave->list, &queue->all_slaves);
 		queue->num_slaves++;
 		slave->dev->flags |= IFF_SLAVE;
@@ -413,39 +414,35 @@ static int eql_enslave(struct net_device *master_dev, slaving_request_t __user *
 	if (copy_from_user(&srq, srqp, sizeof (slaving_request_t)))
 		return -EFAULT;
 
-	slave_dev  = dev_get_by_name(&init_net, srq.slave_name);
-	if (slave_dev) {
-		if ((master_dev->flags & IFF_UP) == IFF_UP) {
-			/* slave is not a master & not already a slave: */
-			if (!eql_is_master(slave_dev) &&
-			    !eql_is_slave(slave_dev)) {
-				slave_t *s = kmalloc(sizeof(*s), GFP_KERNEL);
-				equalizer_t *eql = netdev_priv(master_dev);
-				int ret;
-
-				if (!s) {
-					dev_put(slave_dev);
-					return -ENOMEM;
-				}
-
-				memset(s, 0, sizeof(*s));
-				s->dev = slave_dev;
-				s->priority = srq.priority;
-				s->priority_bps = srq.priority;
-				s->priority_Bps = srq.priority / 8;
-
-				spin_lock_bh(&eql->queue.lock);
-				ret = __eql_insert_slave(&eql->queue, s);
-				if (ret) {
-					dev_put(slave_dev);
-					kfree(s);
-				}
-				spin_unlock_bh(&eql->queue.lock);
-
-				return ret;
-			}
+	slave_dev = __dev_get_by_name(&init_net, srq.slave_name);
+	if (!slave_dev)
+		return -ENODEV;
+
+	if ((master_dev->flags & IFF_UP) == IFF_UP) {
+		/* slave is not a master & not already a slave: */
+		if (!eql_is_master(slave_dev) && !eql_is_slave(slave_dev)) {
+			slave_t *s = kmalloc(sizeof(*s), GFP_KERNEL);
+			equalizer_t *eql = netdev_priv(master_dev);
+			int ret;
+
+			if (!s)
+				return -ENOMEM;
+
+			memset(s, 0, sizeof(*s));
+			s->dev = slave_dev;
+			s->priority = srq.priority;
+			s->priority_bps = srq.priority;
+			s->priority_Bps = srq.priority / 8;
+
+			spin_lock_bh(&eql->queue.lock);
+			ret = __eql_insert_slave(&eql->queue, s);
+			if (ret)
+				kfree(s);
+
+			spin_unlock_bh(&eql->queue.lock);
+
+			return ret;
 		}
-		dev_put(slave_dev);
 	}
 
 	return -EINVAL;
@@ -461,24 +458,20 @@ static int eql_emancipate(struct net_device *master_dev, slaving_request_t __use
 	if (copy_from_user(&srq, srqp, sizeof (slaving_request_t)))
 		return -EFAULT;
 
-	slave_dev = dev_get_by_name(&init_net, srq.slave_name);
-	ret = -EINVAL;
-	if (slave_dev) {
-		spin_lock_bh(&eql->queue.lock);
-
-		if (eql_is_slave(slave_dev)) {
-			slave_t *slave = __eql_find_slave_dev(&eql->queue,
-							      slave_dev);
+	slave_dev = __dev_get_by_name(&init_net, srq.slave_name);
+	if (!slave_dev)
+		return -ENODEV;
 
-			if (slave) {
-				eql_kill_one_slave(&eql->queue, slave);
-				ret = 0;
-			}
+	ret = -EINVAL;
+	spin_lock_bh(&eql->queue.lock);
+	if (eql_is_slave(slave_dev)) {
+		slave_t *slave = __eql_find_slave_dev(&eql->queue, slave_dev);
+		if (slave) {
+			eql_kill_one_slave(&eql->queue, slave);
+			ret = 0;
 		}
-		dev_put(slave_dev);
-
-		spin_unlock_bh(&eql->queue.lock);
 	}
+	spin_unlock_bh(&eql->queue.lock);
 
 	return ret;
 }
@@ -494,7 +487,7 @@ static int eql_g_slave_cfg(struct net_device *dev, slave_config_t __user *scp)
 	if (copy_from_user(&sc, scp, sizeof (slave_config_t)))
 		return -EFAULT;
 
-	slave_dev = dev_get_by_name(&init_net, sc.slave_name);
+	slave_dev = __dev_get_by_name(&init_net, sc.slave_name);
 	if (!slave_dev)
 		return -ENODEV;
 
@@ -510,8 +503,6 @@ static int eql_g_slave_cfg(struct net_device *dev, slave_config_t __user *scp)
 	}
 	spin_unlock_bh(&eql->queue.lock);
 
-	dev_put(slave_dev);
-
 	if (!ret && copy_to_user(scp, &sc, sizeof (slave_config_t)))
 		ret = -EFAULT;
 
@@ -529,7 +520,7 @@ static int eql_s_slave_cfg(struct net_device *dev, slave_config_t __user *scp)
 	if (copy_from_user(&sc, scp, sizeof (slave_config_t)))
 		return -EFAULT;
 
-	slave_dev = dev_get_by_name(&init_net, sc.slave_name);
+	slave_dev = __dev_get_by_name(&init_net, sc.slave_name);
 	if (!slave_dev)
 		return -ENODEV;
 
@@ -548,8 +539,6 @@ static int eql_s_slave_cfg(struct net_device *dev, slave_config_t __user *scp)
 	}
 	spin_unlock_bh(&eql->queue.lock);
 
-	dev_put(slave_dev);
-
 	return ret;
 }
 
-- 
1.7.9.5

[PATCH net-next 04/10] dcb: use __dev_get_by_name instead of dev_get_by_name to find interface

[Ying Xue]

The following call chain indicates that dcb_doit() is protected
under rtnl_lock. So if we use __dev_get_by_name() instead of
dev_get_by_name() to find interface handlers in it, this would
help us avoid to change interface reference counter.

rtnetlink_rcv()
  rtnl_lock()
  netlink_rcv_skb()
    dcb_doit()
  rtnl_unlock()

Cc: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/dcb/dcbnl.c |   15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
index 66fbe19..5536444 100644
--- a/net/dcb/dcbnl.c
+++ b/net/dcb/dcbnl.c
@@ -1688,21 +1688,17 @@ static int dcb_doit(struct sk_buff *skb, struct nlmsghdr *nlh)
 	if (!tb[DCB_ATTR_IFNAME])
 		return -EINVAL;
 
-	netdev = dev_get_by_name(net, nla_data(tb[DCB_ATTR_IFNAME]));
+	netdev = __dev_get_by_name(net, nla_data(tb[DCB_ATTR_IFNAME]));
 	if (!netdev)
 		return -ENODEV;
 
-	if (!netdev->dcbnl_ops) {
-		ret = -EOPNOTSUPP;
-		goto out;
-	}
+	if (!netdev->dcbnl_ops)
+		return -EOPNOTSUPP;
 
 	reply_skb = dcbnl_newmsg(fn->type, dcb->cmd, portid, nlh->nlmsg_seq,
 				 nlh->nlmsg_flags, &reply_nlh);
-	if (!reply_skb) {
-		ret = -ENOBUFS;
-		goto out;
-	}
+	if (!reply_skb)
+		return -ENOBUFS;
 
 	ret = fn->cb(netdev, nlh, nlh->nlmsg_seq, tb, reply_skb);
 	if (ret < 0) {
@@ -1714,7 +1710,6 @@ static int dcb_doit(struct sk_buff *skb, struct nlmsghdr *nlh)
 
 	ret = rtnl_unicast(reply_skb, net, portid);
 out:
-	dev_put(netdev);
 	return ret;
 }
 
-- 
1.7.9.5

[PATCH net-next 05/10] decnet: use __dev_get_by_index instead of dev_get_by_index to find interface

[Ying Xue]

The following call chain we can identify that dn_cache_getroute() is
protected under rtnl_lock. So if we use __dev_get_by_index() instead
of dev_get_by_index() to find interface handlers in it, this would help
us avoid to change interface reference counter.

rtnetlink_rcv()
  rtnl_lock()
    netlink_rcv_skb()
      dn_cache_getroute()
  rtnl_unlock()

Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/decnet/dn_route.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
index ad2efa5..22390e4 100644
--- a/net/decnet/dn_route.c
+++ b/net/decnet/dn_route.c
@@ -1666,12 +1666,12 @@ static int dn_cache_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh)
 
 	if (fld.flowidn_iif) {
 		struct net_device *dev;
-		if ((dev = dev_get_by_index(&init_net, fld.flowidn_iif)) == NULL) {
+		dev = __dev_get_by_index(&init_net, fld.flowidn_iif);
+		if (!dev) {
 			kfree_skb(skb);
 			return -ENODEV;
 		}
 		if (!dev->dn_ptr) {
-			dev_put(dev);
 			kfree_skb(skb);
 			return -ENODEV;
 		}
@@ -1693,8 +1693,6 @@ static int dn_cache_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh)
 		err = dn_route_output_key((struct dst_entry **)&rt, &fld, 0);
 	}
 
-	if (skb->dev)
-		dev_put(skb->dev);
 	skb->dev = NULL;
 	if (err)
 		goto out_free;
-- 
1.7.9.5

[PATCH net-next 06/10] vxlan: use __dev_get_by_index instead of dev_get_by_index to find interface

[Ying Xue]

The following call chains indicate that vxlan_fdb_parse() is
under rtnl_lock protection. So if we use __dev_get_by_index()
instead of dev_get_by_index() to find interface handler in it,
this would help us avoid to change interface reference counter.

rtnetlink_rcv()
  rtnl_lock()
  netlink_rcv_skb()
    rtnl_fdb_add()
      vxlan_fdb_add()
        vxlan_fdb_parse()
  rtnl_unlock()

rtnetlink_rcv()
  rtnl_lock()
  netlink_rcv_skb()
    rtnl_fdb_del()
      vxlan_fdb_del()
        vxlan_fdb_parse()
  rtnl_unlock()

Cc: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 drivers/net/vxlan.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 481f85d..8c40802 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -741,10 +741,9 @@ static int vxlan_fdb_parse(struct nlattr *tb[], struct vxlan_dev *vxlan,
 		if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32))
 			return -EINVAL;
 		*ifindex = nla_get_u32(tb[NDA_IFINDEX]);
-		tdev = dev_get_by_index(net, *ifindex);
+		tdev = __dev_get_by_index(net, *ifindex);
 		if (!tdev)
 			return -EADDRNOTAVAIL;
-		dev_put(tdev);
 	} else {
 		*ifindex = 0;
 	}
-- 
1.7.9.5

Re: [PATCH net-next 06/10] vxlan: use __dev_get_by_index instead of dev_get_by_index to find interface

[Stephen Hemminger]

On Tue, 14 Jan 2014 15:41:05 +0800
Ying Xue <ying.xue@windriver.com> wrote:

> The following call chains indicate that vxlan_fdb_parse() is
> under rtnl_lock protection. So if we use __dev_get_by_index()
> instead of dev_get_by_index() to find interface handler in it,
> this would help us avoid to change interface reference counter.
> 
> rtnetlink_rcv()
>   rtnl_lock()
>   netlink_rcv_skb()
>     rtnl_fdb_add()
>       vxlan_fdb_add()
>         vxlan_fdb_parse()
>   rtnl_unlock()
> 
> rtnetlink_rcv()
>   rtnl_lock()
>   netlink_rcv_skb()
>     rtnl_fdb_del()
>       vxlan_fdb_del()
>         vxlan_fdb_parse()
>   rtnl_unlock()
> 
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Ying Xue <ying.xue@windriver.com>

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH net-next 07/10] batman-adv: use __dev_get_by_index instead of dev_get_by_index to find interface

[Ying Xue]

The following call chains indicate that batadv_is_on_batman_iface()
is always under rtnl_lock protection as call_netdevice_notifier()
is protected by rtnl_lock. So if __dev_get_by_index() rather than
dev_get_by_index() is used to find interface handler in it, this
would help us avoid to change interface reference counter.

call_netdevice_notifier()
  batadv_hard_if_event()
    batadv_hardif_add_interface()
      batadv_is_valid_iface()
        batadv_is_on_batman_iface()

Cc: Antonio Quartulli <antonio@meshcoding.com>
Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/batman-adv/hard-interface.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
index bebd46c..115d14e 100644
--- a/net/batman-adv/hard-interface.c
+++ b/net/batman-adv/hard-interface.c
@@ -86,15 +86,13 @@ static bool batadv_is_on_batman_iface(const struct net_device *net_dev)
 		return false;
 
 	/* recurse over the parent device */
-	parent_dev = dev_get_by_index(&init_net, net_dev->iflink);
+	parent_dev = __dev_get_by_index(&init_net, net_dev->iflink);
 	/* if we got a NULL parent_dev there is something broken.. */
 	if (WARN(!parent_dev, "Cannot find parent device"))
 		return false;
 
 	ret = batadv_is_on_batman_iface(parent_dev);
 
-	if (parent_dev)
-		dev_put(parent_dev);
 	return ret;
 }
 
-- 
1.7.9.5

Re: [PATCH net-next 07/10] batman-adv: use __dev_get_by_index instead of dev_get_by_index to find interface

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

On 14/01/14 08:41, Ying Xue wrote:
> The following call chains indicate that batadv_is_on_batman_iface()
> is always under rtnl_lock protection as call_netdevice_notifier()
> is protected by rtnl_lock. So if __dev_get_by_index() rather than
> dev_get_by_index() is used to find interface handler in it, this
> would help us avoid to change interface reference counter.
> 
> call_netdevice_notifier()
>   batadv_hard_if_event()
>     batadv_hardif_add_interface()
>       batadv_is_valid_iface()
>         batadv_is_on_batman_iface()
> 
> Cc: Antonio Quartulli <antonio@meshcoding.com>
> Signed-off-by: Ying Xue <ying.xue@windriver.com>

Acked-by: Antonio Quartulli <antonio@meshcoding.com>


> ---
>  net/batman-adv/hard-interface.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
> index bebd46c..115d14e 100644
> --- a/net/batman-adv/hard-interface.c
> +++ b/net/batman-adv/hard-interface.c
> @@ -86,15 +86,13 @@ static bool batadv_is_on_batman_iface(const struct net_device *net_dev)
>  		return false;
>  
>  	/* recurse over the parent device */
> -	parent_dev = dev_get_by_index(&init_net, net_dev->iflink);
> +	parent_dev = __dev_get_by_index(&init_net, net_dev->iflink);
>  	/* if we got a NULL parent_dev there is something broken.. */
>  	if (WARN(!parent_dev, "Cannot find parent device"))
>  		return false;
>  
>  	ret = batadv_is_on_batman_iface(parent_dev);
>  
> -	if (parent_dev)
> -		dev_put(parent_dev);
>  	return ret;
>  }
>  
> 


-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH net-next 08/10] caif: __dev_get_by_index instead of dev_get_by_index to find interface

[Ying Xue]

The following call chains indicate that chnl_net_open() is under
rtnl_lock protection as __dev_open() is protected by rtnl_lock.
So if __dev_get_by_index() instead of dev_get_by_index() is used
to find interface handler in it, this would help us avoid to change
interface reference counter.

__dev_open()
  chnl_net_open()

Cc: Dmitry Tarnyagin <dmitry.tarnyagin@lockless.no>
Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/caif/chnl_net.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/caif/chnl_net.c b/net/caif/chnl_net.c
index 7344a8f..4589ff67 100644
--- a/net/caif/chnl_net.c
+++ b/net/caif/chnl_net.c
@@ -285,7 +285,7 @@ static int chnl_net_open(struct net_device *dev)
 				goto error;
 		}
 
-		lldev = dev_get_by_index(dev_net(dev), llifindex);
+		lldev = __dev_get_by_index(dev_net(dev), llifindex);
 
 		if (lldev == NULL) {
 			pr_debug("no interface?\n");
@@ -307,7 +307,6 @@ static int chnl_net_open(struct net_device *dev)
 		mtu = min_t(int, dev->mtu, lldev->mtu - (headroom + tailroom));
 		mtu = min_t(int, GPRS_PDP_MTU, mtu);
 		dev_set_mtu(dev, mtu);
-		dev_put(lldev);
 
 		if (mtu < 100) {
 			pr_warn("CAIF Interface MTU too small (%d)\n", mtu);
-- 
1.7.9.5

[PATCH net-next 09/10] can: use __dev_get_by_index instead of dev_get_by_index to find interface

[Ying Xue]

As cgw_create_job() is always under rtnl_lock protection,
__dev_get_by_index() instead of dev_get_by_index() should be used to
find interface handler in it having us avoid to change interface
reference counter.

Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/can/gw.c |   15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/net/can/gw.c b/net/can/gw.c
index 88c8a39..ac31891 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -839,21 +839,21 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh)
 	if (!gwj->ccgw.src_idx || !gwj->ccgw.dst_idx)
 		goto out;
 
-	gwj->src.dev = dev_get_by_index(&init_net, gwj->ccgw.src_idx);
+	gwj->src.dev = __dev_get_by_index(&init_net, gwj->ccgw.src_idx);
 
 	if (!gwj->src.dev)
 		goto out;
 
 	if (gwj->src.dev->type != ARPHRD_CAN)
-		goto put_src_out;
+		goto out;
 
-	gwj->dst.dev = dev_get_by_index(&init_net, gwj->ccgw.dst_idx);
+	gwj->dst.dev = __dev_get_by_index(&init_net, gwj->ccgw.dst_idx);
 
 	if (!gwj->dst.dev)
-		goto put_src_out;
+		goto out;
 
 	if (gwj->dst.dev->type != ARPHRD_CAN)
-		goto put_src_dst_out;
+		goto out;
 
 	gwj->limit_hops = limhops;
 
@@ -862,11 +862,6 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh)
 	err = cgw_register_filter(gwj);
 	if (!err)
 		hlist_add_head_rcu(&gwj->list, &cgw_list);
-
-put_src_dst_out:
-	dev_put(gwj->dst.dev);
-put_src_out:
-	dev_put(gwj->src.dev);
 out:
 	if (err)
 		kmem_cache_free(cgw_cache, gwj);
-- 
1.7.9.5

Re: [PATCH net-next 09/10] can: use __dev_get_by_index instead of dev_get_by_index to find interface

[Oliver Hartkopp]

On 14.01.2014 08:41, Ying Xue wrote:
> As cgw_create_job() is always under rtnl_lock protection,
> __dev_get_by_index() instead of dev_get_by_index() should be used to
> find interface handler in it having us avoid to change interface
> reference counter.
> 
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Signed-off-by: Ying Xue <ying.xue@windriver.com>

Thanks for the simplification!

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>


> ---
>  net/can/gw.c |   15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/net/can/gw.c b/net/can/gw.c
> index 88c8a39..ac31891 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -839,21 +839,21 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh)
>  	if (!gwj->ccgw.src_idx || !gwj->ccgw.dst_idx)
>  		goto out;
>  
> -	gwj->src.dev = dev_get_by_index(&init_net, gwj->ccgw.src_idx);
> +	gwj->src.dev = __dev_get_by_index(&init_net, gwj->ccgw.src_idx);
>  
>  	if (!gwj->src.dev)
>  		goto out;
>  
>  	if (gwj->src.dev->type != ARPHRD_CAN)
> -		goto put_src_out;
> +		goto out;
>  
> -	gwj->dst.dev = dev_get_by_index(&init_net, gwj->ccgw.dst_idx);
> +	gwj->dst.dev = __dev_get_by_index(&init_net, gwj->ccgw.dst_idx);
>  
>  	if (!gwj->dst.dev)
> -		goto put_src_out;
> +		goto out;
>  
>  	if (gwj->dst.dev->type != ARPHRD_CAN)
> -		goto put_src_dst_out;
> +		goto out;
>  
>  	gwj->limit_hops = limhops;
>  
> @@ -862,11 +862,6 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh)
>  	err = cgw_register_filter(gwj);
>  	if (!err)
>  		hlist_add_head_rcu(&gwj->list, &cgw_list);
> -
> -put_src_dst_out:
> -	dev_put(gwj->dst.dev);
> -put_src_out:
> -	dev_put(gwj->src.dev);
>  out:
>  	if (err)
>  		kmem_cache_free(cgw_cache, gwj);
> 

[PATCH net-next 10/10] net: nl80211: __dev_get_by_index instead of dev_get_by_index to find interface

[Ying Xue]

As __cfg80211_rdev_from_attrs(), nl80211_dump_wiphy_parse() and
nl80211_set_wiphy() are all under rtnl_lock protection,
__dev_get_by_index() instead of dev_get_by_index() should be used
to find interface handler in them allowing us to avoid to change
interface reference counter.

Cc: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Ying Xue <ying.xue@windriver.com>
---
 net/wireless/nl80211.c |  100 +++++++++++++++++-------------------------------
 1 file changed, 36 insertions(+), 64 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index b4f40fe..0d4d7fd 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -165,7 +165,7 @@ __cfg80211_rdev_from_attrs(struct net *netns, struct nlattr **attrs)
 
 	if (attrs[NL80211_ATTR_IFINDEX]) {
 		int ifindex = nla_get_u32(attrs[NL80211_ATTR_IFINDEX]);
-		netdev = dev_get_by_index(netns, ifindex);
+		netdev = __dev_get_by_index(netns, ifindex);
 		if (netdev) {
 			if (netdev->ieee80211_ptr)
 				tmp = wiphy_to_dev(
@@ -173,8 +173,6 @@ __cfg80211_rdev_from_attrs(struct net *netns, struct nlattr **attrs)
 			else
 				tmp = NULL;
 
-			dev_put(netdev);
-
 			/* not wireless device -- return error */
 			if (!tmp)
 				return ERR_PTR(-EINVAL);
@@ -1656,7 +1654,7 @@ static int nl80211_dump_wiphy_parse(struct sk_buff *skb,
 		struct cfg80211_registered_device *rdev;
 		int ifidx = nla_get_u32(tb[NL80211_ATTR_IFINDEX]);
 
-		netdev = dev_get_by_index(sock_net(skb->sk), ifidx);
+		netdev = __dev_get_by_index(sock_net(skb->sk), ifidx);
 		if (!netdev)
 			return -ENODEV;
 		if (netdev->ieee80211_ptr) {
@@ -1664,7 +1662,6 @@ static int nl80211_dump_wiphy_parse(struct sk_buff *skb,
 				netdev->ieee80211_ptr->wiphy);
 			state->filter_wiphy = rdev->wiphy_idx;
 		}
-		dev_put(netdev);
 	}
 
 	return 0;
@@ -1987,7 +1984,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 	if (info->attrs[NL80211_ATTR_IFINDEX]) {
 		int ifindex = nla_get_u32(info->attrs[NL80211_ATTR_IFINDEX]);
 
-		netdev = dev_get_by_index(genl_info_net(info), ifindex);
+		netdev = __dev_get_by_index(genl_info_net(info), ifindex);
 		if (netdev && netdev->ieee80211_ptr)
 			rdev = wiphy_to_dev(netdev->ieee80211_ptr->wiphy);
 		else
@@ -2015,32 +2012,24 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 			rdev, nla_data(info->attrs[NL80211_ATTR_WIPHY_NAME]));
 
 	if (result)
-		goto bad_res;
+		return result;
 
 	if (info->attrs[NL80211_ATTR_WIPHY_TXQ_PARAMS]) {
 		struct ieee80211_txq_params txq_params;
 		struct nlattr *tb[NL80211_TXQ_ATTR_MAX + 1];
 
-		if (!rdev->ops->set_txq_params) {
-			result = -EOPNOTSUPP;
-			goto bad_res;
-		}
+		if (!rdev->ops->set_txq_params)
+			return -EOPNOTSUPP;
 
-		if (!netdev) {
-			result = -EINVAL;
-			goto bad_res;
-		}
+		if (!netdev)
+			return -EINVAL;
 
 		if (netdev->ieee80211_ptr->iftype != NL80211_IFTYPE_AP &&
-		    netdev->ieee80211_ptr->iftype != NL80211_IFTYPE_P2P_GO) {
-			result = -EINVAL;
-			goto bad_res;
-		}
+		    netdev->ieee80211_ptr->iftype != NL80211_IFTYPE_P2P_GO)
+			return -EINVAL;
 
-		if (!netif_running(netdev)) {
-			result = -ENETDOWN;
-			goto bad_res;
-		}
+		if (!netif_running(netdev))
+			return -ENETDOWN;
 
 		nla_for_each_nested(nl_txq_params,
 				    info->attrs[NL80211_ATTR_WIPHY_TXQ_PARAMS],
@@ -2051,12 +2040,12 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 				  txq_params_policy);
 			result = parse_txq_params(tb, &txq_params);
 			if (result)
-				goto bad_res;
+				return result;
 
 			result = rdev_set_txq_params(rdev, netdev,
 						     &txq_params);
 			if (result)
-				goto bad_res;
+				return result;
 		}
 	}
 
@@ -2065,7 +2054,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 				nl80211_can_set_dev_channel(wdev) ? wdev : NULL,
 				info);
 		if (result)
-			goto bad_res;
+			return result;
 	}
 
 	if (info->attrs[NL80211_ATTR_WIPHY_TX_POWER_SETTING]) {
@@ -2076,19 +2065,15 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 		if (!(rdev->wiphy.features & NL80211_FEATURE_VIF_TXPOWER))
 			txp_wdev = NULL;
 
-		if (!rdev->ops->set_tx_power) {
-			result = -EOPNOTSUPP;
-			goto bad_res;
-		}
+		if (!rdev->ops->set_tx_power)
+			return -EOPNOTSUPP;
 
 		idx = NL80211_ATTR_WIPHY_TX_POWER_SETTING;
 		type = nla_get_u32(info->attrs[idx]);
 
 		if (!info->attrs[NL80211_ATTR_WIPHY_TX_POWER_LEVEL] &&
-		    (type != NL80211_TX_POWER_AUTOMATIC)) {
-			result = -EINVAL;
-			goto bad_res;
-		}
+		    (type != NL80211_TX_POWER_AUTOMATIC))
+			return -EINVAL;
 
 		if (type != NL80211_TX_POWER_AUTOMATIC) {
 			idx = NL80211_ATTR_WIPHY_TX_POWER_LEVEL;
@@ -2097,7 +2082,7 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 
 		result = rdev_set_tx_power(rdev, txp_wdev, type, mbm);
 		if (result)
-			goto bad_res;
+			return result;
 	}
 
 	if (info->attrs[NL80211_ATTR_WIPHY_ANTENNA_TX] &&
@@ -2105,10 +2090,8 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 		u32 tx_ant, rx_ant;
 		if ((!rdev->wiphy.available_antennas_tx &&
 		     !rdev->wiphy.available_antennas_rx) ||
-		    !rdev->ops->set_antenna) {
-			result = -EOPNOTSUPP;
-			goto bad_res;
-		}
+		    !rdev->ops->set_antenna)
+			return -EOPNOTSUPP;
 
 		tx_ant = nla_get_u32(info->attrs[NL80211_ATTR_WIPHY_ANTENNA_TX]);
 		rx_ant = nla_get_u32(info->attrs[NL80211_ATTR_WIPHY_ANTENNA_RX]);
@@ -2116,17 +2099,15 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 		/* reject antenna configurations which don't match the
 		 * available antenna masks, except for the "all" mask */
 		if ((~tx_ant && (tx_ant & ~rdev->wiphy.available_antennas_tx)) ||
-		    (~rx_ant && (rx_ant & ~rdev->wiphy.available_antennas_rx))) {
-			result = -EINVAL;
-			goto bad_res;
-		}
+		    (~rx_ant && (rx_ant & ~rdev->wiphy.available_antennas_rx)))
+			return -EINVAL;
 
 		tx_ant = tx_ant & rdev->wiphy.available_antennas_tx;
 		rx_ant = rx_ant & rdev->wiphy.available_antennas_rx;
 
 		result = rdev_set_antenna(rdev, tx_ant, rx_ant);
 		if (result)
-			goto bad_res;
+			return result;
 	}
 
 	changed = 0;
@@ -2134,30 +2115,27 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 	if (info->attrs[NL80211_ATTR_WIPHY_RETRY_SHORT]) {
 		retry_short = nla_get_u8(
 			info->attrs[NL80211_ATTR_WIPHY_RETRY_SHORT]);
-		if (retry_short == 0) {
-			result = -EINVAL;
-			goto bad_res;
-		}
+		if (retry_short == 0)
+			return -EINVAL;
+
 		changed |= WIPHY_PARAM_RETRY_SHORT;
 	}
 
 	if (info->attrs[NL80211_ATTR_WIPHY_RETRY_LONG]) {
 		retry_long = nla_get_u8(
 			info->attrs[NL80211_ATTR_WIPHY_RETRY_LONG]);
-		if (retry_long == 0) {
-			result = -EINVAL;
-			goto bad_res;
-		}
+		if (retry_long == 0)
+			return -EINVAL;
+
 		changed |= WIPHY_PARAM_RETRY_LONG;
 	}
 
 	if (info->attrs[NL80211_ATTR_WIPHY_FRAG_THRESHOLD]) {
 		frag_threshold = nla_get_u32(
 			info->attrs[NL80211_ATTR_WIPHY_FRAG_THRESHOLD]);
-		if (frag_threshold < 256) {
-			result = -EINVAL;
-			goto bad_res;
-		}
+		if (frag_threshold < 256)
+			return -EINVAL;
+
 		if (frag_threshold != (u32) -1) {
 			/*
 			 * Fragments (apart from the last one) are required to
@@ -2187,10 +2165,8 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 		u32 old_frag_threshold, old_rts_threshold;
 		u8 old_coverage_class;
 
-		if (!rdev->ops->set_wiphy_params) {
-			result = -EOPNOTSUPP;
-			goto bad_res;
-		}
+		if (!rdev->ops->set_wiphy_params)
+			return -EOPNOTSUPP;
 
 		old_retry_short = rdev->wiphy.retry_short;
 		old_retry_long = rdev->wiphy.retry_long;
@@ -2218,10 +2194,6 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
 			rdev->wiphy.coverage_class = old_coverage_class;
 		}
 	}
-
- bad_res:
-	if (netdev)
-		dev_put(netdev);
 	return result;
 }
 
-- 
1.7.9.5

Re: [PATCH net-next 10/10] net: nl80211: __dev_get_by_index instead of dev_get_by_index to find interface

[Johannes Berg]

On Tue, 2014-01-14 at 15:41 +0800, Ying Xue wrote:

> @@ -2218,10 +2194,6 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
>  			rdev->wiphy.coverage_class = old_coverage_class;
>  		}
>  	}
> -
> - bad_res:
> -	if (netdev)
> -		dev_put(netdev);
>  	return result;

I believe that could be "return 0;" now, which would make the code
easier to read.

johannes

Re: [PATCH net-next 10/10] net: nl80211: __dev_get_by_index instead of dev_get_by_index to find interface

[Ying Xue]

On 01/14/2014 08:11 PM, Johannes Berg wrote:
> On Tue, 2014-01-14 at 15:41 +0800, Ying Xue wrote:
> 
>> @@ -2218,10 +2194,6 @@ static int nl80211_set_wiphy(struct sk_buff *skb, struct genl_info *info)
>>  			rdev->wiphy.coverage_class = old_coverage_class;
>>  		}
>>  	}
>> -
>> - bad_res:
>> -	if (netdev)
>> -		dev_put(netdev);
>>  	return result;
> 
> I believe that could be "return 0;" now, which would make the code
> easier to read.
> 

Yes, I will change it in next version.

Thanks,
Ying

> johannes
> 
> 
>