[PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

  in the 'fcount' looping,
    if 'new->fields[*].type" has 2 or more AUDIT_FILTERKEYs
    need judge new->filterkey whether has value, or memory leak.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/auditfilter.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index f9fc54b..936ac79 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -859,6 +859,8 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
 						       &old->fields[i]);
 			break;
 		case AUDIT_FILTERKEY:
+			if (new->filterkey)
+				break;
 			fk = kstrdup(old->filterkey, GFP_KERNEL);
 			if (unlikely(!fk))
 				err = -ENOMEM;
-- 
1.7.7.6

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

in another function: audit_data_to_entry:

  a. has the same issue for case AUDIT_WATCH.

  b. has an new issue for AUDIT_DIR:
       after AUDIT_DIR succeed, it will set rule->tree.
       next, the other case fail, then will call audit_free_rule.
       but audit_free_rule will not free rule->tree.


  I find them only by reading code, not test them.
  and I also do not know about the related features.
  so please help check my 2 opinions whether are correct.


  welcome any suggestion or completions.

  thanks.

  :-)


gchen.


On 2013年04月10日 17:52, Chen Gang wrote:
> 
>   in the 'fcount' looping,
>     if 'new->fields[*].type" has 2 or more AUDIT_FILTERKEYs
>     need judge new->filterkey whether has value, or memory leak.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/auditfilter.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index f9fc54b..936ac79 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -859,6 +859,8 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
>  						       &old->fields[i]);
>  			break;
>  		case AUDIT_FILTERKEY:
> +			if (new->filterkey)
> +				break;
>  			fk = kstrdup(old->filterkey, GFP_KERNEL);
>  			if (unlikely(!fk))
>  				err = -ENOMEM;
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

  also for function audit_list:
    when call audit_make_reply fails (will return NULL).
    we need free all its related variables instead of only kfree rull.
      (such as call autit_free_rule)

  please help check, thanks.

  :-)

gchen.

On 2013年04月10日 18:18, Chen Gang wrote:
> 
> 
> in another function: audit_data_to_entry:
> 
>   a. has the same issue for case AUDIT_WATCH.
> 
>   b. has an new issue for AUDIT_DIR:
>        after AUDIT_DIR succeed, it will set rule->tree.
>        next, the other case fail, then will call audit_free_rule.
>        but audit_free_rule will not free rule->tree.
> 
> 
>   I find them only by reading code, not test them.
>   and I also do not know about the related features.
>   so please help check my 2 opinions whether are correct.
> 
> 
>   welcome any suggestion or completions.
> 
>   thanks.
> 
>   :-)
> 
> 
> gchen.
> 
> 
> On 2013年04月10日 17:52, Chen Gang wrote:
>>
>>   in the 'fcount' looping,
>>     if 'new->fields[*].type" has 2 or more AUDIT_FILTERKEYs
>>     need judge new->filterkey whether has value, or memory leak.
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>>  kernel/auditfilter.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>> index f9fc54b..936ac79 100644
>> --- a/kernel/auditfilter.c
>> +++ b/kernel/auditfilter.c
>> @@ -859,6 +859,8 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
>>  						       &old->fields[i]);
>>  			break;
>>  		case AUDIT_FILTERKEY:
>> +			if (new->filterkey)
>> +				break;
>>  			fk = kstrdup(old->filterkey, GFP_KERNEL);
>>  			if (unlikely(!fk))
>>  				err = -ENOMEM;
>>
> 
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

  also for function audit_list_rules:
    when call audit_make_reply fails (will return NULL).
    we also need process data->buf, not only data itself.

  please help check, thanks.

  :-)

gchen.

