[PATCH] kernel/audit_tree.c: tree will memory leak when failure occurs for audit_trim_trees()

[PATCH] kernel/audit_tree.c: tree will memory leak when failure occurs for audit_trim_trees()

[Chen Gang]

in audit_trim_trees(), has called get_tree() before failure occurs,
so need also call put_tree after go to skip_it:

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/audit_tree.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 642a89c..de46ec0 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -617,10 +617,10 @@ void audit_trim_trees(void)
 		}
 		spin_unlock(&hash_lock);
 		trim_marked(tree);
-		put_tree(tree);
 		drop_collected_mounts(root_mnt);
 skip_it:
 		mutex_lock(&audit_filter_mutex);
+		put_tree(tree);
 	}
 	list_del(&cursor);
 	mutex_unlock(&audit_filter_mutex);
-- 
1.7.7.6

Re: [PATCH] kernel/audit_tree.c: tree will memory leak when failure occurs for audit_trim_trees()

[Andrew Morton]

On Fri, 19 Apr 2013 17:39:06 +0800 Chen Gang <gang.chen@asianux.com> wrote:

> 
> in audit_trim_trees(), has called get_tree() before failure occurs,
> so need also call put_tree after go to skip_it:
> 
> ...
>
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -617,10 +617,10 @@ void audit_trim_trees(void)
>  		}
>  		spin_unlock(&hash_lock);
>  		trim_marked(tree);
> -		put_tree(tree);
>  		drop_collected_mounts(root_mnt);
>  skip_it:
>  		mutex_lock(&audit_filter_mutex);
> +		put_tree(tree);
>  	}
>  	list_del(&cursor);
>  	mutex_unlock(&audit_filter_mutex);

That looks right to me.

I think we can micro-optimise the code by performing the put_tree()
before taking the mutex, to slightly reduce mutex hold times?

--- a/kernel/audit_tree.c~kernel-audit_treec-tree-will-leak-memory-when-failure-occurs-in-audit_trim_trees-fix
+++ a/kernel/audit_tree.c
@@ -619,8 +619,8 @@ void audit_trim_trees(void)
 		trim_marked(tree);
 		drop_collected_mounts(root_mnt);
 skip_it:
-		mutex_lock(&audit_filter_mutex);
 		put_tree(tree);
+		mutex_lock(&audit_filter_mutex);
 	}
 	list_del(&cursor);
 	mutex_unlock(&audit_filter_mutex);
_

Re: [PATCH] kernel/audit_tree.c: tree will memory leak when failure occurs for audit_trim_trees()

[Chen Gang]

On 2013年04月23日 07:04, Andrew Morton wrote:
> On Fri, 19 Apr 2013 17:39:06 +0800 Chen Gang <gang.chen@asianux.com> wrote:
> 
>>
>> in audit_trim_trees(), has called get_tree() before failure occurs,
>> so need also call put_tree after go to skip_it:
>>
>> ...
>>
>> --- a/kernel/audit_tree.c
>> +++ b/kernel/audit_tree.c
>> @@ -617,10 +617,10 @@ void audit_trim_trees(void)
>>  		}
>>  		spin_unlock(&hash_lock);
>>  		trim_marked(tree);
>> -		put_tree(tree);
>>  		drop_collected_mounts(root_mnt);
>>  skip_it:
>>  		mutex_lock(&audit_filter_mutex);
>> +		put_tree(tree);
>>  	}
>>  	list_del(&cursor);
>>  	mutex_unlock(&audit_filter_mutex);
> 
> That looks right to me.
> 
> I think we can micro-optimise the code by performing the put_tree()
> before taking the mutex, to slightly reduce mutex hold times?
> 

  ok, thanks.

> --- a/kernel/audit_tree.c~kernel-audit_treec-tree-will-leak-memory-when-failure-occurs-in-audit_trim_trees-fix
> +++ a/kernel/audit_tree.c
> @@ -619,8 +619,8 @@ void audit_trim_trees(void)
>  		trim_marked(tree);
>  		drop_collected_mounts(root_mnt);
>  skip_it:
> -		mutex_lock(&audit_filter_mutex);
>  		put_tree(tree);
> +		mutex_lock(&audit_filter_mutex);
>  	}
>  	list_del(&cursor);
>  	mutex_unlock(&audit_filter_mutex);
> _
> 
> 
> 
> 


