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

[Lino Sanfilippo]

John Ogness wrote:
> Please specify an example code path in Linux proving the problem can
> occur.
>   

This problem can occur every time after the daemon that ignored itself
terminates (unexpectedly or not) before it is able to remove its
proc structure from the list.
When this happens there is a proc structure in the list pointing to
the stack memory of a process that does not exist any more.
And who can say what that memory actually contains the next time
it is accessed?
Sooner or later it is reused by the kernel (maybe for a new process),
and its content is modified in an unpredictable way.

The patch I provided before does not handle those obsolete list entries,
too. But it ensures that the content of the memory at which the list
entries point to, cant be modified in an unpredictable way.

But anyway, maybe we can get rid of this whole recursion stuff at all:
Why do we not simply pass the _lower_ mount and the _lower_ dentry
to dentry_open()? I tried it and it seems to work (or do I oversee a
reason why this could lead to problems?)
I created a patch for 3.1.2 (see attachment) , if you agree that this 
could work
we could remove that whole recursion implementation (including the patch
I sent before).

Regards,
Lino






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

diff -rup dazukofs-3.1.2/event.c dazukofs-3.1.2-PATCHED/event.c
--- dazukofs-3.1.2/event.c      2009-07-02 21:15:05.000000000 +0200
+++ dazukofs-3.1.2-PATCHED/event.c      2009-11-24 13:09:35.000000000 +0100
@@ -548,46 +548,6 @@ tryagain:
 }
 
 /**
- * check_recursion - check if current process is recursing
- *
- * Description: A list of anonymous processes is managed to prevent
- * access event recursion. This function checks if the current process is
- * a part of that list.
- *
- * If the current process is found in the process list, it is removed.
- *
- * NOTE: The proc structure is not freed. It is only removed from the
- *       list. Since it is a recursive call, the caller can free the
- *       structure after the call chain is finished.
- *
- * Returns 0 if this is a recursive process call.
- */
-static int check_recursion(void)
-{
-       struct dazukofs_proc *proc;
-       struct list_head *pos;
-       int found = 0;
-       struct pid *cur_proc_id = get_pid(task_pid(current));
-
-       mutex_lock(&proc_mutex);
-       list_for_each(pos, &proc_list.list) {
-               proc = list_entry(pos, struct dazukofs_proc, list);
-               if (proc->proc_id == cur_proc_id) {
-                       found = 1;
-                       put_pid(proc->proc_id);
-                       list_del(pos);
-                       break;
-               }
-       }
-       mutex_unlock(&proc_mutex);
-
-       put_pid(cur_proc_id);
-
-       /* process event if not found */
-       return !found;
-}
-
-/**
  * event_assigned - check if event is (still) assigned
  * @event: event to check
  *
@@ -621,10 +581,6 @@ static int check_access_precheck(int grp
        if (grp_count == 0)
                return -1;
 
-       /* am I a recursion process? */
-       if (!check_recursion())
-               return -1;
-
        /* am I an ignored process? */
        if (!dazukofs_check_ignore_process())
                return -1;
@@ -755,8 +711,8 @@ int dazukofs_check_access(struct dentry 
                goto out;
        }
 
-       evt->dentry = dget(dentry);
-       evt->mnt = mntget(mnt);
+       evt->dentry = dget(get_lower_dentry(dentry));
+       evt->mnt = mntget(get_lower_mnt(dentry));
        evt->proc_id = get_pid(task_pid(current));
 
        assign_event_to_groups(evt, ec_array);
@@ -943,23 +899,6 @@ static struct dazukofs_event_container *
 }
 
 /**
- * mask_proc - mask the current process
- * @proc: process structure to use for the list
- *
- * Description: Assign the current process to the provided proc structure
- * and add the structure to the list. The list is used to prevent
- * generating recursive file access events. The process is removed from
- * the list with the check_recursion() function.
- */
-static void mask_proc(struct dazukofs_proc *proc)
-{
-       proc->proc_id = get_pid(task_pid(current));
-       mutex_lock(&proc_mutex);
-       list_add(&proc->list, &proc_list.list);
-       mutex_unlock(&proc_mutex);
-}
-
-/**
  * open_file - open a file for the current process (avoiding recursion)
  * @ec: event container to store opened file descriptor
  *
@@ -972,7 +911,6 @@ static void mask_proc(struct dazukofs_pr
 static int open_file(struct dazukofs_event_container *ec)
 {
        struct dazukofs_event *evt = ec->event;
-       struct dazukofs_proc proc;
        int ret;
 
        /* open the file read-only */
@@ -983,13 +921,9 @@ static int open_file(struct dazukofs_eve
                goto error_out1;
        }
 
-       /* 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 | O_LARGEFILE, current_cred());
        if (IS_ERR(ec->file)) {
-               check_recursion();  /* remove myself from proc_list */
                ret = PTR_ERR(ec->file);
                goto error_out2;
        }