On 2013年04月10日 18:28, Chen Gang wrote:
> 
>   also for function audit_list:
>     when call audit_make_reply fails (will return NULL).
>     we need free all its related variables instead of only kfree rull.
>       (such as call autit_free_rule)
> 
>   please help check, thanks.
> 
>   :-)
> 
> gchen.
> 
> On 2013年04月10日 18:18, Chen Gang wrote:
>>
>>
>> in another function: audit_data_to_entry:
>>
>>   a. has the same issue for case AUDIT_WATCH.
>>
>>   b. has an new issue for AUDIT_DIR:
>>        after AUDIT_DIR succeed, it will set rule->tree.
>>        next, the other case fail, then will call audit_free_rule.
>>        but audit_free_rule will not free rule->tree.
>>
>>
>>   I find them only by reading code, not test them.
>>   and I also do not know about the related features.
>>   so please help check my 2 opinions whether are correct.
>>
>>
>>   welcome any suggestion or completions.
>>
>>   thanks.
>>
>>   :-)
>>
>>
>> gchen.
>>
>>
>> On 2013年04月10日 17:52, Chen Gang wrote:
>>>
>>>   in the 'fcount' looping,
>>>     if 'new->fields[*].type" has 2 or more AUDIT_FILTERKEYs
>>>     need judge new->filterkey whether has value, or memory leak.
>>>
>>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>>> ---
>>>  kernel/auditfilter.c |    2 ++
>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>>> index f9fc54b..936ac79 100644
>>> --- a/kernel/auditfilter.c
>>> +++ b/kernel/auditfilter.c
>>> @@ -859,6 +859,8 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
>>>  						       &old->fields[i]);
>>>  			break;
>>>  		case AUDIT_FILTERKEY:
>>> +			if (new->filterkey)
>>> +				break;
>>>  			fk = kstrdup(old->filterkey, GFP_KERNEL);
>>>  			if (unlikely(!fk))
>>>  				err = -ENOMEM;
>>>
>>
>>
> 
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Eric Paris]

----- Original Message -----
> 
>   also for function audit_list_rules:
>     when call audit_make_reply fails (will return NULL).
>     we also need process data->buf, not only data itself.
> 
>   please help check, thanks.

struct audit_rule_data {
[...]
        char            buf[0]; /* string fields buffer */
};

The last element in the struct is 0 length.  But the allocation in audit_krule_to_data() looks like:

data = kmalloc(sizeof(*data) + krule->buflen, GFP_KERNEL);

So now data->buf appears as an allocation of size krule->buflen.

We do not need to free it separately.  This is a pretty common C trick.

-Eric

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

On 2013年04月11日 05:38, Eric Paris wrote:
> ----- Original Message -----
>> > 
>> >   also for function audit_list_rules:
>> >     when call audit_make_reply fails (will return NULL).
>> >     we also need process data->buf, not only data itself.
>> > 
>> >   please help check, thanks.
> struct audit_rule_data {
> [...]
>         char            buf[0]; /* string fields buffer */
> };
> 
> The last element in the struct is 0 length.  But the allocation in audit_krule_to_data() looks like:
> 
> data = kmalloc(sizeof(*data) + krule->buflen, GFP_KERNEL);
> 
> So now data->buf appears as an allocation of size krule->buflen.
> 
> We do not need to free it separately.  This is a pretty common C trick.

  ok, thanks

  it is my fault.

  :-)

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Eric Paris]

----- Original Message -----
> 
>   also for function audit_list:
>     when call audit_make_reply fails (will return NULL).
>     we need free all its related variables instead of only kfree rull.
>       (such as call autit_free_rule)
> 
>   please help check, thanks.

audit_free_rule() takes a struct audit_entry, not an audit_rule.  struct audit_rule does not have additional things which need to be freed...

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

On 2013年04月11日 05:32, Eric Paris wrote:
> ----- Original Message -----
>> > 
>> >   also for function audit_list:
>> >     when call audit_make_reply fails (will return NULL).
>> >     we need free all its related variables instead of only kfree rull.
>> >       (such as call autit_free_rule)
>> > 
>> >   please help check, thanks.
> audit_free_rule() takes a struct audit_entry, not an audit_rule. 

  yes. but maybe you misunderstand what I said (excuse me, my English is
not quite weill)
  I said: "need we process the rule just like audit_free_rule has done ?"