-- 
Chen Gang

Asianux Corporation

[PATCH kernel-next] kernel/audit_tree.c: fix the original version merging issue for put_tree().

[Chen Gang]

During version merging, added another redundant put_tree().

The related original patch: 1b4f5c2 kernel/audit_tree.c: tree will leak
memory when failure occurs in audit_trim_trees().

So need delete it, or it will cause issue.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/audit_tree.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 9fd17f0..a291aa2 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -621,7 +621,6 @@ void audit_trim_trees(void)
 skip_it:
 		put_tree(tree);
 		mutex_lock(&audit_filter_mutex);
-		put_tree(tree);
 	}
 	list_del(&cursor);
 	mutex_unlock(&audit_filter_mutex);
-- 
1.7.7.6

[PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures

[Chen Gang]

On 2013年04月20日 15:31, Chen Gang wrote:
> On 2013年04月18日 09:19, Chen Gang F T wrote:
>> > On 2013年04月18日 04:07, Andrew Morton wrote:
>>>> >> > On Wed, 17 Apr 2013 12:04:02 +0800 Chen Gang <gang.chen@asianux.com> wrote:
>>>> >> > 
>>>>>>>> >>>> >> >   since "normally audit_add_tree_rule() will free it on failure",
>>>>>>>> >>>> >> >   need free it completely, when failure occures.
>>>>>>>> >>>> >> > 
>>>>>>>> >>>> >> >     need additional put_tree before return, since get_tree was called.
>>>>>>>> >>>> >> >     always need goto error processing area for list_del_init.
>>>> >> > Isn't that get_tree() in audit_add_tree_rule() simply unneeded?  In
>>>> >> > other words, is this patch correct:
>>>> >> > 
> 

Hell all:

get_tree() in audit_add_tree_rule() is needed (my original conclusion is incorrect).

  the reason is when failed, trim_marked() will call put_tree() to free the tree and the related entry.
    trim_marked() ->
      ...
      tree->goner = 1
      ...
      kill_rules()
      ...
      prune_one() ->
        put_tree()
      ...
  after trim_marked() returns, should not call put_tree() again.
  but after trim_marked() returns, it has to 'goto Err' to call additional put_tree().
  so it has to call additional get_tree() firstly.

And get_tree() need not be in audit_filter_mutex lock protected: it is only for self task to call trim_marked().

All get_tree() and put_tree() in audit_add_tree_rule() are matched with each other.


But we need let 'rule->tree = NULL;' firstly, so can protect rule itself freed in kill_rules().

  the reason is when it is killed, the 'rule' itself may have already released, we should not access it.
  one example: we add a rule to an inode, just at the same time the other task is deleting this inode.

  the work flow for add a rule:

    audit_receive() -> (need audit_cmd_mutex lock)
      audit_receive_skb() ->
        audit_receive_msg() ->
          audit_receive_filter() ->
            audit_add_rule() ->
              audit_add_tree_rule() -> (need audit_filter_mutex lock)
                ...
                unlock audit_filter_mutex
                get_tree()
                ...
                iterate_mounts() -> (iterate all related inodes)
                  tag_mount() ->
                    tag_trunk() ->
                      create_trunk() -> (assume it is 1st rule)
                        fsnotify_add_mark() -> 
                          fsnotify_add_inode_mark() ->  (add mark to inode->i_fsnotify_marks)
                        ...
                        get_tree(); (each inode will get one)
                ...
                lock audit_filter_mutex

  the work flow for delete inode:

    __destroy_inode() ->
     fsnotify_inode_delete() ->
       __fsnotify_inode_delete() ->
        fsnotify_clear_marks_by_inode() ->  (get mark from inode->i_fsnotify_marks)
          fsnotify_destroy_mark() ->
           fsnotify_destroy_mark_locked() ->
             audit_tree_freeing_mark() ->
               evict_chunk() ->
                 ...
                 tree->goner = 1
                 ...
                 kill_rules() ->   (assume current->audit_context == NULL)
                   call_rcu() ->   (rule->tree != NULL)
                     audit_free_rule_rcu() ->
                       audit_free_rule()
                 ...
                 audit_schedule_prune() ->  (assume current->audit_context == NULL)
                   kthread_run() ->    (need audit_cmd_mutex and audit_filter_mutex lock)
                     prune_one() ->    (delete it from prue_list)
                       put_tree(); (match the original get_tree above)


 
-----------------------------diff begin---------------------------------

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 9fd17f0..85eb26c 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -659,6 +659,7 @@ int audit_add_tree_rule(struct audit_krule *rule)
 	struct vfsmount *mnt;
 	int err;
 
+	rule->tree = NULL;
 	list_for_each_entry(tree, &tree_list, list) {
 		if (!strcmp(seed->pathname, tree->pathname)) {
 			put_tree(seed);

-----------------------------diff end-----------------------------------


For me, after 'rule->tree = NULL', all things seems fine !!

Please help check.

Thanks.


> after reading code again, I think:
> 
>   we can remove the pair of get_tree() and put_tree(),
> 
>     a. the author want to pretect tree not to be freed after unlock audit_filter_mutex
>          when tree is really freed by others, we have chance to check tree->goner
> 
>     b. it just like another functions done (audit_tag_tree, audit_trim_trees)
>          for audit_add_tree_rule, also need let get_tree() protected by audit_filter_mutex.
>            (move 'get_tree' before unlock audit_filter_mutex)
> 
>     c. but after read code (at least for audit_add_tree_rule)
>        during unlock audit_filter_mutex in audit_add_tree_rule:
>          I find only one posible related work flow which may happen (maybe it can not happen either).
>            fsnotify_destroy_mark_locked ->
>              audit_tree_freeing_mark ->
>                evict_chunk ->
>                  kill_rules ->
>                    call_rcu -> 
>                      audit_free_rule_rcu ->
>                        audit_free_rule
>          it will free rules and entry, but it does not call put_tree().
> 
>     so I think, at least for audit_add_tree_rule, we can remove the pair of get_tree() and put_tree()
>       (maybe also need give a check for audit_tag_tree and audit_trim_trees, whether can remove 'get_tree' too)
> 
> 
>   and the process is incorrect after lock audit_filter_mutex again (line 701..705)
> 
>     a. if rule->rlist was really empty
>         the 'rule' itself would already be freed.
>         the caller and the caller of caller, need notice this (not double free)
>         instead, we need check tree->gonar (and also need spin_lock protected).
> 
>     b. we do not need set "rule->tree = tree" again.
>          i. if 'rule' is not touched by any other thread
>               it should be rule->tree == tree.
> 
>         ii. else if rule->tree == NULL (freed by other thread)
>               'rule' itself might be freed too, we'd better return by failure.
> 
>        iii. else (!rule->tree && rule->tree != tree) (reused by other thread)
>               firstly, it should not happen.
>               if could happen, 'rule->tree = tree' would cause original rule->tree memory leak.
> 
> 
>   my conclusion is only by reading code (and still I am not quite expert about it, either)
>   so welcome experts (especially maintainers) to providing suggestions or completions.
> 
>   thanks.
> 
> 
>   if no reply within a week (2013-04-28), I should send related patch.
> 
> gchen.
> 
> 653 /* called with audit_filter_mutex */
> 654 int audit_add_tree_rule(struct audit_krule *rule)
> 655 {
> 656         struct audit_tree *seed = rule->tree, *tree;
> 657         struct path path;
> 658         struct vfsmount *mnt;
> 659         int err;
> 660 
> 661         list_for_each_entry(tree, &tree_list, list) {
> 662                 if (!strcmp(seed->pathname, tree->pathname)) {
> 663                         put_tree(seed);
> 664                         rule->tree = tree;
> 665                         list_add(&rule->rlist, &tree->rules);
> 666                         return 0;
> 667                 }
> 668         }
> 669         tree = seed;
> 670         list_add(&tree->list, &tree_list);
> 671         list_add(&rule->rlist, &tree->rules);
> 672         /* do not set rule->tree yet */
> 673         mutex_unlock(&audit_filter_mutex);
> 674 
> 675         err = kern_path(tree->pathname, 0, &path);
> 676         if (err)
> 677                 goto Err;
> 678         mnt = collect_mounts(&path);
> 679         path_put(&path);
> 680         if (IS_ERR(mnt)) {
> 681                 err = PTR_ERR(mnt);
> 682                 goto Err;
> 683         }
> 684 
> 685         get_tree(tree);
> 686         err = iterate_mounts(tag_mount, tree, mnt);
> 687         drop_collected_mounts(mnt);
> 688 
> 689         if (!err) {
> 690                 struct node *node;
> 691                 spin_lock(&hash_lock);
> 692                 list_for_each_entry(node, &tree->chunks, list)
> 693                         node->index &= ~(1U<<31);
> 694                 spin_unlock(&hash_lock);
> 695         } else {
> 696                 trim_marked(tree);
> 697                 goto Err;
> 698         }
> 699 
> 700         mutex_lock(&audit_filter_mutex);
> 701         if (list_empty(&rule->rlist)) {
> 702                 put_tree(tree);
> 703                 return -ENOENT;
> 704         }
> 705         rule->tree = tree;
> 706         put_tree(tree);
> 707 
> 708         return 0;
> 709 Err:
> 710         mutex_lock(&audit_filter_mutex);
> 711         list_del_init(&tree->list);
> 712         list_del_init(&tree->rules);
> 713         put_tree(tree);
> 714         return err;
> 715 }
> 
> 
> 
> 
>> >   excuse me:
>> >     I am not quite familiar with it, and also have to do another things.
>> >     so I have to spend additional time resource to make sure about it.
>> > 
>> >   is it ok ?
>> >     I should make sure about it within this week (2013-04-21)
>> >     I should finish related test (if need), within next week (2013-4-28)
>> > 
>> >   if have additional suggestions or completions, please reply.
>> >     (if no reply, I will follow the time point above)
>> > 
>> >   thanks.
>> > 
>> > gchen.
>> > 
>> > 
> 
> -- Chen Gang Asianux Corporation
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures

[Andrew Morton]

On Thu, 09 May 2013 20:53:06 +0800 Chen Gang <gang.chen@asianux.com> wrote:

> get_tree() in audit_add_tree_rule() is needed (my original conclusion is incorrect).

OK..

> But we need let 'rule->tree = NULL;' firstly, so can protect rule itself freed in kill_rules().

I'll believe you ;) I turned this into a proper patch and added your
(missed) Signed-off-by:.

> For me, after 'rule->tree = NULL', all things seems fine !!

Well, what was wrong before?  Is there some user-triggerable
misbehaviour which you observed?  If so, please describe it.

Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures

[Chen Gang]

On 05/10/2013 04:11 AM, Andrew Morton wrote:
>> > But we need let 'rule->tree = NULL;' firstly, so can protect rule itself freed in kill_rules().
> I'll believe you ;) I turned this into a proper patch and added your
> (missed) Signed-off-by:.
> 

Thanks.

At least, let 'rule->tree = NULL;' can:

  a. it matches 'rule->tree = tree;' which is before successful return.
       also can make 'if (list_empty(&rule->rlist))' reasonable.

  b. protect rule itself freed in kill_rules(), if it could happen.
       just like all 'rule->tree = NULL;' in audit_remove_tree_rule().

  c. it will no negative effect.


>> > For me, after 'rule->tree = NULL', all things seems fine !!
> Well, what was wrong before?  Is there some user-triggerable
> misbehaviour which you observed?  If so, please describe it.
> 
> 
> 

I think, it will cause issue (randomly): if when we are using auditctl
to add rule to monitor one file, and at the same time, the other user is
just deleting this file.

I guess, it is why original code need 'if (list_empty(&rule->rlist))'
after lock 'audit_filter_mutex' again.

Currently, I am just testing for it (and should give a test), and I will
send the test plan and test result within this week (2013-05-12).


Thanks.

-- 
Chen Gang

Asianux Corporation

Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures

[Chen Gang]

On 05/10/2013 10:08 AM, Chen Gang wrote:
> On 05/10/2013 04:11 AM, Andrew Morton wrote:
>>>> >> > For me, after 'rule->tree = NULL', all things seems fine !!
>> > Well, what was wrong before?  Is there some user-triggerable
>> > misbehaviour which you observed?  If so, please describe it.
>> > 
>> > 
>> > 

Oh, sorry, after have a test, the original code is no issue (it is my
fault).

When the deleting work flow call evict_chunk(), I assume that the
'postponed' can be NULL (at least, in some condition, it can), so
kill_rules() can be called directly. But in fact, 'postponed' will
never be NULL:

  audit_tree depend on CONFIG_AUDIT_TREE which depend on CONFIG_AUDITSYSCALL.
  if CONFIG_AUDITSYSCALL defined.
    do_fork() -> copy_process() -> audit_alloc() -> alloc 'audit_context'.
    so the audit_killed_tree() will return valid pointer to 'postponed'.

  although already have quite a few code for 'postponed == NULL', they are really useless now.

I also read all other work flow which related with kill_rule(), I can
not find any of them can lead audit_add_tree_rule() to cause issue: all
work flow related with kill_rule() are protected by audit_cmd_mutex now.


Test plan:
  code preparation:
    define a flag varaible.
    wait the flag to be true, before lock 'audit_filter_mutex' again. in audit_add_tree_rule()
    when evict_trunc() finish, set the flag true.
  firstly start: 'rm -rvf /tmp/gchen/linux-next'
  then start: 'audit -w /tmp/gchen/linux-next/drivers/char'
    (notice the order should not be changed, or all system call will be locked)

Test result:
  the evict_chunk() will not call kill_rule() directly, so no issues.
  the output sample result like this: ('printk' the related information)

---------------------------sample begin-----------------------------

[   85.455891] gchen_tag: ffff880099f0ddc0, audit_init_entry(): create entry: ffff880097ca2800
[   85.455900] gchen_tag: ffff880099f0ddc0, audit_receive_filter(): before call, type: 1011
[   85.455903] gchen_tag: ffff880099f0ddc0, audit_add_tree_rule(): enter function
[   85.455927] ida_remove called for id=0 which is not allocated.
[   85.455935] gchen_tag: ffff880099f0ddc0, audit_add_tree_rule(): begin waiting 100...., rule: ffff880097ca2820
[   91.425947] gchen_tag: ffff880097995dc0, fsnotify_clear_marks_by_inode(): set audit_test_count = true
[   91.425960] gchen_tag: ffff880099f0ddc0, audit_add_tree_rule(): end waiting, rule: ffff880097ca2820
[   91.426055] gchen_tag: ffff880099f0ddc0, audit_receive_filter(): after call for succeed, type: 1011
[   91.426558] gchen_tag: ffff880097995dc0, kill_rules(): list_del_init, rule: ffff880097ca2820, tree: ffff880099dfff00
[   91.426564] gchen_tag: ffff880097995dc0, kill_rules(): remove entry: ffff880097ca2800
[   91.431023] gchen_tag: ffff880097995dc0, audit_free_rule(): remove entry: ffff880097ca2800

---------------------------sample end-------------------------------


Now, my original fix makes the related code consistent, but the related
code maybe be useless now (if what I said is true, in audit, quite a
few of code are useless for this reason).

I can not be sure whether these useless code will be used, in the
future (whether let AUDIT_TREE and AUDIT_WATCH independent on
AUDIT_SYSCALL in the future).

If it will be used in the future, my fix is useful too, else we'd
better to delete the related useless code.

Thanks.

> I think, it will cause issue (randomly): if when we are using auditctl
> to add rule to monitor one file, and at the same time, the other user is
> just deleting this file.
> 
> I guess, it is why original code need 'if (list_empty(&rule->rlist))'
> after lock 'audit_filter_mutex' again.
> 
> Currently, I am just testing for it (and should give a test), and I will
> send the test plan and test result within this week (2013-05-12).
> 
> 
> Thanks.
> 
> -- Chen Gang Asianux Corporation
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures

[Chen Gang]

On 05/10/2013 05:50 PM, Chen Gang wrote:
> On 05/10/2013 10:08 AM, Chen Gang wrote:
>> On 05/10/2013 04:11 AM, Andrew Morton wrote:
>>>>>>>> For me, after 'rule->tree = NULL', all things seems fine !!
>>>> Well, what was wrong before?  Is there some user-triggerable
>>>> misbehaviour which you observed?  If so, please describe it.
>>>>
>>>>
>>>>
> 

Oh, sorry again, the 'postponed' in evict_chunk() still has a chance to
be NULL: firstly, 'audit_context->in_syscall' also checked in
audit_killed_trees(), and also not all tasks are generated by do_fork().

But really, for most cases, the 'postponed' is not NULL, so my test case
can not cause issue.

Currently, I just force 'postponed' to be NULL to see the test result... :-)

It seems my original fix is still useful ! ;-)

Thanks.

> Oh, sorry, after have a test, the original code is no issue (it is my
> fault).
> 
> When the deleting work flow call evict_chunk(), I assume that the
> 'postponed' can be NULL (at least, in some condition, it can), so
> kill_rules() can be called directly. But in fact, 'postponed' will
> never be NULL:
> 
>   audit_tree depend on CONFIG_AUDIT_TREE which depend on CONFIG_AUDITSYSCALL.
>   if CONFIG_AUDITSYSCALL defined.
>     do_fork() -> copy_process() -> audit_alloc() -> alloc 'audit_context'.
>     so the audit_killed_tree() will return valid pointer to 'postponed'.
> 
>   although already have quite a few code for 'postponed == NULL', they are really useless now.
> 
> I also read all other work flow which related with kill_rule(), I can
> not find any of them can lead audit_add_tree_rule() to cause issue: all
> work flow related with kill_rule() are protected by audit_cmd_mutex now.
> 
> 
> Test plan:
>   code preparation:
>     define a flag varaible.
>     wait the flag to be true, before lock 'audit_filter_mutex' again. in audit_add_tree_rule()
>     when evict_trunc() finish, set the flag true.
>   firstly start: 'rm -rvf /tmp/gchen/linux-next'
>   then start: 'audit -w /tmp/gchen/linux-next/drivers/char'
>     (notice the order should not be changed, or all system call will be locked)
> 
> Test result:
>   the evict_chunk() will not call kill_rule() directly, so no issues.
>   the output sample result like this: ('printk' the related information)
> 
> ---------------------------sample begin-----------------------------
> 
> [   85.455891] gchen_tag: ffff880099f0ddc0, audit_init_entry(): create entry: ffff880097ca2800
> [   85.455900] gchen_tag: ffff880099f0ddc0, audit_receive_filter(): before call, type: 1011
> [   85.455903] gchen_tag: ffff880099f0ddc0, audit_add_tree_rule(): enter function
> [   85.455927] ida_remove called for id=0 which is not allocated.
> [   85.455935] gchen_tag: ffff880099f0ddc0, audit_add_tree_rule(): begin waiting 100...., rule: ffff880097ca2820
> [   91.425947] gchen_tag: ffff880097995dc0, fsnotify_clear_marks_by_inode(): set audit_test_count = true
> [   91.425960] gchen_tag: ffff880099f0ddc0, audit_add_tree_rule(): end waiting, rule: ffff880097ca2820
> [   91.426055] gchen_tag: ffff880099f0ddc0, audit_receive_filter(): after call for succeed, type: 1011
> [   91.426558] gchen_tag: ffff880097995dc0, kill_rules(): list_del_init, rule: ffff880097ca2820, tree: ffff880099dfff00
> [   91.426564] gchen_tag: ffff880097995dc0, kill_rules(): remove entry: ffff880097ca2800
> [   91.431023] gchen_tag: ffff880097995dc0, audit_free_rule(): remove entry: ffff880097ca2800
> 
> ---------------------------sample end-------------------------------
> 
> 
> Now, my original fix makes the related code consistent, but the related
> code maybe be useless now (if what I said is true, in audit, quite a
> few of code are useless for this reason).
> 
> I can not be sure whether these useless code will be used, in the
> future (whether let AUDIT_TREE and AUDIT_WATCH independent on
> AUDIT_SYSCALL in the future).
> 
> If it will be used in the future, my fix is useful too, else we'd
> better to delete the related useless code.
> 
> Thanks.
> 
>> I think, it will cause issue (randomly): if when we are using auditctl
>> to add rule to monitor one file, and at the same time, the other user is
>> just deleting this file.
>>
>> I guess, it is why original code need 'if (list_empty(&rule->rlist))'
>> after lock 'audit_filter_mutex' again.
>>
>> Currently, I am just testing for it (and should give a test), and I will
>> send the test plan and test result within this week (2013-05-12).
>>
>>
>> Thanks.
>>
>> -- Chen Gang Asianux Corporation
>>
> 
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures

[Chen Gang]

On 05/10/2013 07:29 PM, Chen Gang wrote:
> On 05/10/2013 05:50 PM, Chen Gang wrote:
>> On 05/10/2013 10:08 AM, Chen Gang wrote:
>>> On 05/10/2013 04:11 AM, Andrew Morton wrote:
>>>>>>>>> For me, after 'rule->tree = NULL', all things seems fine !!
>>>>> Well, what was wrong before?  Is there some user-triggerable
>>>>> misbehaviour which you observed?  If so, please describe it.
>>>>>
>>>>>
>>>>>
>>
> 


If we force 'postponed' to be NUL, and still use the original test plan:

>> Test plan:
>>   code preparation:
>>     define a flag varaible.
>>     wait the flag to be true, before lock 'audit_filter_mutex' again. in audit_add_tree_rule()
>>     when evict_trunc() finish, set the flag true.
>>   firstly start: 'rm -rvf /tmp/gchen/linux-next'
>>   then start: 'audit -w /tmp/gchen/linux-next/drivers/char'
>>     (notice the order should not be changed, or all system call will be locked)

If not set 'rule->tree = NULL', it will cause issue: kernel will die !!.

But if set 'rule->tree = NULL', all things seems OK, and the output log
is just what we expected.

---------------------------test result begin--------------------------------

                          task_struct ptr:   function():         action:       related value:

[  627.422698] gchen_tag: ffff88009b8787a0, audit_init_entry(): create entry: ffff88008bc75600
[  627.422712] gchen_tag: ffff88009b8787a0, audit_receive_filter(): before call, type: 1011
[  627.422718] gchen_tag: ffff88009b8787a0, audit_add_tree_rule(): enter function
[  627.422822] ida_remove called for id=0 which is not allocated.
[  627.422834] gchen_tag: ffff88009b8787a0, audit_add_tree_rule(): begin waiting 100...., rule: ffff88008bc75620
[  639.421114] gchen_tag: ffff88009b878000, evict_chunk(): enter function postponed:           (null)
[  639.421197] gchen_tag: ffff88009b878000, evict_chunk(): kill_rull postponed:           (null)
[  639.421209] gchen_tag: ffff88009b878000, kill_rules(): list_del_init, rule: ffff88008bc75620, tree:           (null)
[  639.421213] gchen_tag: ffff88009b878000, evict_chunk(): audit_schedule_prune postponed:           (null)
[  639.421274] gchen_tag: ffff88009b878000, evict_chunk(): set audit_test_count = true, postponed:           (null)
[  639.421282] gchen_tag: ffff88009b8787a0, audit_add_tree_rule(): end waiting, rule: ffff88008bc75620
[  639.421289] gchen_tag: ffff88009b8787a0, audit_add_tree_rule(): list empty for rule->rlist and return fail: ffff88008bc75620
[  639.421364] gchen_tag: ffff88009b8787a0, audit_receive_filter(): after call for failure, type: 1011
[  639.421369] gchen_tag: ffff88009b8787a0, audit_free_rule(): remove entry: ffff88008bc75600

---------------------------test result end----------------------------------

Next, I will try to find a test case to let 'postponed' to NUL by
itself (not with hard coded by force), then test it again :-).

