Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754616Ab3DWDvz (ORCPT ); Mon, 22 Apr 2013 23:51:55 -0400 Received: from intranet.asianux.com ([58.214.24.6]:53950 "EHLO intranet.asianux.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754478Ab3DWDvy (ORCPT ); Mon, 22 Apr 2013 23:51:54 -0400 X-Spam-Score: -100.8 Message-ID: <51760528.8000304@asianux.com> Date: Tue, 23 Apr 2013 11:51:04 +0800 From: Chen Gang User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: Chen Gang F T , Andrew Morton CC: Eric Paris , Al Viro , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree and goto Err when failure occures References: <516790ED.6060202@asianux.com> <516CFF2C.7010509@asianux.com> <516E1F32.6010009@asianux.com> <20130417130739.03fb8d9ccb908afc0a1db861@linux-foundation.org> <516F4A28.2050806@gmail.com> <5172446D.2070903@asianux.com> In-Reply-To: <5172446D.2070903@asianux.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7517 Lines: 201 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 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 -- 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/