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