Bug in mask_proc (repost)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug in mask_proc (repost)

Lino Sanfilippo

Hello,

This is a repost of a patch that I posted on 9th July 2009. This time
the patch is
for the recent dazukofs version (3.1.1).

The problem is that there is a (dazukofs_proc) structure allocated  on
the stack and
afterwards put in a global list. This will very likely leed to problems
sooner or later,
since the stack memory may be overwritten the next time the proc
structure is accessed
in the list.

This patch solves this problem by using a structure that is allocated on
the heap.

Regards,
Lino Sanfilippo

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.1/event.c dazukofs-3.1.1-PATCHED/event.c
--- dazukofs-3.1.1/event.c 2009-07-02 21:15:05.000000000 +0200
+++ dazukofs-3.1.1-PATCHED/event.c 2009-11-19 09:30:14.000000000 +0100
@@ -87,6 +87,7 @@ static struct dazukofs_proc proc_list;
 static struct kmem_cache *dazukofs_group_cachep;
 static struct kmem_cache *dazukofs_event_container_cachep;
 static struct kmem_cache *dazukofs_event_cachep;
+static struct kmem_cache *dazukofs_proc_cachep;
 
 static int last_event_id;
 
@@ -128,6 +129,13 @@ int dazukofs_init_events(void)
  if (!dazukofs_event_cachep)
  goto error_out;
 
+ dazukofs_proc_cachep =
+ kmem_cache_create("dazukofs_proc_cache",
+  sizeof(struct dazukofs_proc), 0,
+  SLAB_HWCACHE_ALIGN, NULL);
+ if (!dazukofs_proc_cachep)
+ goto error_out;
+
  return 0;
 
 error_out:
@@ -137,6 +145,9 @@ error_out:
  kmem_cache_destroy(dazukofs_event_container_cachep);
  if (dazukofs_event_cachep)
  kmem_cache_destroy(dazukofs_event_cachep);
+ if (dazukofs_proc_cachep)
+ kmem_cache_destroy(dazukofs_proc_cachep);
+
  return -ENOMEM;
 }
 
@@ -253,6 +264,41 @@ static void __remove_group(struct dazuko
  wake_up_all(&grp->queue);
 }
 
+
+static void cleanup_groups(void)
+{
+ struct list_head *q;
+ struct list_head *pos;
+ struct dazukofs_group *grp;
+
+ list_for_each_safe(pos, q, &group_list.list) {
+ grp = list_entry(pos, struct dazukofs_group, list);
+ list_del(pos);
+
+ __remove_group(grp);
+
+ /* free group name */
+ kfree(grp->name);
+
+ /* free group */
+ kmem_cache_free(dazukofs_group_cachep, grp);
+ }
+}
+
+static void cleanup_ignored_procs(void)
+{ struct list_head *q;
+ struct list_head *pos;
+ struct dazukofs_proc *proc;
+
+ /* free the groups */
+ list_for_each_safe(pos, q, &proc_list.list) {
+ proc = list_entry(pos, struct dazukofs_proc, list);
+ list_del(pos);
+ /* free proc */
+ kmem_cache_free(dazukofs_proc_cachep, proc);
+ }
+}
+
 /**
  * dazukofs_destroy_events - cleanup/shutdown event handling infrastructure
  *
@@ -260,9 +306,6 @@ static void __remove_group(struct dazuko
  */
 void dazukofs_destroy_events(void)
 {
- struct dazukofs_group *grp;
- struct list_head *pos;
- struct list_head *q;
 
  /*
  * We are not using any locks here because we assume
@@ -271,23 +314,16 @@ void dazukofs_destroy_events(void)
  */
 
  /* free the groups */
- list_for_each_safe(pos, q, &group_list.list) {
- grp = list_entry(pos, struct dazukofs_group, list);
- list_del(pos);
-
- __remove_group(grp);
-
- /* free group name */
- kfree(grp->name);
+ cleanup_groups();
 
- /* free group */
- kmem_cache_free(dazukofs_group_cachep, grp);
- }
+ /* free ignored processes */
+ cleanup_ignored_procs();
 
  /* free everything else */
  kmem_cache_destroy(dazukofs_group_cachep);
  kmem_cache_destroy(dazukofs_event_container_cachep);
  kmem_cache_destroy(dazukofs_event_cachep);
+ kmem_cache_destroy(dazukofs_proc_cachep);
 }
 
 /**
@@ -576,6 +612,7 @@ static int check_recursion(void)
  found = 1;
  put_pid(proc->proc_id);
  list_del(pos);
+ kmem_cache_free(dazukofs_proc_cachep, proc);
  break;
  }
  }
@@ -951,12 +988,19 @@ static struct dazukofs_event_container *
  * 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)
+static int mask_proc(void)
 {
+ struct dazukofs_proc *proc;
+
+ proc = kmem_cache_zalloc(dazukofs_proc_cachep, GFP_KERNEL);
+ if (!proc)
+ return -ENOMEM;
+
  proc->proc_id = get_pid(task_pid(current));
  mutex_lock(&proc_mutex);
  list_add(&proc->list, &proc_list.list);
  mutex_unlock(&proc_mutex);
+ return 0;
 }
 
 /**
@@ -972,7 +1016,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 */
@@ -984,7 +1027,9 @@ static int open_file(struct dazukofs_eve
  }
 
  /* add myself to be ignored on file open (to avoid recursion) */
