Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 = >fields[i];
thanks.
:-)
>>>
>>>
>>
>>
>
>
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 = >fields[i];
>>>
>>>
>>> thanks.
>>>
>>> :-)
>>>
>>
>>
>
>
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 = >fields[i];
>>
>>
>> thanks.
>>
>> :-)
>>
>
>
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 = >fields[i];
>
>
> thanks.
>
> :-)
>
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 = >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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 = >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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
- 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 = >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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
- 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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 = >fields[i];
thanks.
:-)
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 {
> [...]
> charbuf[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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
- 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 {
[...]
charbuf[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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
- 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... -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
- 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. -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
- 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() -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 > --- > 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) > >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 > -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 >>> --- >>> 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) >>>>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 -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 >> --- >> 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) >> >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 -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 > --- > 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) > >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 -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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.c...@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 -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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.c...@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 -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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.c...@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 -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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.c...@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 -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
- 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() -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
- 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. -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
- 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... -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
- 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 {
[...]
charbuf[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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 {
[...]
charbuf[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
--
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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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 -- 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/
Re: [PATCH] kernel: auditfilter: looping issue, memory leak if has 2 or more AUDIT_FILTERKEYs
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
--
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/
