Re: An assortment of module inc/dec races

[Alexander Viro]

On Mon, 7 Aug 2000, Dawson Engler wrote:

> Hi,
> 
> we have a g++ extension that checks that linux module code follows the
> three rules laid out by Tim & Rusty w.r.t. MOD_INC/MOD_DEC:
> 
> 	1. cannot call a sleep function before MOD_INC
> 	2. cannot call a sleep function after MOD_DEC
> 	3. if you return with an error after MOD_INC, must do a MOD_DEC
> 
> The current functions it thinks can sleep are limited to:
> 	- kmalloc(*, GFP_KERNEL)
> 	- *_user
> 
> Run on the 2.3.99 kernel it turned up 22 cases that look like bugs (a
> representative subset is enclosed).  If this is something that is useful
> to people, we also did a global analysis using the transitive closure of   
> all functions that could call functions that could sleep.  This turned
> up about 2x more suspicious cases.

	Not too interesting - most of these calls simply disappeared in
2.4.0-test. pcmcia ones didn't, though. That subsystem is still not fixed.
Ditto for ATM and net devices.
 
> /u2/engler/ic/linux-2.3.99/net/sched/sch_dsmark.c:347:dsmark_init: ERROR: MOD_INC_COUNT after sleep!
> 
> int dsmark_init(struct Qdisc *sch,struct rtattr *opt)
> {
> 	...
>         p->set_tc_index = !!tb[TCA_DSMARK_SET_TC_INDEX-1];
>         p->mask = kmalloc(p->indices*2,GFP_KERNEL);
> 	...
>         MOD_INC_USE_COUNT;
>         return 0;
> }
> 
> ----------------------------------------------------------------------------
> CONFIRMED
> 
> /u2/engler/ic/linux-2.3.99/net/appletalk/ddp.c:365:atif_add_device: ERROR: MOD_INC_COUNT after sleep!
> 
> static struct atalk_iface *atif_add_device(struct net_device *dev, struct at_addr *sa)
> {
>         struct atalk_iface *iface = (struct atalk_iface *)
>                 kmalloc(sizeof(*iface), GFP_KERNEL);
> 
>         if (iface == NULL)
>                 return NULL;
> 
>         iface->dev = dev;
>         dev->atalk_ptr = iface;
>         iface->address = *sa;
>         iface->status = 0;
> 
>         spin_lock_bh(&atalk_iface_lock);        iface->next = atalk_iface_list;
>         atalk_iface_list = iface;
>         spin_unlock_bh(&atalk_iface_lock);
> 
>         MOD_INC_USE_COUNT;
> 
>         return iface;
> }
> ----------------------------------------------------------------------------

Hmm... No comments on these.

Thanks for pointing to the ATM stuff - looks like they are also worth
shifting the refcounting into callers.

It would be great if you would run your tools over the recent 2.4.0-test -
_many_ things had changed in that area.