Thanks.


> Oh, sorry again, the 'postponed' in evict_chunk() still has a chance to
> be NULL: firstly, 'audit_context->in_syscall' also checked in
> audit_killed_trees(), and also not all tasks are generated by do_fork().
> 
> But really, for most cases, the 'postponed' is not NULL, so my test case
> can not cause issue.
> 
> Currently, I just force 'postponed' to be NULL to see the test result... :-)
> 
> It seems my original fix is still useful ! ;-)
> 
> Thanks.
> 
>> Oh, sorry, after have a test, the original code is no issue (it is my
>> fault).
>>
>> When the deleting work flow call evict_chunk(), I assume that the
>> 'postponed' can be NULL (at least, in some condition, it can), so
>> kill_rules() can be called directly. But in fact, 'postponed' will
>> never be NULL:
>>
>>   audit_tree depend on CONFIG_AUDIT_TREE which depend on CONFIG_AUDITSYSCALL.
>>   if CONFIG_AUDITSYSCALL defined.
>>     do_fork() -> copy_process() -> audit_alloc() -> alloc 'audit_context'.
>>     so the audit_killed_tree() will return valid pointer to 'postponed'.
>>
>>   although already have quite a few code for 'postponed == NULL', they are really useless now.
>>
>> I also read all other work flow which related with kill_rule(), I can
>> not find any of them can lead audit_add_tree_rule() to cause issue: all
>> work flow related with kill_rule() are protected by audit_cmd_mutex now.
>>
>>
>> Test plan:
>>   code preparation:
>>     define a flag varaible.
>>     wait the flag to be true, before lock 'audit_filter_mutex' again. in audit_add_tree_rule()
>>     when evict_trunc() finish, set the flag true.
>>   firstly start: 'rm -rvf /tmp/gchen/linux-next'
>>   then start: 'audit -w /tmp/gchen/linux-next/drivers/char'
>>     (notice the order should not be changed, or all system call will be locked)
>>
>> Test result:
>>   the evict_chunk() will not call kill_rule() directly, so no issues.
>>   the output sample result like this: ('printk' the related information)
>>
>> ---------------------------sample begin-----------------------------
>>
>> [   85.455891] gchen_tag: ffff880099f0ddc0, audit_init_entry(): create entry: ffff880097ca2800
>> [   85.455900] gchen_tag: ffff880099f0ddc0, audit_receive_filter(): before call, type: 1011
>> [   85.455903] gchen_tag: ffff880099f0ddc0, audit_add_tree_rule(): enter function
>> [   85.455927] ida_remove called for id=0 which is not allocated.
>> [   85.455935] gchen_tag: ffff880099f0ddc0, audit_add_tree_rule(): begin waiting 100...., rule: ffff880097ca2820
>> [   91.425947] gchen_tag: ffff880097995dc0, fsnotify_clear_marks_by_inode(): set audit_test_count = true
>> [   91.425960] gchen_tag: ffff880099f0ddc0, audit_add_tree_rule(): end waiting, rule: ffff880097ca2820
>> [   91.426055] gchen_tag: ffff880099f0ddc0, audit_receive_filter(): after call for succeed, type: 1011
>> [   91.426558] gchen_tag: ffff880097995dc0, kill_rules(): list_del_init, rule: ffff880097ca2820, tree: ffff880099dfff00
>> [   91.426564] gchen_tag: ffff880097995dc0, kill_rules(): remove entry: ffff880097ca2800
>> [   91.431023] gchen_tag: ffff880097995dc0, audit_free_rule(): remove entry: ffff880097ca2800
>>
>> ---------------------------sample end-------------------------------
>>
>>
>> Now, my original fix makes the related code consistent, but the related
>> code maybe be useless now (if what I said is true, in audit, quite a
>> few of code are useless for this reason).
>>
>> I can not be sure whether these useless code will be used, in the
>> future (whether let AUDIT_TREE and AUDIT_WATCH independent on
>> AUDIT_SYSCALL in the future).
>>
>> If it will be used in the future, my fix is useful too, else we'd
>> better to delete the related useless code.
>>
>> Thanks.
>>
>>> I think, it will cause issue (randomly): if when we are using auditctl
>>> to add rule to monitor one file, and at the same time, the other user is
>>> just deleting this file.
>>>
>>> I guess, it is why original code need 'if (list_empty(&rule->rlist))'
>>> after lock 'audit_filter_mutex' again.
>>>
>>> Currently, I am just testing for it (and should give a test), and I will
>>> send the test plan and test result within this week (2013-05-12).
>>>
>>>
>>> Thanks.
>>>
>>> -- Chen Gang Asianux Corporation
>>>
>>
>>
> 
> 


-- 
Chen Gang

Asianux Corporation