> struct audit_rule does not have additional things which need to be freed...
> 
> 

  oh, it is my fault:
    (I did not notice: rule is struct audit_rule, not struct audit_krule).

  thanks.

  :-)

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Eric Paris]

----- Original Message -----
> 
> 
> in another function: audit_data_to_entry:
> 
>   a. has the same issue for case AUDIT_WATCH.

You are saying if there were 2 of them it will leak the old one?  No.  If you have 2 AUDIT_WATCH entries the first one will set entry->rule->watch and the second will bomb with EINVAL in audit_to_watch()

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

On 2013年04月11日 04:29, Eric Paris wrote:
> ----- Original Message -----
>> > 
>> > 
>> > in another function: audit_data_to_entry:
>> > 
>> >   a. has the same issue for case AUDIT_WATCH.
> You are saying if there were 2 of them it will leak the old one?  No.  If you have 2 AUDIT_WATCH entries the first one will set entry->rule->watch and the second will bomb with EINVAL in audit_to_watch()
> 
> 

  thanks, really it is. it is my fault.

  :-)


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Eric Paris]

----- Original Message -----

>   b. has an new issue for AUDIT_DIR:
>        after AUDIT_DIR succeed, it will set rule->tree.
>        next, the other case fail, then will call audit_free_rule.
>        but audit_free_rule will not free rule->tree.

Definitely a couple of leaks here...

I'm seeing leaks on size 8, 64, and 128.

Al, what do you think?  Should I be calling audit_put_tree() in the error case if entry->tree != NULL?  The audit trees are some of the most complex code in the kernel I think.

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

On 2013年04月11日 05:19, Eric Paris wrote:
> ----- Original Message -----
> 
>> >   b. has an new issue for AUDIT_DIR:
>> >        after AUDIT_DIR succeed, it will set rule->tree.
>> >        next, the other case fail, then will call audit_free_rule.
>> >        but audit_free_rule will not free rule->tree.
> Definitely a couple of leaks here...
> 
> I'm seeing leaks on size 8, 64, and 128.
> 
> Al, what do you think?  Should I be calling audit_put_tree() in the error case if entry->tree != NULL?  The audit trees are some of the most complex code in the kernel I think.
> 
> 

  can we add it in audit_free_rule ?

  maybe like this:

@@ -75,6 +75,8 @@ static inline void audit_free_rule(struct audit_entry *e)
 	/* some rules don't have associated watches */
 	if (erule->watch)
 		audit_put_watch(erule->watch);
+	if (erule->tree)
+		audit_put_tree(erule->tree);
 	if (erule->fields)
 		for (i = 0; i < erule->field_count; i++) {
 			struct audit_field *f = &erule->fields[i];


  thanks.

  :-)

-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Eric Paris]

