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