* [PATCH] kernel/audit_tree.c: tree will memory leak when failure occurs for audit_trim_trees() @ 2013-04-19 9:39 Chen Gang 2013-04-22 23:04 ` Andrew Morton 0 siblings, 1 reply; 10+ messages in thread From: Chen Gang @ 2013-04-19 9:39 UTC (permalink / raw) To: Eric Paris, Al Viro; +Cc: Andrew Morton, linux-kernel 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] kernel/audit_tree.c: tree will memory leak when failure occurs for audit_trim_trees() 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-04-23 1:46 ` Chen Gang ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Andrew Morton @ 2013-04-22 23:04 UTC (permalink / raw) To: Chen Gang; +Cc: Eric Paris, Al Viro, linux-kernel 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); _ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] kernel/audit_tree.c: tree will memory leak when failure occurs for audit_trim_trees() 2013-04-22 23:04 ` Andrew Morton @ 2013-04-23 1:46 ` Chen Gang 2013-05-06 7:41 ` [PATCH kernel-next] kernel/audit_tree.c: fix the original version merging issue for put_tree() Chen Gang 2013-05-09 12:53 ` [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures Chen Gang 2 siblings, 0 replies; 10+ messages in thread From: Chen Gang @ 2013-04-23 1:46 UTC (permalink / raw) To: Andrew Morton; +Cc: Eric Paris, Al Viro, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH kernel-next] kernel/audit_tree.c: fix the original version merging issue for put_tree(). 2013-04-22 23:04 ` Andrew Morton 2013-04-23 1:46 ` Chen Gang @ 2013-05-06 7:41 ` Chen Gang 2013-05-09 12:53 ` [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures Chen Gang 2 siblings, 0 replies; 10+ messages in thread From: Chen Gang @ 2013-05-06 7:41 UTC (permalink / raw) To: Andrew Morton; +Cc: Eric Paris, Al Viro, linux-kernel 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 ^ permalink raw reply related [flat|nested] 10+ 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-04-23 1:46 ` Chen Gang 2013-05-06 7:41 ` [PATCH kernel-next] kernel/audit_tree.c: fix the original version merging issue for put_tree() Chen Gang @ 2013-05-09 12:53 ` Chen Gang 2013-05-09 20:11 ` Andrew Morton 2 siblings, 1 reply; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ messages in thread
end of thread, other threads:[~2013-05-13 2:54 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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-04-23 1:46 ` Chen Gang 2013-05-06 7:41 ` [PATCH kernel-next] kernel/audit_tree.c: fix the original version merging issue for put_tree() Chen Gang 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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.