----- Original Message -----
> On 2013年04月11日 05:19, Eric Paris wrote:
> > ----- Original Message -----
> > 
> >> >   b. has an new issue for AUDIT_DIR:
> >> >        after AUDIT_DIR succeed, it will set rule->tree.
> >> >        next, the other case fail, then will call audit_free_rule.
> >> >        but audit_free_rule will not free rule->tree.
> > Definitely a couple of leaks here...
> > 
> > I'm seeing leaks on size 8, 64, and 128.
> > 
> > Al, what do you think?  Should I be calling audit_put_tree() in the error
> > case if entry->tree != NULL?  The audit trees are some of the most complex
> > code in the kernel I think.
> > 
> > 
> 
>   can we add it in audit_free_rule ?
> 
>   maybe like this:
> 
> @@ -75,6 +75,8 @@ static inline void audit_free_rule(struct audit_entry *e)
>  	/* some rules don't have associated watches */
>  	if (erule->watch)
>  		audit_put_watch(erule->watch);
> +	if (erule->tree)
> +		audit_put_tree(erule->tree);
>  	if (erule->fields)
>  		for (i = 0; i < erule->field_count; i++) {
>  			struct audit_field *f = &erule->fields[i];

Where does the tree information get freed normally?  That's the code you need to run down.  You don't want to start getting double frees on the non-error case.  I'll try to dig into it if Al doesn't.  It's easy to show the leak on current kernels.

while(1)
    auditctl -a exit,always -w /etc -F auid=-1

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

On 2013年04月11日 21:40, Eric Paris wrote:
>> >   can we add it in audit_free_rule ?
>> > 
>> >   maybe like this:
>> > 
>> > @@ -75,6 +75,8 @@ static inline void audit_free_rule(struct audit_entry *e)
>> >  	/* some rules don't have associated watches */
>> >  	if (erule->watch)
>> >  		audit_put_watch(erule->watch);
>> > +	if (erule->tree)
>> > +		audit_put_tree(erule->tree);
>> >  	if (erule->fields)
>> >  		for (i = 0; i < erule->field_count; i++) {
>> >  			struct audit_field *f = &erule->fields[i];
> Where does the tree information get freed normally?  That's the code you need to run down.  You don't want to start getting double frees on the non-error case.  I'll try to dig into it if Al doesn't.  It's easy to show the leak on current kernels.
> 

  I think:
    it is in function audit_del_rule. when del, also set NULL.
    so the deletion in audit_free_rule is safe.
    the process of erule->watch and erule->tree are similar.

  please check, thanks.


> while(1)
>     auditctl -a exit,always -w /etc -F auid=-1
> 
> 
> 

  it is valuable to me, thanks.



-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

On 2013年04月11日 22:34, Chen Gang wrote:
> On 2013年04月11日 21:40, Eric Paris wrote:
>>>> >> >   can we add it in audit_free_rule ?
>>>> >> > 
>>>> >> >   maybe like this:
>>>> >> > 
>>>> >> > @@ -75,6 +75,8 @@ static inline void audit_free_rule(struct audit_entry *e)
>>>> >> >  	/* some rules don't have associated watches */
>>>> >> >  	if (erule->watch)
>>>> >> >  		audit_put_watch(erule->watch);
>>>> >> > +	if (erule->tree)
>>>> >> > +		audit_put_tree(erule->tree);
>>>> >> >  	if (erule->fields)
>>>> >> >  		for (i = 0; i < erule->field_count; i++) {
>>>> >> >  			struct audit_field *f = &erule->fields[i];
>> > Where does the tree information get freed normally?  That's the code you need to run down.  You don't want to start getting double frees on the non-error case.  I'll try to dig into it if Al doesn't.  It's easy to show the leak on current kernels.
>> > 

  oh.. it seems another issues need consider !!

    a. in function audit_remove_watch_rule, it does not set NULL for krule->watch.

    b. function audit_del_rule and audit_remove_watch_rule need lock protected.
         it seems we should call audit_del_rule in audit_free_rule.
           audit_del_rule will instead of audit_put_watch and audit_put_tree.
         but we need consider whether will cause dead lock !

       I will continue to see it.


>   I think:
>     it is in function audit_del_rule. when del, also set NULL.
>     so the deletion in audit_free_rule is safe.
>     the process of erule->watch and erule->tree are similar.
> 
>   please check, thanks.
> 
> 
>> > while(1)
>> >     auditctl -a exit,always -w /etc -F auid=-1
>> > 
>> > 
>> > 
>   it is valuable to me, thanks.
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

On 2013年04月11日 12:10, Chen Gang wrote:
> On 2013年04月11日 05:19, Eric Paris wrote:
>> ----- Original Message -----
>>
>>>>   b. has an new issue for AUDIT_DIR:
>>>>        after AUDIT_DIR succeed, it will set rule->tree.
>>>>        next, the other case fail, then will call audit_free_rule.
>>>>        but audit_free_rule will not free rule->tree.
>> Definitely a couple of leaks here...
>>
>> I'm seeing leaks on size 8, 64, and 128.
>>
>> Al, what do you think?  Should I be calling audit_put_tree() in the error case if entry->tree != NULL?  The audit trees are some of the most complex code in the kernel I think.
>>
>>

  it seems, your way is the only executable way (if not change code much).
  what my original idea is incorrect.

    we need add related code at failure process area in audit_data_to_entry.
    and another functions need not add these code (should not add).
    'watch' also need be processed, since audit_to_watch let ref count = 2.
      (it just like the function audit_del_rule has done)

  please help check thanks.

  :-)


diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 81f63f9..f5327ce 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -594,6 +594,10 @@ exit_nofree:
 	return entry;
 
 exit_free:
+	if (entry->rule.watch)
+		audit_put_watch(entry->rule.watch); /* matches initial get */
+	if (entry->rule.tree)
+		audit_put_tree(entry->rule.tree); /* that's the temporary one */
 	audit_free_rule(entry);
 	return ERR_PTR(err);
 }



> 
>   can we add it in audit_free_rule ?
> 
>   maybe like this:
> 
> @@ -75,6 +75,8 @@ static inline void audit_free_rule(struct audit_entry *e)
>  	/* some rules don't have associated watches */
>  	if (erule->watch)
>  		audit_put_watch(erule->watch);
> +	if (erule->tree)
> +		audit_put_tree(erule->tree);
>  	if (erule->fields)
>  		for (i = 0; i < erule->field_count; i++) {
>  			struct audit_field *f = &erule->fields[i];
> 
> 
>   thanks.
> 
>   :-)
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

On 2013年04月12日 17:42, Chen Gang wrote:
> On 2013年04月11日 12:10, Chen Gang wrote:
>> On 2013年04月11日 05:19, Eric Paris wrote:
>>> ----- Original Message -----
>>>
>>>>>   b. has an new issue for AUDIT_DIR:
>>>>>        after AUDIT_DIR succeed, it will set rule->tree.
>>>>>        next, the other case fail, then will call audit_free_rule.
>>>>>        but audit_free_rule will not free rule->tree.
>>> Definitely a couple of leaks here...
>>>
>>> I'm seeing leaks on size 8, 64, and 128.
>>>
>>> Al, what do you think?  Should I be calling audit_put_tree() in the error case if entry->tree != NULL?  The audit trees are some of the most complex code in the kernel I think.
>>>
>>>

  I am just testing about it with:

---
while(1)
    auditctl -a exit,always -w /etc -F auid=-1
---

  under fedora 17, we need modify the auditctl source code:
    a. let -w /etc can pass auditctl checking.
    b. let loop infinitely in a process (if process quit, will free mem)
    c. need fix a bug for auditctl (under Fedora 17)
         audit_open may open 2 times.
         when loop infinitely, it will cause resource handle leak.

  I have checked (by insert printf in kernel/auditfilter.c):
    after modify the auditct, the work flow is just what we want to be.
      (will alloc watch, alloc tree, then failure occurs)


  I guess, we need 2-3 days to get a test result.


  welcome any suggestions and completions.

  thanks.



> 
>   it seems, your way is the only executable way (if not change code much).
>   what my original idea is incorrect.
> 
>     we need add related code at failure process area in audit_data_to_entry.
>     and another functions need not add these code (should not add).
>     'watch' also need be processed, since audit_to_watch let ref count = 2.
>       (it just like the function audit_del_rule has done)
> 
>   please help check thanks.
> 
>   :-)
> 
> 
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 81f63f9..f5327ce 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -594,6 +594,10 @@ exit_nofree:
>  	return entry;
>  
>  exit_free:
> +	if (entry->rule.watch)
> +		audit_put_watch(entry->rule.watch); /* matches initial get */
> +	if (entry->rule.tree)
> +		audit_put_tree(entry->rule.tree); /* that's the temporary one */
>  	audit_free_rule(entry);
>  	return ERR_PTR(err);
>  }
> 
> 
> 
>>
>>   can we add it in audit_free_rule ?
>>
>>   maybe like this:
>>
>> @@ -75,6 +75,8 @@ static inline void audit_free_rule(struct audit_entry *e)
>>  	/* some rules don't have associated watches */
>>  	if (erule->watch)
>>  		audit_put_watch(erule->watch);
>> +	if (erule->tree)
>> +		audit_put_tree(erule->tree);
>>  	if (erule->fields)
>>  		for (i = 0; i < erule->field_count; i++) {
>>  			struct audit_field *f = &erule->fields[i];
>>
>>
>>   thanks.
>>
>>   :-)
>>
> 
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

