FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Ubuntu > Ubuntu Kernel Team

 
 
LinkBack Thread Tools
 
Old 10-29-2009, 01:59 PM
John Johansen
 
Default SRU: AppArmor fix file perm reporting and dfa allocation

The following changes since commit 7423c4c3b22816168b912c39a0298227076854b8:
Scott James Remnant (1):
UBUNTU: SAUCE: trace: add trace events for open(), exec() and uselib()

are available in the git repository at:

kernel.ubuntu.com:/srv/kernel.ubuntu.com/git/jj/apparmor-karmic.git master

John Johansen (2):
UBUNTU: SAUCE: AppArmor: AppArmor wrongly reports allow perms as denied
UBUNTU: SAUCE: AppArmor: Policy load and replacement can fail to alloc mem

ubuntu/apparmor/file.c | 4 ++--
ubuntu/apparmor/match.c | 5 +++--
2 files changed, 5 insertions(+), 4 deletions(-)

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 10-29-2009, 02:26 PM
Stefan Bader
 
Default SRU: AppArmor fix file perm reporting and dfa allocation

John Johansen wrote:
> The following changes since commit 7423c4c3b22816168b912c39a0298227076854b8:
> Scott James Remnant (1):
> UBUNTU: SAUCE: trace: add trace events for open(), exec() and uselib()
>
> are available in the git repository at:
>
> kernel.ubuntu.com:/srv/kernel.ubuntu.com/git/jj/apparmor-karmic.git master
>
> John Johansen (2):
> UBUNTU: SAUCE: AppArmor: AppArmor wrongly reports allow perms as denied
> UBUNTU: SAUCE: AppArmor: Policy load and replacement can fail to alloc mem
>
> ubuntu/apparmor/file.c | 4 ++--
> ubuntu/apparmor/match.c | 5 +++--
> 2 files changed, 5 insertions(+), 4 deletions(-)
>

I had a look at both of those and they look safe to me. ACK

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 10-29-2009, 05:06 PM
Andy Whitcroft
 
Default SRU: AppArmor fix file perm reporting and dfa allocation

On Thu, Oct 29, 2009 at 07:59:43AM -0700, John Johansen wrote:
> The following changes since commit 7423c4c3b22816168b912c39a0298227076854b8:
> Scott James Remnant (1):
> UBUNTU: SAUCE: trace: add trace events for open(), exec() and uselib()
>
> are available in the git repository at:
>
> kernel.ubuntu.com:/srv/kernel.ubuntu.com/git/jj/apparmor-karmic.git master
>
> John Johansen (2):
> UBUNTU: SAUCE: AppArmor: AppArmor wrongly reports allow perms as denied
> UBUNTU: SAUCE: AppArmor: Policy load and replacement can fail to alloc mem

This second one is worrying me a little. To quote the description:

AppArmor dfas can be rather large under some circumstances with some
requiring allocations of 128K+ per table (with at least 4 tables per dfa,
and a dfa per profile).

This can result in memory allocation asking for page allocations with an
order of 5 or more, which will fail if memory has become fragemented.

If I am reading the above correctly we are saying we have 4*128K per
profile = 512K per profile. And we have more than 5 profiles already?
Does this mean that we need multiple megabytes of vmalloc space?
Bear in mind that vmalloc space on 32bit machines with 1GB of ram is
very small indeed:

[ 0.000000] vmalloc : 0xf7ffe000 - 0xff7fe000 ( 120 MB)

I think it might be safer to use kmalloc if we can and if that fails use
vmalloc. There is a is_vmalloc_addr() helper to help with the free
case.

Reviewing the change itself with smb we think that the failure cases in
unpack_dfa() should also be using the matching vfree() style if you are
making this conversion?

int unpack_dfa(struct aa_dfa *dfa, void *blob, size_t size)
[...]
default:
kfree(table);
goto fail;
[...]
for (i = 0; i < ARRAY_SIZE(dfa->tables); i++) {
kfree(dfa->tables[i]);
dfa->tables[i] = NULL;
}
[...]

-apw

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 10-29-2009, 05:23 PM
John Johansen
 
Default SRU: AppArmor fix file perm reporting and dfa allocation

Andy Whitcroft wrote:
> On Thu, Oct 29, 2009 at 07:59:43AM -0700, John Johansen wrote:
>> The following changes since commit 7423c4c3b22816168b912c39a0298227076854b8:
>> Scott James Remnant (1):
>> UBUNTU: SAUCE: trace: add trace events for open(), exec() and uselib()
>>
>> are available in the git repository at:
>>
>> kernel.ubuntu.com:/srv/kernel.ubuntu.com/git/jj/apparmor-karmic.git master
>>
>> John Johansen (2):
>> UBUNTU: SAUCE: AppArmor: AppArmor wrongly reports allow perms as denied
>> UBUNTU: SAUCE: AppArmor: Policy load and replacement can fail to alloc mem
>
> This second one is worrying me a little. To quote the description:
>
> AppArmor dfas can be rather large under some circumstances with some
> requiring allocations of 128K+ per table (with at least 4 tables per dfa,
> and a dfa per profile).
>
> This can result in memory allocation asking for page allocations with an
> order of 5 or more, which will fail if memory has become fragemented.
>
> If I am reading the above correctly we are saying we have 4*128K per
> profile = 512K per profile. And we have more than 5 profiles already?
> Does this mean that we need multiple megabytes of vmalloc space?
> Bear in mind that vmalloc space on 32bit machines with 1GB of ram is
> very small indeed:
>
> [ 0.000000] vmalloc : 0xf7ffe000 - 0xff7fe000 ( 120 MB)
>
I should rewrite that. It is correct that tables can be very large in certain
situations, and if 1 table is large all tables in the profile will be relative
large as well. The next and check tables are the largest, theoretically up to
256 * the other 3 tables in the dfa (all the same size) without any packing.

The user space policy compiler does do packing and heuristic based expression
elimination, so that in general the next/check tables are no where near the
theoretical max (they tend to be in the 3-20x range). There is also on going
work to improve the dfa with a state minimization pass and a new table packing
scheme so those both will help.

This does not mean all dfas are the same size, in fact most of them run under
20k in size. It was more of a worst case scenario statement.

> I think it might be safer to use kmalloc if we can and if that fails use
> vmalloc. There is a is_vmalloc_addr() helper to help with the free
> case.
>
agreed and this is what I did for the upstreamed version, I was just trying to
keep the patch for the SRU minimal.

> Reviewing the change itself with smb we think that the failure cases in
> unpack_dfa() should also be using the matching vfree() style if you are
> making this conversion?
>
hrmm sorry must have forgotten to refresh the patch
> int unpack_dfa(struct aa_dfa *dfa, void *blob, size_t size)
> [...]
> default:
> kfree(table);
> goto fail;
> [...]
> for (i = 0; i < ARRAY_SIZE(dfa->tables); i++) {
> kfree(dfa->tables[i]);
> dfa->tables[i] = NULL;
> }
> [...]
>

thanks for the review I will update and repost

john

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 

Thread Tools




All times are GMT. The time now is 11:14 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org