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

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

[Chen Gang]

  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.

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

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 642a89c..3729d49 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -694,13 +694,15 @@ int audit_add_tree_rule(struct audit_krule *rule)
 		spin_unlock(&hash_lock);
 	} else {
 		trim_marked(tree);
+		put_tree(tree);
 		goto Err;
 	}
 
 	mutex_lock(&audit_filter_mutex);
 	if (list_empty(&rule->rlist)) {
 		put_tree(tree);
-		return -ENOENT;
+		err = -ENOENT;
+		got Err1;
 	}
 	rule->tree = tree;
 	put_tree(tree);
@@ -708,6 +710,7 @@ int audit_add_tree_rule(struct audit_krule *rule)
 	return 0;
 Err:
 	mutex_lock(&audit_filter_mutex);
+Err1:
 	list_del_init(&tree->list);
 	list_del_init(&tree->rules);
 	put_tree(tree);
-- 
1.7.7.6

[PATCH] kernel: audit_watch: resource management: better reset to NULL.

[Chen Gang]

  better to set krule->watch = NULL.
  maybe it is not a real issue, but can make code clearer,
  so can help the readers to analyse another issues.

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

diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 22831c4..abd8fdb 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -456,6 +456,7 @@ void audit_remove_watch_rule(struct audit_krule *krule)
 
 	if (list_empty(&watch->rules)) {
 		audit_remove_watch(watch);
+		krule->watch = NULL;
 
 		if (list_empty(&parent->watches)) {
 			audit_get_parent(parent);
-- 
1.7.7.6

Re: [PATCH] kernel: audit_watch: resource management: better reset to NULL.

[Chen Gang]

  oh, sorry, this patch is incorrect, please skip it.


On 2013年04月12日 12:50, Chen Gang wrote:
> 
>   better to set krule->watch = NULL.
>   maybe it is not a real issue, but can make code clearer,
>   so can help the readers to analyse another issues.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/audit_watch.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 22831c4..abd8fdb 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -456,6 +456,7 @@ void audit_remove_watch_rule(struct audit_krule *krule)
>  
>  	if (list_empty(&watch->rules)) {
>  		audit_remove_watch(watch);
> +		krule->watch = NULL;
>  
>  		if (list_empty(&parent->watches)) {
>  			audit_get_parent(parent);
> 


-- 
Chen Gang

Asianux Corporation

[PATCH] kernel: auditfilter: resource management, need process tree when audit_add_watch failed in audit_add_rule

[Chen Gang]

  need call audit_put_tree, if tree is valid.
  just like another area have done in function audit_add_rule.

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

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index f9fc54b..81f63f9 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -952,6 +952,12 @@ static inline int audit_add_rule(struct audit_entry *entry)
 		err = audit_add_watch(&entry->rule, &list);
 		if (err) {
 			mutex_unlock(&audit_filter_mutex);
+			/*
+			 * normally audit_add_tree_rule() will free it
+			 * on failure
+			 */
+			if (tree)
+				audit_put_tree(tree);
 			goto error;
 		}
 	}
-- 
1.7.7.6

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

[Chen Gang]

  oh, sorry, this patch need improving (got --> goto)

  and I should compile, install, running in normal condition, and be
sure of no additional issues occur at least.


On 2013年04月12日 12:43, Chen Gang 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.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/audit_tree.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 642a89c..3729d49 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -694,13 +694,15 @@ int audit_add_tree_rule(struct audit_krule *rule)
>  		spin_unlock(&hash_lock);
>  	} else {
>  		trim_marked(tree);
> +		put_tree(tree);
>  		goto Err;
>  	}
>  
>  	mutex_lock(&audit_filter_mutex);
>  	if (list_empty(&rule->rlist)) {
>  		put_tree(tree);
> -		return -ENOENT;
> +		err = -ENOENT;
> +		got Err1;
>  	}
>  	rule->tree = tree;
>  	put_tree(tree);
> @@ -708,6 +710,7 @@ int audit_add_tree_rule(struct audit_krule *rule)
>  	return 0;
>  Err:
>  	mutex_lock(&audit_filter_mutex);
> +Err1:
>  	list_del_init(&tree->list);
>  	list_del_init(&tree->rules);
>  	put_tree(tree);
> 


-- 
Chen Gang

Asianux Corporation

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

[Chen Gang]

  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.

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

diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 642a89c..9dfb0da 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -694,13 +694,15 @@ int audit_add_tree_rule(struct audit_krule *rule)
 		spin_unlock(&hash_lock);
 	} else {
 		trim_marked(tree);
+		put_tree(tree);
 		goto Err;
 	}
 
 	mutex_lock(&audit_filter_mutex);
 	if (list_empty(&rule->rlist)) {
 		put_tree(tree);
-		return -ENOENT;
+		err = -ENOENT;
+		goto Err1;
 	}
 	rule->tree = tree;
 	put_tree(tree);
@@ -708,6 +710,7 @@ int audit_add_tree_rule(struct audit_krule *rule)
 	return 0;
 Err:
 	mutex_lock(&audit_filter_mutex);
+Err1:
 	list_del_init(&tree->list);
 	list_del_init(&tree->rules);
 	put_tree(tree);
-- 
1.7.7.6

Re: [PATCH] kernel: auditfilter: resource management, need process tree when audit_add_watch failed in audit_add_rule

[Chen Gang]

  also please help checking this patch, when you have time.

  thanks.

On 2013年04月12日 16:56, Chen Gang wrote:
> 
>   need call audit_put_tree, if tree is valid.
>   just like another area have done in function audit_add_rule.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/auditfilter.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index f9fc54b..81f63f9 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -952,6 +952,12 @@ static inline int audit_add_rule(struct audit_entry *entry)
>  		err = audit_add_watch(&entry->rule, &list);
>  		if (err) {
>  			mutex_unlock(&audit_filter_mutex);
> +			/*
> +			 * normally audit_add_tree_rule() will free it
> +			 * on failure
> +			 */
> +			if (tree)
> +				audit_put_tree(tree);
>  			goto error;
>  		}
>  	}
> 