On 2013年04月16日 18:25, Chen Gang wrote:
> On 2013年04月12日 17:42, Chen Gang wrote:
>> On 2013年04月11日 12:10, Chen Gang wrote:
>>> On 2013年04月11日 05:19, Eric Paris wrote:
>>>> ----- Original Message -----
>>>>
>>>>>>   b. has an new issue for AUDIT_DIR:
>>>>>>        after AUDIT_DIR succeed, it will set rule->tree.
>>>>>>        next, the other case fail, then will call audit_free_rule.
>>>>>>        but audit_free_rule will not free rule->tree.
>>>> Definitely a couple of leaks here...
>>>>
>>>> I'm seeing leaks on size 8, 64, and 128.
>>>>
>>>> Al, what do you think?  Should I be calling audit_put_tree() in the error case if entry->tree != NULL?  The audit trees are some of the most complex code in the kernel I think.
>>>>
>>>>

  oh, also need buffering optarg of auditctl under fedora 17.
  or "-F auid=-1" will be truncated to "-F auid".
  it is ok if not looping again. but in our case, we need loop again.

  to see memory usage, I think:
    in top, really used memory = 'used' - 'cached'
    it is enough for us.

  welcome any suggestions or completions.

  thanks.


> 
>   I am just testing about it with:
> 
> ---
> while(1)
>     auditctl -a exit,always -w /etc -F auid=-1
> ---
> 
>   under fedora 17, we need modify the auditctl source code:
>     a. let -w /etc can pass auditctl checking.
>     b. let loop infinitely in a process (if process quit, will free mem)
>     c. need fix a bug for auditctl (under Fedora 17)
>          audit_open may open 2 times.
>          when loop infinitely, it will cause resource handle leak.
> 
>   I have checked (by insert printf in kernel/auditfilter.c):
>     after modify the auditct, the work flow is just what we want to be.
>       (will alloc watch, alloc tree, then failure occurs)
> 
> 
>   I guess, we need 2-3 days to get a test result.
> 
> 
>   welcome any suggestions and completions.
> 
>   thanks.
> 
> 
> 
>>
>>   it seems, your way is the only executable way (if not change code much).
>>   what my original idea is incorrect.
>>
>>     we need add related code at failure process area in audit_data_to_entry.
>>     and another functions need not add these code (should not add).
>>     'watch' also need be processed, since audit_to_watch let ref count = 2.
>>       (it just like the function audit_del_rule has done)
>>
>>   please help check thanks.
>>
>>   :-)
>>
>>
>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>> index 81f63f9..f5327ce 100644
>> --- a/kernel/auditfilter.c
>> +++ b/kernel/auditfilter.c
>> @@ -594,6 +594,10 @@ exit_nofree:
>>  	return entry;
>>  
>>  exit_free:
>> +	if (entry->rule.watch)
>> +		audit_put_watch(entry->rule.watch); /* matches initial get */
>> +	if (entry->rule.tree)
>> +		audit_put_tree(entry->rule.tree); /* that's the temporary one */
>>  	audit_free_rule(entry);
>>  	return ERR_PTR(err);
>>  }
>>
>>
>>
>>>
>>>   can we add it in audit_free_rule ?
>>>
>>>   maybe like this:
>>>
>>> @@ -75,6 +75,8 @@ static inline void audit_free_rule(struct audit_entry *e)
>>>  	/* some rules don't have associated watches */
>>>  	if (erule->watch)
>>>  		audit_put_watch(erule->watch);
>>> +	if (erule->tree)
>>> +		audit_put_tree(erule->tree);
>>>  	if (erule->fields)
>>>  		for (i = 0; i < erule->field_count; i++) {
>>>  			struct audit_field *f = &erule->fields[i];
>>>
>>>
>>>   thanks.
>>>
>>>   :-)
>>>
>>
>>
> 
> 


