Re: [Dazuko-devel] Bug in mask_proc (repost)

[Lino Sanfilippo]

John Ogness wrote:
> I actually thought a lot about this when I first implemented it. And I
> just finished investigating it again. It is assumed that no preemption
> occurs in the calling chain between event.c:dentry_open() and
> file.c:dazukofs_open(). Right now that assumption is correct...
>   

Why do you think this assumption is correct? If you compile your kernel
with kernel preemption enabled, kernel code in process context may
be preempted at any time (except if you hold a lock or disable preemption
explicitly with preempt_disable()). And think of multiprocessor systems:
The last daemon may be unregistered on one processor while the other
one is still processing a file access.

> Rather than:
>
> ec->file = dentry_open(dget(evt->dentry), mntget(evt->mnt),
>                        O_RDONLY | O_LARGEFILE, current_cred());
> if (IS_ERR(ec->file)) {
>         check_recursion();  /* remove myself from proc_list */
>         ret = PTR_ERR(ec->file);
>         goto error_out2;
> }
>
> the code could look like this:
>
> ec->file = dentry_open(dget(evt->dentry), mntget(evt->mnt),
>                        O_RDONLY | O_LARGEFILE, current_cred());
> if (!proc.removed)
>         check_recursion();  /* remove myself from proc_list */
> if (IS_ERR(ec->file)) {
>         ret = PTR_ERR(ec->file);
>         goto error_out2;
> }
>   

Looks ok, but even better would be to remove the process
from list in the same function in which it was added: in open_file()


/* add myself to be ignored on file open (to avoid recursion) */
mask_proc(&proc);

ec->file = dentry_open(dget(evt->dentry), mntget(evt->mnt), O_RDONLY);
if (IS_ERR(ec->file)) {
check_recursion(); /* remove myself from proc_list */
ret = PTR_ERR(ec->file);
goto error_out2;
}
/* remove myself from being ignored on file open */
unmask_proc(&proc);

This is much easier to understand when reading the code and
it is save, since the process is always able to remove itself from the list
again, no matter if the last daemon was unregistered.

With this, check_recursion() would be reduced to a simple check:

static int check_recursion(void)
{
struct dazukofs_proc *proc;
struct list_head *pos;
int found = 0;

mutex_lock(&proc_mutex);
list_for_each(pos, &proc_list.list) {
proc = list_entry(pos, struct dazukofs_proc, list);
if (proc->curr == current) {
found = 1;

#if 0
list_del(pos);
#endif

break;
}
}
mutex_unlock(&proc_mutex);

/* process event if not found */
return !found;
}







Geschäftsführender Gesellschafter: Tjark Auerbach
Sitz der Gesellschaft: Tettnang
Handelsregister: Amtsgericht Ulm, HRB 630992
ALLGEMEINE GESCHÄFTSBEDINGUNGEN
Es gelten unsere Allgemeinen Geschäftsbedingungen
(AGB). Sie finden sie in der jeweils gültigen Fassung
im Internet unter http://www.avira.de/agb
***************************************************