Leaked net-device reference in eql.c

Leaked net-device reference in eql.c

[Ben Greear]

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;
}


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Leaked net-device reference in eql.c

[Patrick McHardy]

Ben Greear wrote:
> I think the eql_s_slave_cfg method in eql.c leaks
> the reference to slave_dev.  Am I missing something?

No, it should also put the device, as in eql_g_slave_cfg.

Re: Leaked net-device reference in eql.c

[Ben Greear]

Patrick McHardy wrote:
> Ben Greear wrote:
> 
>> I think the eql_s_slave_cfg method in eql.c leaks
>> the reference to slave_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);
}



-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Leaked net-device reference in eql.c

[Arnaldo Carvalho de Melo]

On 8/27/05, Ben Greear <greearb@candelatech.com> wrote:
> Patrick McHardy wrote:
> > Ben Greear wrote:
> >
> >> I think the eql_s_slave_cfg method in eql.c leaks
> >> the reference to slave_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);
> }

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...

Re: Leaked net-device reference in eql.c

[Arnaldo Carvalho de Melo]

On 8/27/05, Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote:
> On 8/27/05, Ben Greear <greearb@candelatech.com> wrote:
> > Patrick McHardy wrote:
> > > Ben Greear wrote:
> > >
> > >> I think the eql_s_slave_cfg method in eql.c leaks
> > >> the reference to slave_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);
> > }
> 
> 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...

Yup, look at __netdev_watchdog_up :-)

- Arnaldo