-- 
Chen Gang

Asianux Corporation

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

On 2013年04月16日 18:38, Chen Gang wrote:
> On 2013年04月16日 18:25, Chen Gang wrote:
>> On 2013年04月12日 17:42, Chen Gang wrote:
>>> On 2013年04月11日 12:10, Chen Gang wrote:
>>>> On 2013年04月11日 05:19, Eric Paris wrote:
>>>>> ----- Original Message -----
>>>>>
>>>>>>>   b. has an new issue for AUDIT_DIR:
>>>>>>>        after AUDIT_DIR succeed, it will set rule->tree.
>>>>>>>        next, the other case fail, then will call audit_free_rule.
>>>>>>>        but audit_free_rule will not free rule->tree.
>>>>> Definitely a couple of leaks here...
>>>>>
>>>>> I'm seeing leaks on size 8, 64, and 128.
>>>>>
>>>>> Al, what do you think?  Should I be calling audit_put_tree() in the error case if entry->tree != NULL?  The audit trees are some of the most complex code in the kernel I think.
>>>>>
>>>>>

  after the test, the original version really has memory leak.

  test:
    the related monitor command is:
      watch -d -n 1 "cat /proc/meminfo | awk '{print \$2}' \
        | head -n 4 | xargs \
        | awk '{print \"used \",\$1 - \$2 - \$3 - \$4}'"
    I run 15 processes of modified auditctl at the same time.

  result:
    for original version:
      can see the memory leak, it will be more clear after 1 - 2 hours.

    for new version (fix it):
      can not see the memory leak after ran 12 - 14 hours.


  I will use LTP (ltp-full-20130109) to test audit again under fedora 17