- mask_proc(&proc);
+ ret = mask_proc();
+ if (ret)
+ goto error_out2;
 
  ec->file = dentry_open(dget(evt->dentry), mntget(evt->mnt),
        O_RDONLY | O_LARGEFILE, current_cred());

_______________________________________________
Dazuko-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/dazuko-devel
Reply | Threaded
Open this post in threaded view
|

Re: Bug in mask_proc (repost)

John Ogness-9
On 2009-11-19, Lino Sanfilippo <[hidden email]> wrote:
> This is a repost of a patch that I posted on 9th July 2009. This
> time the patch is for the recent dazukofs version (3.1.1).
>
> The problem is that there is a (dazukofs_proc) structure allocated
> on the stack and afterwards put in a global list. This will very
> likely leed to problems sooner or later, since the stack memory may
> be overwritten the next time the proc structure is accessed in the
> list.

Please specify an example code path in Linux proving the problem can
occur.

In my opinion you are fixing a problem that does not exist and instead
are creating an extra (expensive) malloc for every file access.

John Ogness

--
Dazuko Maintainer


_______________________________________________
Dazuko-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/dazuko-devel
Reply | Threaded
Open this post in threaded view
|

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

_______________________________________________
Dazuko-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/dazuko-devel
Reply | Threaded
Open this post in threaded view
|

Re: Bug in mask_proc (repost)

Lino Sanfilippo
Lino Sanfilippo wrote:
> 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.

Ok, forget about that. Of course that cant happen, since a signal (like
SIGTERM)
sent to the process wont keep the kernel code from cleaning up before
the process
is terminated. So it should always be able to clean up its own list entry :P

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


_______________________________________________
Dazuko-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/dazuko-devel
Reply | Threaded
Open this post in threaded view
|

Re: Bug in mask_proc (repost)

John Ogness-9
On 2009-11-24, Lino Sanfilippo <[hidden email]> wrote:
>> 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.
>
> Ok, forget about that. Of course that cant happen, since a signal
> (like SIGTERM) sent to the process wont keep the kernel code from
> cleaning up before the process is terminated. So it should always be
> able to clean up its own list entry :P

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, but it
could change at some point in the future.

If preemption did occur, it would be possible for
dazukofs_remove_group() to be called, which could result in
check_access_precheck() aborting without removing the process from the
list (if group_count was now 0). This would then lead to invalid
memory in the list, because open_file() also would not remove it.

Probably a good (and efficient) fix would be to have check_recursion()
set a "removed" flag in the process structure. That way, open_file()
could very easily see if it has been removed or not. In my opinion,
that is much better than relying on the return value of dentry_open().

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

John Ogness

--
Dazuko Maintainer


_______________________________________________
Dazuko-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/dazuko-devel
Reply | Threaded
Open this post in threaded view
|

Re: Bug in mask_proc (repost)

Lino Sanfilippo
John Ogness wrote:

> Probably a good (and efficient) fix would be to have check_recursion()
> set a "removed" flag in the process structure. That way, open_file()
> could very easily see if it has been removed or not. In my opinion,
> that is much better than relying on the return value of dentry_open().
>
> 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;
> }
>
> John Ogness
>
>  
And what about the solution I suggested to avoid the whole
recursion thing (by simply passing _lower_ dentry and mount
to dazukofs_open()).
Do you (or someone else) see any problems with that?


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


_______________________________________________
Dazuko-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/dazuko-devel
Reply | Threaded
Open this post in threaded view
|

Re: Bug in mask_proc (repost)

John Ogness-9
On 2009-11-26, Lino Sanfilippo <[hidden email]> wrote:
> And what about the solution I suggested to avoid the whole recursion
> thing (by simply passing _lower_ dentry and mount to
> dazukofs_open()).  Do you (or someone else) see any problems with
> that?

I don't like the idea of userspace getting a handle directly to the
lower mount. All activity of the registered process would go unnoticed
by DazukoFS. This may even cause problems because the atime's could
become out of sync.

I'm not certain how safe it is for any processes to be playing with
files underneath DazukoFS while DazukoFS is mounted.

John Ogness

--
Dazuko Maintainer


_______________________________________________
Dazuko-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/dazuko-devel
Reply | Threaded
Open this post in threaded view
|

Re: Bug in mask_proc (repost)

Lino Sanfilippo
In reply to this post by John Ogness-9
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
***************************************************


_______________________________________________
Dazuko-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/dazuko-devel