-- 
Chen Gang

Asianux Corporation

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

[Andrew Morton]

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:

--- a/kernel/audit_tree.c~a
+++ a/kernel/audit_tree.c
@@ -682,7 +682,6 @@ int audit_add_tree_rule(struct audit_kru
 		goto Err;
 	}
 
-	get_tree(tree);
 	err = iterate_mounts(tag_mount, tree, mnt);
 	drop_collected_mounts(mnt);
 
@@ -703,7 +702,6 @@ int audit_add_tree_rule(struct audit_kru
 		return -ENOENT;
 	}
 	rule->tree = tree;
-	put_tree(tree);
 
 	return 0;
 Err:
_

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

[Chen Gang F T]

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

  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.


> --- a/kernel/audit_tree.c~a
> +++ a/kernel/audit_tree.c
> @@ -682,7 +682,6 @@ int audit_add_tree_rule(struct audit_kru
>  		goto Err;
>  	}
>  
> -	get_tree(tree);
>  	err = iterate_mounts(tag_mount, tree, mnt);
>  	drop_collected_mounts(mnt);
>  
> @@ -703,7 +702,6 @@ int audit_add_tree_rule(struct audit_kru
>  		return -ENOENT;
>  	}
>  	rule->tree = tree;
> -	put_tree(tree);
>  
>  	return 0;
>  Err:
> _
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


-- 
Chen Gang

Flying Transformer

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

[Chen Gang]

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


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

Re: [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:
>>>> >> > 
> 

I hope the maintainers to provide your suggestions or completion. if we
get non-reply from any maintainers, I should continue to analyse.

if we continue analyse independent with original maintainers,
I think, before get a conclusion:

  I should understand the API of audit: what it is, how to use it (test
it), know about all of sub-systems which are related with it (such as fs
sub-system);

  also should know about the design of audit (read through the code),

  and should analyse the lower-level design of audit tree (read the code
completely).

and excuse me, I am not quite familiar with audit, now, and also have to
plan to do another things, so I think I should get a conclusion within
next month (2013-05-31), is it ok ?


BTW:
  my plan for 1st half of 2013 is mainly for familiar environments: can
provide contributes to many various sub systems (e.g. arm, powerpc,
drivers, net, kernel ...)

  my plan for 2nd half of 2013 is mainly for familiar kernel: should
analyse the deeper issues and kernel API/design step by step (e.g. mm,
fs, kernel API documents...)

  the original related mail:
http://linux-kernel.2935.n7.nabble.com/Consult-Plan-personal-contributes-plan-for-2013-td587319.html


> 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