x86_64 for next-20130415, then send related patch.


  welcome any suggestions or completions.


> 
>   oh, also need buffering optarg of auditctl under fedora 17.
>   or "-F auid=-1" will be truncated to "-F auid".
>   it is ok if not looping again. but in our case, we need loop again.
> 
>   to see memory usage, I think:
>     in top, really used memory = 'used' - 'cached'
>     it is enough for us.
> 
>   welcome any suggestions or completions.
> 
>   thanks.
> 
> 
>>
>>   I am just testing about it with:
>>
>> ---
>> while(1)
>>     auditctl -a exit,always -w /etc -F auid=-1
>> ---
>>
>>   under fedora 17, we need modify the auditctl source code:
>>     a. let -w /etc can pass auditctl checking.
>>     b. let loop infinitely in a process (if process quit, will free mem)
>>     c. need fix a bug for auditctl (under Fedora 17)
>>          audit_open may open 2 times.
>>          when loop infinitely, it will cause resource handle leak.
>>
>>   I have checked (by insert printf in kernel/auditfilter.c):
>>     after modify the auditct, the work flow is just what we want to be.
>>       (will alloc watch, alloc tree, then failure occurs)
>>
>>
>>   I guess, we need 2-3 days to get a test result.
>>
>>
>>   welcome any suggestions and completions.
>>
>>   thanks.
>>
>>
>>
>>>
>>>   it seems, your way is the only executable way (if not change code much).
>>>   what my original idea is incorrect.
>>>
>>>     we need add related code at failure process area in audit_data_to_entry.
>>>     and another functions need not add these code (should not add).
>>>     'watch' also need be processed, since audit_to_watch let ref count = 2.
>>>       (it just like the function audit_del_rule has done)
>>>
>>>   please help check thanks.
>>>
>>>   :-)
>>>
>>>
>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>>> index 81f63f9..f5327ce 100644
>>> --- a/kernel/auditfilter.c
>>> +++ b/kernel/auditfilter.c
>>> @@ -594,6 +594,10 @@ exit_nofree:
>>>  	return entry;
>>>  
>>>  exit_free:
>>> +	if (entry->rule.watch)
>>> +		audit_put_watch(entry->rule.watch); /* matches initial get */
>>> +	if (entry->rule.tree)
>>> +		audit_put_tree(entry->rule.tree); /* that's the temporary one */
>>>  	audit_free_rule(entry);
>>>  	return ERR_PTR(err);
>>>  }
>>>
>>>
>>>
>>>>
>>>>   can we add it in audit_free_rule ?
>>>>
>>>>   maybe like this:
>>>>
>>>> @@ -75,6 +75,8 @@ static inline void audit_free_rule(struct audit_entry *e)
>>>>  	/* some rules don't have associated watches */
>>>>  	if (erule->watch)
>>>>  		audit_put_watch(erule->watch);
>>>> +	if (erule->tree)
>>>> +		audit_put_tree(erule->tree);
>>>>  	if (erule->fields)
>>>>  		for (i = 0; i < erule->field_count; i++) {
>>>>  			struct audit_field *f = &erule->fields[i];
>>>>
>>>>
>>>>   thanks.
>>>>
>>>>   :-)
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
Chen Gang

