Date:	Mon, 13 May 2013 10:54:02 +0800
From:	Chen Gang <gang.chen@...anux.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	Eric Paris <eparis@...hat.com>, Al Viro <viro@...iv.linux.org.uk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] kernel: audit_tree: resource management: need put_tree
 and goto Err when failure occures

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists