Re : [CHECKER] 28 potential interrupt errors

Re : [CHECKER] 28 potential interrupt errors

[Jean Tourrilhes]

Junfeng Yang wrote :
> Hi,
> 
> Here are yet more results from the MC project.  This checker looks for
> inconsistent usage of interrupt functions.
[...]
> ---------------------------------------------------------
> [BUG] error path
> 
> /u2/acc/oses/linux/2.4.1/drivers/net/irda/irport.c:943:irport_net_ioctl: ERROR:INTR:947:997: Interrupts inconsistent, severity `20':997
> 
>         /* Disable interrupts & save flags */
>         save_flags(flags);
> Start --->
>         cli();
> 
>         switch (cmd) {
>         case SIOCSBANDWIDTH: /* Set bandwidth */
>                 if (!capable(CAP_NET_ADMIN))
>                         return -EPERM;
>                 irda_task_execute(self, __irport_change_speed, NULL, NULL,
> 
>         ... DELETED 40 lines ...
> 
>         }
> 
>         restore_flags(flags);
> 
> Error --->
>         return ret;
> }
> 
> static struct net_device_stats *irport_net_get_stats(struct net_device *dev)
> {
> ---------------------------------------------------------

	I agree that the IrDA stack is full of irq/locking bugs (there
is a patch of mine waiting in Dag's mailbox), but this one is not a
bug, it's a false positive.
	The restore_flags(flags); will restore the state of the
interrupt register before the cli happened, so will automatically
reenable interrupts. The exact same code was used all over the kernel
before spinlock were introduced.

	So, if you see :
	        save_flags(flags);
	        cli();
		...
	        restore_flags(flags);
	It's correct (but a bit outdated).


> ---------------------------------------------------------
> [BUG] error path. this bug is interesting
> 
> /u2/acc/oses/linux/2.4.1/drivers/net/pcmcia/wavelan_cs.c:2561:wavelan_get_wireless_stats: ERROR:INTR:2528:2561: Interrupts inconsistent, severity `20':2561
> 
> 
>   /* Disable interrupts & save flags */
> Start --->
>   spin_lock_irqsave (&lp->lock, flags);
> 
>   if(lp == (net_local *) NULL)
>     return (iw_stats *) NULL;
>   wstats = &lp->wstats;
> 
>   /* Get data from the mmc */
> 
>         ... DELETED 23 lines ...
> 
> 
> #ifdef DEBUG_IOCTL_TRACE
>   printk(KERN_DEBUG "%s: <-wavelan_get_wireless_stats()\n", dev->name);
> #endif
> Error --->
>   return &lp->wstats;
> }
> #endif  /* WIRELESS_EXT */
> 
> ---------------------------------------------------------

	Didn't look into 2.4.1, but in 2.4.2 the irq_restore is just
above the printk, in the part that is "DELETED". It even has a nice
comments to that effect. Check the code by yourself.
	So, I guess it's another false positive and a bug in your
parser. That's why it's so "interesting" ;-)

	Good luck...

	Jean

Re: Re : [CHECKER] 28 potential interrupt errors

[Junfeng Yang]

Sometimes the line number reported by the checker is not correct.
But if you go into the function, you can find the bug.

On Thu, 22 Mar 2001, Jean Tourrilhes wrote:

> Junfeng Yang wrote :
> > Hi,
> >
> > Here are yet more results from the MC project.  This checker looks for
> > inconsistent usage of interrupt functions.
> [...]
> > ---------------------------------------------------------
> > [BUG] error path

at line 952, this function exits without restoring flags.

> >
> > /u2/acc/oses/linux/2.4.1/drivers/net/irda/irport.c:943:irport_net_ioctl: ERROR:INTR:947:997: Interrupts inconsistent, severity `20':997
> >
> >         /* Disable interrupts & save flags */
> >         save_flags(flags);
> > Start --->
> >         cli();
> >
> >         switch (cmd) {
> >         case SIOCSBANDWIDTH: /* Set bandwidth */
> >                 if (!capable(CAP_NET_ADMIN))
> >                         return -EPERM;
> >                 irda_task_execute(self, __irport_change_speed, NULL, NULL,
> >
> >         ... DELETED 40 lines ...
> >
> >         }
> >
> >         restore_flags(flags);
> >
> > Error --->
> >         return ret;
> > }
> >
> > [BUG] error path. this bug is interesting
> >
> > /u2/acc/oses/linux/2.4.1/drivers/net/pcmcia/wavelan_cs.c:2561:wavelan_get_wireless_stats: ERROR:INTR:2528:2561: Interrupts inconsistent, severity `20':2561
> >
> >
> >   /* Disable interrupts & save flags */
> > Start --->
> >   spin_lock_irqsave (&lp->lock, flags);
> >
> >   if(lp == (net_local *) NULL)

return without restoring flags here
(use lp->lock first, then check if lp == NULL)
--->

> >     return (iw_stats *) NULL;
> >   wstats = &lp->wstats;
> >
> >   /* Get data from the mmc */
> >
> >         ... DELETED 23 lines ...
> >

Re: Re : [CHECKER] 28 potential interrupt errors

[Jean Tourrilhes]

On Thu, Mar 22, 2001 at 03:49:31PM -0800, Junfeng Yang wrote:
> 
> Sometimes the line number reported by the checker is not correct.
> But if you go into the function, you can find the bug.

	Gotcha. It in fact indicate the error at the end of the
function instead of the place where the error is. Very confusing.
	So, mea culpa, I was wrong...

	Jean

Re: Re : [CHECKER] 28 potential interrupt errors

[Linus Torvalds]

In article <20010322153641.B13162@bougret.hpl.hp.com>,
Jean Tourrilhes  <jt@bougret.hpl.hp.com> wrote:
>
>	I agree that the IrDA stack is full of irq/locking bugs (there
>is a patch of mine waiting in Dag's mailbox), but this one is not a
>bug, it's a false positive.
>	The restore_flags(flags); will restore the state of the
>interrupt register before the cli happened, so will automatically
>reenable interrupts.

Look closer. The error report is a big bogus, because it points out as
an error the "return" that is _correct_, not the "return" that is buggy.

Their checkers verify that all exists out of a function have the same
characteristics, and they found a case where one exit exists with
interrupts still disabled, while another one exists after having done a
"restore_flags()". 

So it looks like a real bug, it's just that the error is the _earlier_
return value, not the one pointed at.

		Linus