Leaked net-device reference in eql.c
Subsystems (2)
Hello!
I think the eql_s_slave_cfg method in eql.c leaks the reference to slave_dev. Am I missing something?
static int eql_s_slave_cfg(struct net_device *dev, slave_config_t __user *scp)
{
slave_t *slave;
equalizer_t *eql;
struct net_device *slave_dev;
slave_config_t sc;
int ret;
if (copy_from_user(&sc, scp, sizeof (slave_config_t)))
return -EFAULT;
slave_dev = dev_get_by_name(sc.slave_name);
if (!slave_dev)
return -ENODEV;
ret = -EINVAL;
eql = netdev_priv(dev);
spin_lock_bh(&eql->queue.lock);
if (eql_is_slave(slave_dev)) {
slave = __eql_find_slave_dev(&eql->queue, slave_dev);
if (slave) {
slave->priority = sc.priority;
slave->priority_bps = sc.priority;
slave->priority_Bps = sc.priority / 8;
ret = 0;
}
}
spin_unlock_bh(&eql->queue.lock);
return ret;
}Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Quoted 2 lines · Ben Greear
I think the
eql_s_slave_cfgmethod ineql.cleaks the reference toslave_dev. Am I missing something?
No, it should also put the device, as in eql_g_slave_cfg.
Quoted 7 lines · Patrick McHardy
Quoted 2 lines · Ben Greear
I think the
eql_s_slave_cfgmethod ineql.cleaks the reference toslave_dev. Am I missing something?No, it should also put the device, as in
eql_g_slave_cfg.
Ok, I'm making a patch...will add this to it.
How about this one. It seems like it does a dev_put when it shouldn't (if some of the if's fail, the dev_get never happened):
net/sched/sch_generic.c
static void dev_watchdog(unsigned long arg)
{
struct net_device *dev = (struct net_device *)arg;
spin_lock(&dev->xmit_lock);
if (dev->qdisc != &noop_qdisc) {
if (netif_device_present(dev) &&
netif_running(dev) &&
netif_carrier_ok(dev)) {
if (netif_queue_stopped(dev) &&
(jiffies - dev->trans_start) > dev->watchdog_timeo) {
printk(KERN_INFO "NETDEV WATCHDOG: %s: transmit timed out\n", dev->name);
dev->tx_timeout(dev);
}
if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
dev_hold(dev);
}
}
spin_unlock(&dev->xmit_lock);
dev_put(dev);
}Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Quoted 30 lines · Ben Greear
Quoted 7 lines · Patrick McHardy
Quoted 2 lines · Ben Greear
I think the
eql_s_slave_cfgmethod ineql.cleaks the reference toslave_dev. Am I missing something?No, it should also put the device, as in
eql_g_slave_cfg.Ok, I'm making a patch...will add this to it.
How about this one. It seems like it does a
dev_putwhen it shouldn't (if some of the if's fail, thedev_getnever happened):
net/sched/sch_generic.cstatic void
dev_watchdog(unsigned long arg){ struct net_device *dev = (struct net_device *)arg; spin_lock(&dev->xmit_lock); if (dev->qdisc != &noop_qdisc) { if (netif_device_present(dev) && netif_running(dev) && netif_carrier_ok(dev)) { if (netif_queue_stopped(dev) && (jiffies - dev->trans_start) > dev->watchdog_timeo) { printk(KERN_INFO "NETDEV WATCHDOG: %s: transmit timed out\n", dev->name); dev->tx_timeout(dev); }
Quoted 8 lines
if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
dev_hold(dev);
}
}
spin_unlock(&dev->xmit_lock);
dev_put(dev);
}Doesn't look like its a problem, its the classical case where when you associate some data structure to a timer you grab a refcount, when the timer expires you drop the refcount, and as the code above shows when mod_timer is succesfully called it grabs a reference, so the reference being dropped above is from the previous timer firing, now its just a matter if looking for the first mod_timer, that must be at some other place in sched_generic.c, lemme see...
Quoted 40 lines · Arnaldo Carvalho de Melo
Quoted 30 lines · Ben Greear
Quoted 7 lines · Patrick McHardy
Quoted 2 lines · Ben Greear
I think the
eql_s_slave_cfgmethod ineql.cleaks the reference toslave_dev. Am I missing something?No, it should also put the device, as in
eql_g_slave_cfg.Ok, I'm making a patch...will add this to it.
How about this one. It seems like it does a
dev_putwhen it shouldn't (if some of the if's fail, thedev_getnever happened):
net/sched/sch_generic.cstatic void
dev_watchdog(unsigned long arg){ struct net_device *dev = (struct net_device *)arg; spin_lock(&dev->xmit_lock); if (dev->qdisc != &noop_qdisc) { if (netif_device_present(dev) && netif_running(dev) && netif_carrier_ok(dev)) { if (netif_queue_stopped(dev) && (jiffies - dev->trans_start) > dev->watchdog_timeo) { printk(KERN_INFO "NETDEV WATCHDOG: %s: transmit timed out\n", dev->name); dev->tx_timeout(dev); }Quoted 8 lines
if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo)) dev_hold(dev); } } spin_unlock(&dev->xmit_lock); dev_put(dev); }
Quoted 7 lines
Doesn't look like its a problem, its the classical case where when you associate some data structure to a timer you grab a refcount, when the timer expires you drop the refcount, and as the code above shows when
mod_timeris succesfully called it grabs a reference, so the reference being dropped above is from the previous timer firing, now its just a matter if looking for the firstmod_timer, that must be at some other place insched_generic.c, lemme see...
Yup, look at __netdev_watchdog_up :-)
- Arnaldo