Asianux Corporation

[PATCH v2] kernel: auditfilter: resource management, tree and watch will memory leak when failure occurs

[Chen Gang]

  in function audit_data_to_entry:
    when failure occurs, need check and free tree and watch.
    or memory leak.

  test:
    plan:
      test command:
        "auditctl -a exit,always -w /etc -F auid=-1"
        (on fedora17, need modify auditctl to let "-w /etc" has effect)
      running:
        under fedora17 x86_64, 2 CPUs 3.20GHz, 2.5GB RAM.
        let 15 auditctl processes continue running at the same time.
      monitor command: 
        watch -d -n 1 "cat /proc/meminfo | awk '{print \$2}' \
          | head -n 4 | xargs \
          | awk '{print \"used \",\$1 - \$2 - \$3 - \$4}'"

    result:
      for original version:
        will use up all memory, within 3 hours.
        kill all auditctl, the memory still does not free.
      for new version (apply this patch):
        after 14 hours later, not find issues.


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 kernel/auditfilter.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index f9fc54b..2674368 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -594,6 +594,10 @@ exit_nofree:
 	return entry;
 
 exit_free:
+	if (entry->rule.watch)
+		audit_put_watch(entry->rule.watch); /* matches initial get */
+	if (entry->rule.tree)
+		audit_put_tree(entry->rule.tree); /* that's the temporary one */
 	audit_free_rule(entry);
 	return ERR_PTR(err);
 }
-- 
1.7.7.6

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Eric Paris]

We only allow one filter key per rule.  So we should never be able to get into this situation.  See audit_data_to_entry()

-Eric

----- Original Message -----
> 
>   in the 'fcount' looping,
>     if 'new->fields[*].type" has 2 or more AUDIT_FILTERKEYs
>     need judge new->filterkey whether has value, or memory leak.
> 
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>  kernel/auditfilter.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index f9fc54b..936ac79 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -859,6 +859,8 @@ struct audit_entry *audit_dupe_rule(struct audit_krule
> *old)
>  						       &old->fields[i]);
>  			break;
>  		case AUDIT_FILTERKEY:
> +			if (new->filterkey)
> +				break;
>  			fk = kstrdup(old->filterkey, GFP_KERNEL);
>  			if (unlikely(!fk))
>  				err = -ENOMEM;
> --
> 1.7.7.6
> 

Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs

[Chen Gang]

On 2013年04月11日 04:08, Eric Paris wrote:
> We only allow one filter key per rule.  So we should never be able to get into this situation.  See audit_data_to_entry()

  really it is, thanks.

  :-)

-- 
Chen Gang

Asianux Corporation