On 7/6/15 10:37 AM, Nikolay Aleksandrov wrote:
+static int vrf_add_slave(struct net_device *dev,
+ struct net_device *port_dev)
+{
+ if (!dev || !port_dev || dev_net(dev) != dev_net(port_dev))
+ return -ENODEV;
+
+ if (!vrf_is_master(port_dev) && !vrf_is_slave(port_dev)) {
+ struct slave *s = kzalloc(sizeof(*s), GFP_KERNEL);
+ struct net_vrf *vrf = netdev_priv(dev);
+ struct slave_queue *queue = &vrf->queue;
+ bool is_running = netif_running(port_dev);
+ unsigned int flags = port_dev->flags;
+ int ret;
+
+ if (!s)
+ return -ENOMEM;
+
+ s->dev = port_dev;
+
+ spin_lock_bh(&queue->lock);
+ __vrf_insert_slave(queue, s, dev);
+ spin_unlock_bh(&queue->lock);
+
+ port_dev->vrf_ptr = kmalloc(sizeof(*port_dev->vrf_ptr),
+ GFP_KERNEL);
+ if (!port_dev->vrf_ptr)
+ return -ENOMEM;
^^^^^^^^^
I believe you'll have a slave in the list with inconsistent state which could
even lead to null ptr derefernce if vrf_ptr is used, also __vrf_insert_slave
does dev_hold so the dev refcnt will be incorrect as well.
Right. Good catch, will fix.
+
+ port_dev->vrf_ptr->ifindex = dev->ifindex;
+ port_dev->vrf_ptr->tb_id = vrf->tb_id;
+
+ /* register the packet handler for slave ports */
+ ret = netdev_rx_handler_register(port_dev, vrf_handle_frame,
+ (void *)dev);
+ if (ret) {
+ netdev_err(port_dev,
+ "Device %s failed to register rx_handler\n",
+ port_dev->name);
+ kfree(port_dev->vrf_ptr);
+ kfree(s);
+ return ret;
^^^^^^^^^^
The slave is being freed while on the list here, device's refcnt will be wrong
etc.
ack. Will fix.
+ }
+
+ if (is_running) {
+ ret = dev_change_flags(port_dev, flags & ~IFF_UP);
+ if (ret < 0)
+ goto out_fail;
+ }
+
+ ret = netdev_master_upper_dev_link(port_dev, dev);
+ if (ret < 0)
+ goto out_fail;
+
+ if (is_running) {
+ ret = dev_change_flags(port_dev, flags);
+ if (ret < 0)
+ goto out_fail;
+ }
+
+ port_dev->flags |= IFF_SLAVE;
+
+ return 0;
+
+out_fail:
+ spin_lock_bh(&queue->lock);
+ __vrf_kill_slave(queue, s);
+ spin_unlock_bh(&queue->lock);
__vrf_kill_slave() doesn't do upper device unlink and the device can be linked
if we fail in the dev_change_flags above.
will fix.
+
+ return ret;
+ }
+
+ return -EINVAL;
+}
^^^^
In my opinion the structure of the above function should change to something
more
straightforward with proper exit labels and cleanup upon failure, also a level
of
indentation can be avoided.
Sure. The indentation comes after the pointer checks so locals can be
intialized when declared. Will work on the clean up/simplification for
next rev.
+
+static int vrf_del_slave(struct net_device *dev,
+ struct net_device *port_dev)
+{
+ struct net_vrf *vrf = netdev_priv(dev);
+ struct slave_queue *queue = &vrf->queue;
+ struct slave *slave = __vrf_find_slave_dev(queue, port_dev);
+ bool is_running = netif_running(port_dev);
+ unsigned int flags = port_dev->flags;
+ int ret = 0;
ret seems unused/unchecked in this function
It is used but not checked. I struggled with what to do on the error
path. Do we want netdev_err() on a failure?
+
+ if (!slave)
+ return -EINVAL;
+
+ if (is_running)
+ ret = dev_change_flags(port_dev, flags & ~IFF_UP);
+
+ spin_lock_bh(&queue->lock);
+ __vrf_kill_slave(queue, slave);
+ spin_unlock_bh(&queue->lock);
+
+ netdev_upper_dev_unlink(port_dev, dev);
+
+ if (is_running)
+ ret = dev_change_flags(port_dev, flags);
+
+ return 0;
+}
+
+static int vrf_dev_init(struct net_device *dev)
+{
+ struct net_vrf *vrf = netdev_priv(dev);
+
+ spin_lock_init(&vrf->queue.lock);
+ INIT_LIST_HEAD(&vrf->queue.all_slaves);
+ vrf->queue.master_dev = dev;
+
+ dev->dstats = netdev_alloc_pcpu_stats(struct pcpu_dstats);
+ dev->flags = IFF_MASTER | IFF_NOARP;
+ if (!dev->dstats)
+ return -ENOMEM;
^^^^^
nit: I'd suggest moving the check after the allocation
agreed.
David
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
