* [PATCH] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures
@ 2013-04-12 4:43 Chen Gang
2013-04-12 4:50 ` [PATCH] kernel: audit_watch: resource management: better reset to NULL Chen Gang
2013-04-16 7:35 ` [PATCH] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures Chen Gang
0 siblings, 2 replies; 17+ messages in thread
From: Chen Gang @ 2013-04-12 4:43 UTC (permalink / raw)
To: Eric Paris, Al Viro, linux-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH] kernel: audit_watch: resource management: better reset to NULL.
2013-04-12 4:43 [PATCH] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures Chen Gang
@ 2013-04-12 4:50 ` Chen Gang
2013-04-12 8:18 ` Chen Gang
2013-04-16 7:35 ` [PATCH] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures Chen Gang
1 sibling, 1 reply; 17+ messages in thread
From: Chen Gang @ 2013-04-12 4:50 UTC (permalink / raw)
To: Eric Paris, Al Viro, linux-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] kernel: audit_watch: resource management: better reset to NULL.
2013-04-12 4:50 ` [PATCH] kernel: audit_watch: resource management: better reset to NULL Chen Gang
@ 2013-04-12 8:18 ` Chen Gang
2013-04-12 8:56 ` [PATCH] kernel: auditfilter: resource management, need process tree when audit_add_watch failed in audit_add_rule Chen Gang
0 siblings, 1 reply; 17+ messages in thread
From: Chen Gang @ 2013-04-12 8:18 UTC (permalink / raw)
To: Eric Paris, Al Viro, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] kernel: auditfilter: resource management, need process tree when audit_add_watch failed in audit_add_rule
2013-04-12 8:18 ` Chen Gang
@ 2013-04-12 8:56 ` Chen Gang
2013-04-17 4:26 ` Chen Gang
0 siblings, 1 reply; 17+ messages in thread
From: Chen Gang @ 2013-04-12 8:56 UTC (permalink / raw)
To: Eric Paris, Al Viro, linux-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures
2013-04-12 4:43 [PATCH] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures Chen Gang
2013-04-12 4:50 ` [PATCH] kernel: audit_watch: resource management: better reset to NULL Chen Gang
@ 2013-04-16 7:35 ` Chen Gang
2013-04-17 4:04 ` [PATCH v2] " Chen Gang
1 sibling, 1 reply; 17+ messages in thread
From: Chen Gang @ 2013-04-16 7:35 UTC (permalink / raw)
To: Eric Paris, Al Viro, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures
2013-04-16 7:35 ` [PATCH] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures Chen Gang
@ 2013-04-17 4:04 ` Chen Gang
2013-04-17 20:07 ` Andrew Morton
0 siblings, 1 reply; 17+ messages in thread
From: Chen Gang @ 2013-04-17 4:04 UTC (permalink / raw)
To: Eric Paris, Al Viro; +Cc: linux-kernel, Andrew Morton
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] kernel: auditfilter: resource management, need process tree when audit_add_watch failed in audit_add_rule
2013-04-12 8:56 ` [PATCH] kernel: auditfilter: resource management, need process tree when audit_add_watch failed in audit_add_rule Chen Gang
@ 2013-04-17 4:26 ` Chen Gang
0 siblings, 0 replies; 17+ messages in thread
From: Chen Gang @ 2013-04-17 4:26 UTC (permalink / raw)
To: Eric Paris, Al Viro; +Cc: linux-kernel, Andrew Morton
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures
2013-04-17 4:04 ` [PATCH v2] " Chen Gang
@ 2013-04-17 20:07 ` Andrew Morton
2013-04-18 1:19 ` Chen Gang F T
0 siblings, 1 reply; 17+ messages in thread
From: Andrew Morton @ 2013-04-17 20:07 UTC (permalink / raw)
To: Chen Gang; +Cc: Eric Paris, Al Viro, linux-kernel
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:
_
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures
2013-04-17 20:07 ` Andrew Morton
@ 2013-04-18 1:19 ` Chen Gang F T
2013-04-20 7:31 ` Chen Gang
0 siblings, 1 reply; 17+ messages in thread
From: Chen Gang F T @ 2013-04-18 1:19 UTC (permalink / raw)
To: Andrew Morton; +Cc: Chen Gang, Eric Paris, Al Viro, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures
2013-04-18 1:19 ` Chen Gang F T
@ 2013-04-20 7:31 ` Chen Gang
2013-04-23 3:51 ` Chen Gang
0 siblings, 1 reply; 17+ messages in thread
From: Chen Gang @ 2013-04-20 7:31 UTC (permalink / raw)
To: Chen Gang F T; +Cc: Andrew Morton, Eric Paris, Al Viro, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures
2013-04-20 7:31 ` Chen Gang
@ 2013-04-23 3:51 ` Chen Gang
0 siblings, 0 replies; 17+ messages in thread
From: Chen Gang @ 2013-04-23 3:51 UTC (permalink / raw)
To: Chen Gang F T, Andrew Morton; +Cc: Eric Paris, Al Viro, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures
2013-05-10 11:29 ` Chen Gang
@ 2013-05-13 2:54 ` Chen Gang
0 siblings, 0 replies; 17+ messages in thread
From: Chen Gang @ 2013-05-13 2:54 UTC (permalink / raw)
To: Andrew Morton; +Cc: Eric Paris, Al Viro, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures
2013-05-10 9:50 ` Chen Gang
@ 2013-05-10 11:29 ` Chen Gang
2013-05-13 2:54 ` Chen Gang
0 siblings, 1 reply; 17+ messages in thread
From: Chen Gang @ 2013-05-10 11:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Eric Paris, Al Viro, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures
2013-05-10 2:08 ` Chen Gang
@ 2013-05-10 9:50 ` Chen Gang
2013-05-10 11:29 ` Chen Gang
0 siblings, 1 reply; 17+ messages in thread
From: Chen Gang @ 2013-05-10 9:50 UTC (permalink / raw)
To: Andrew Morton; +Cc: Eric Paris, Al Viro, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures
2013-05-09 20:11 ` Andrew Morton
@ 2013-05-10 2:08 ` Chen Gang
2013-05-10 9:50 ` Chen Gang
0 siblings, 1 reply; 17+ messages in thread
From: Chen Gang @ 2013-05-10 2:08 UTC (permalink / raw)
To: Andrew Morton; +Cc: Eric Paris, Al Viro, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures
2013-05-09 12:53 ` [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures Chen Gang
@ 2013-05-09 20:11 ` Andrew Morton
2013-05-10 2:08 ` Chen Gang
0 siblings, 1 reply; 17+ messages in thread
From: Andrew Morton @ 2013-05-09 20:11 UTC (permalink / raw)
To: Chen Gang; +Cc: Eric Paris, Al Viro, linux-kernel
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures
2013-04-22 23:04 ` Andrew Morton
@ 2013-05-09 12:53 ` Chen Gang
2013-05-09 20:11 ` Andrew Morton
0 siblings, 1 reply; 17+ messages in thread
From: Chen Gang @ 2013-05-09 12:53 UTC (permalink / raw)
To: Andrew Morton; +Cc: Eric Paris, Al Viro, linux-kernel
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2013-05-13 2:54 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-12 4:43 [PATCH] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures Chen Gang
2013-04-12 4:50 ` [PATCH] kernel: audit_watch: resource management: better reset to NULL Chen Gang
2013-04-12 8:18 ` Chen Gang
2013-04-12 8:56 ` [PATCH] kernel: auditfilter: resource management, need process tree when audit_add_watch failed in audit_add_rule Chen Gang
2013-04-17 4:26 ` Chen Gang
2013-04-16 7:35 ` [PATCH] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures Chen Gang
2013-04-17 4:04 ` [PATCH v2] " Chen Gang
2013-04-17 20:07 ` Andrew Morton
2013-04-18 1:19 ` Chen Gang F T
2013-04-20 7:31 ` Chen Gang
2013-04-23 3:51 ` Chen Gang
2013-04-19 9:39 [PATCH] kernel/audit_tree.c: tree will memory leak when failure occurs for audit_trim_trees() Chen Gang
2013-04-22 23:04 ` Andrew Morton
2013-05-09 12:53 ` [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures Chen Gang
2013-05-09 20:11 ` Andrew Morton
2013-05-10 2:08 ` Chen Gang
2013-05-10 9:50 ` Chen Gang
2013-05-10 11:29 ` Chen Gang
2013-05-13 2:54 ` Chen Gang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).