Hi everyone,
This patchset focuses on optimizations for memory handling done in internals of
smk_write_rules_list(). It is an update from [1].
The first patch introduces a limit for maximal length of a rule string. The
second patch optimizes rule string parsing to avoid unnecessary allocations.
The third and the forth introduce kmem_cache to reduce memory wasted on padding
bytes.
The patchset is rebased on smack/next. Additionally the "memleak" patch [2] has
to be applied. This patch conflicts with the this patchset.
Some measurements for time and used memory were prepared. The test platform
was ARM target. The Smack configuration contains circa 17K rules and 500
labels. The procedure is following:
1. Boot the target with 'init=/bin/bash' added to cmdline.
2. Mount all needed file systems (procfs and smackfs in principle)
3. Measure SLAB memory with (column Pre expressed in [kiB]):
# grep SUnreclaim: /proc/meminfo
4. Initialize SMACK and measure time (column Time [sec])
# time smackctl apply
5. Measure SLAB memory with (column Post expressed in [kiB]):
# grep SUnreclaim: /proc/meminfo
Each measurement was repeated 5 time to reduce noise.
The column 'Diff' is equal to 'Post' - 'Pre'.
This value is expected to be equal to kernel memory
allocated during the initialization of SMACK.
Result for the reference kernel from smack/next:
Run:|Pre |Post|Diff|Time
.-------------------------
1 |5132|7116|1984|1.162
2 |5148|7024|1876|1.134
3 |5292|7264|1972|1.148
4 |5436|7424|1988|1.156
5 |5364|7276|1912|1.140
.-------------------------
AVG:| | |1946|1.148
Results for reference kernel plus memfix patch.
Run:|Pre |Post|Diff|Time
.-------------------------
1 |5056|6388|1332|1.149
2 |5072|6444|1372|1.127
3 |4892|6336|1444|1.131
4 |5468|6740|1272|1.149
5 |5192|6520|1328|1.145
.-------------------------
AVG:| | |1349|1.140
Fixing the memleak reduced memeory consumption by 600 KiB.
Results for previous kernel + patch 2. Patch 1 was not tested
because it is only a protection limit.
Run:|Pre |Post|Diff|Time
.-------------------------
1 |5264|6616|1352|1.117
2 |5352|6668|1316|1.115
3 |5400|6752|1352|1.118
4 |5220|6668|1448|1.122
5 |5316|6652|1336|1.101
.-------------------------
AVG:| | |1360|1.115
The patch 2 fastened rule loading by 25 ms.
Results for previous kernel + patch 3.
Run:|Pre |Post|Diff|Time
.-------------------------
1 |5212|6432|1220|1.106
2 |5408|6552|1144|1.118
3 |5044|6292|1248|1.107
4 |5232|6428|1196|1.120
5 |5268|6492|1224|1.111
.-------------------------
AVG:| | |1206|1.112
Memory consumption was reduced by 154 kiB. The patch reduced
memory used for single rule entity from 32 to 24 bytes.
This gives 8 * 17k = 132 KiB. The value is consistent with
measurements due to high noise.
Results for previous kernel + patch 3.
Run:|Pre |Post|Diff|Time
.-------------------------
1 |5300|6176| 876|1.098
2 |5044|5984| 940|1.086
3 |5504|6436| 932|1.074
4 |5244|6200| 956|1.083
5 |5280|6164| 884|1.088
.-------------------------
AVG:| | | 918|1.086
Memory consumption was reduced by 288 kiB. The patch reduced
memory used for single master rule entity from 32 to 16 bytes.
This gives 16 * 17k = 264 KiB. The value is consistent with
measurements due to high noise. Moreover, the initialization
was sped up by 26 ms.
To sum up, all the patches plus the memory fix reduced the amount
of memory for rule-related structures from 1946 kiB to 918 kiB.
Memory requirements were reduced by half.
I hope you find this patchset useful.
All comments are welcome.
Regards,
Tomasz Stanislawski.
Changelog:
v1:
- post 'fix memleak in smk_write_rules_list() as a separate patch'
- prepare performance measurements
- remove stack allocations for a rule string, use kmalloc()
[1] http://en.it-usenet.org/thread/20260/343969/
[2] http://www.mail-archive.com/[email protected]/msg454761.html
Tomasz Stanislawski (4):
security: smack: limit a length for a rule string in the long format
security: smack: avoid kmalloc() in smk_parse_long_rule()
security: smack: add kmem_cache for smack_rule allocations
security: smack: add kmem_cache for smack_master_list allocations
security/smack/smack.h | 10 ++++++
security/smack/smack_lsm.c | 19 +++++++++-
security/smack/smackfs.c | 83 ++++++++++++++++++++++++--------------------
3 files changed, 74 insertions(+), 38 deletions(-)
--
1.7.9.5
The maximal length for a rule line for long format is introduced as
SMK_LOAD2LEN. Limiting the length of a rule line helps to avoid allocations of
a very long contiguous buffer from a heap if user calls write() for a very long
chunk. Such an allocation often causes a lot swapper/writeback havoc and it is
very likely to fail.
Signed-off-by: Tomasz Stanislawski <[email protected]>
---
security/smack/smackfs.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 08aebc2..5dcd520 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -137,6 +137,7 @@ const char *smack_cipso_option = SMACK_CIPSO_OPTION;
* SMK_ACCESS: Maximum possible combination of access permissions
* SMK_ACCESSLEN: Maximum length for a rule access field
* SMK_LOADLEN: Smack rule length
+ * SMK_LOAD2LEN: Smack maximal long rule length excluding \0
*/
#define SMK_OACCESS "rwxa"
#define SMK_ACCESS "rwxat"
@@ -144,6 +145,7 @@ const char *smack_cipso_option = SMACK_CIPSO_OPTION;
#define SMK_ACCESSLEN (sizeof(SMK_ACCESS) - 1)
#define SMK_OLOADLEN (SMK_LABELLEN + SMK_LABELLEN + SMK_OACCESSLEN)
#define SMK_LOADLEN (SMK_LABELLEN + SMK_LABELLEN + SMK_ACCESSLEN)
+#define SMK_LOAD2LEN (2 * SMK_LONGLABEL + SMK_ACCESSLEN + 2)
/*
* Stricly for CIPSO level manipulation.
@@ -459,6 +461,9 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
if (*ppos != 0)
return -EINVAL;
+ if (count > SMK_LOAD2LEN)
+ count = SMK_LOAD2LEN;
+
if (format == SMK_FIXED24_FMT) {
/*
* Minor hack for backward compatibility
--
1.7.9.5
On ARM, sizeof(struct smack_master_list) == 12. Allocation by kmalloc() uses a
32-byte-long chunk to allocate 12 bytes. Just ask ksize(). It means that 63%
of memory is simply wasted for padding bytes.
The problem is fixed in this patch by using kmem_cache. The cache allocates
struct smack_master_list using 16-byte-long chunks according to ksize(). This
reduces amount of used memory by 50%.
Signed-off-by: Tomasz Stanislawski <[email protected]>
---
security/smack/smack.h | 7 +++++++
security/smack/smack_lsm.c | 8 ++++++++
security/smack/smackfs.c | 8 ++------
3 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/security/smack/smack.h b/security/smack/smack.h
index 38ba673..463f818 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -194,6 +194,12 @@ struct smk_audit_info {
struct smack_audit_data sad;
#endif
};
+
+struct smack_master_list {
+ struct list_head list;
+ struct smack_rule *smk_rule;
+};
+
/*
* These functions are in smack_lsm.c
*/
@@ -235,6 +241,7 @@ extern struct list_head smk_netlbladdr_list;
/* Cache for fast and thrifty allocations */
extern struct kmem_cache *smack_rule_cache;
+extern struct kmem_cache *smack_master_list_cache;
extern struct security_operations smack_ops;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 7aa696a..1d4a1b0 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3566,6 +3566,7 @@ static __init void init_smack_known_list(void)
/* KMEM caches for fast and thrifty allocations */
struct kmem_cache *smack_rule_cache;
+struct kmem_cache *smack_master_list_cache;
/**
* smack_init - initialize the smack system
@@ -3584,9 +3585,16 @@ static __init int smack_init(void)
if (!smack_rule_cache)
return -ENOMEM;
+ smack_master_list_cache = KMEM_CACHE(smack_master_list, 0);
+ if (!smack_master_list_cache) {
+ kmem_cache_destroy(smack_rule_cache);
+ return -ENOMEM;
+ }
+
tsp = new_task_smack(smack_known_floor.smk_known,
smack_known_floor.smk_known, GFP_KERNEL);
if (tsp == NULL) {
+ kmem_cache_destroy(smack_master_list_cache);
kmem_cache_destroy(smack_rule_cache);
return -ENOMEM;
}
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 4615242..7ffce7e 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -104,11 +104,6 @@ LIST_HEAD(smk_netlbladdr_list);
* Rule lists are maintained for each label.
* This master list is just for reading /smack/load and /smack/load2.
*/
-struct smack_master_list {
- struct list_head list;
- struct smack_rule *smk_rule;
-};
-
LIST_HEAD(smack_rule_list);
struct smack_parsed_rule {
@@ -233,7 +228,8 @@ static int smk_set_access(struct smack_parsed_rule *srp,
* it needs to get added for reporting.
*/
if (global) {
- smlp = kzalloc(sizeof(*smlp), GFP_KERNEL);
+ smlp = kmem_cache_zalloc(smack_master_list_cache,
+ GFP_KERNEL);
if (smlp != NULL) {
smlp->smk_rule = sp;
list_add_rcu(&smlp->list, &smack_rule_list);
--
1.7.9.5
Function smk_parse_long_rule() allocates a number of temporary strings on heap
(kmalloc cache). Moreover, the sizes of those allocations might be large if
user calls write() for a long chunk. A big kmalloc triggers a heavy reclaim
havoc and it is very likely to fail.
This patch introduces smk_parse_substrings() function that parses a string into
substring separated by whitespaces. The buffer for substring is preallocated.
It must store substring the worst case scenario which is SMK_LOAD2LEN in case
of long rule parsing.
Signed-off-by: Tomasz Stanislawski <[email protected]>
---
security/smack/smackfs.c | 68 ++++++++++++++++++++++++++--------------------
1 file changed, 38 insertions(+), 30 deletions(-)
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 5dcd520..34d146b 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -364,6 +364,33 @@ static int smk_parse_rule(const char *data, struct smack_parsed_rule *rule,
}
/**
+ * smk_parse_strings - parse white-space separated substring from a string
+ * @src: a long string to be parsed, null terminated
+ * @dst: a buffer for substrings, should be at least strlen(src)+1 bytes
+ * @args: table for parsed substring
+ * @size: number of slots in args table
+ *
+ * Returns number of parsed substrings
+ */
+static int smk_parse_substrings(const char *src, char *dst,
+ char *args[], int size)
+{
+ int cnt;
+
+ for (cnt = 0; cnt < size; ++cnt) {
+ src = skip_spaces(src);
+ if (*src == 0)
+ break;
+ args[cnt] = dst;
+ for (; *src && !isspace(*src); ++src, ++dst)
+ *dst = *src;
+ *(dst++) = 0;
+ }
+
+ return cnt;
+}
+
+/**
* smk_parse_long_rule - parse Smack rule from rule string
* @data: string to be parsed, null terminated
* @rule: Will be filled with Smack parsed rule
@@ -375,48 +402,29 @@ static int smk_parse_rule(const char *data, struct smack_parsed_rule *rule,
static int smk_parse_long_rule(const char *data, struct smack_parsed_rule *rule,
int import, int change)
{
- char *subject;
- char *object;
- char *access1;
- char *access2;
int datalen;
+ char *buffer;
+ char *args[4];
int rc = -1;
- /* This is inefficient */
- datalen = strlen(data);
+ datalen = strlen(data) + 1;
- /* Our first element can be 64 + \0 with no spaces */
- subject = kzalloc(datalen + 1, GFP_KERNEL);
- if (subject == NULL)
+ buffer = kmalloc(datalen, GFP_KERNEL);
+ if (!buffer)
return -1;
- object = kzalloc(datalen, GFP_KERNEL);
- if (object == NULL)
- goto free_out_s;
- access1 = kzalloc(datalen, GFP_KERNEL);
- if (access1 == NULL)
- goto free_out_o;
- access2 = kzalloc(datalen, GFP_KERNEL);
- if (access2 == NULL)
- goto free_out_a;
if (change) {
- if (sscanf(data, "%s %s %s %s",
- subject, object, access1, access2) == 4)
- rc = smk_fill_rule(subject, object, access1, access2,
+ if (smk_parse_substrings(data, buffer, args, 4) == 4)
+ rc = smk_fill_rule(args[0], args[1], args[2], args[3],
rule, import, 0);
} else {
- if (sscanf(data, "%s %s %s", subject, object, access1) == 3)
- rc = smk_fill_rule(subject, object, access1, NULL,
+ if (smk_parse_substrings(data, buffer, args, 3) == 3)
+ rc = smk_fill_rule(args[0], args[1], args[2], NULL,
rule, import, 0);
}
- kfree(access2);
-free_out_a:
- kfree(access1);
-free_out_o:
- kfree(object);
-free_out_s:
- kfree(subject);
+ kfree(buffer);
+
return rc;
}
--
1.7.9.5
On ARM, sizeof(struct smack_rule)==20. Allocation by kmalloc() uses a
32-byte-long chunk to allocate 20 bytes. Just ask ksize(). It means that 40%
of memory is simply wasted for padding bytes.
The problem is fixed in this patch by using kmem_cache. The cache allocates
struct smack_rule using 24-byte-long chunks according to ksize(). This reduces
amount of used memory by 25%.
Signed-off-by: Tomasz Stanislawski <[email protected]>
---
security/smack/smack.h | 3 +++
security/smack/smack_lsm.c | 11 ++++++++++-
security/smack/smackfs.c | 2 +-
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/security/smack/smack.h b/security/smack/smack.h
index 8ad3095..38ba673 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -233,6 +233,9 @@ extern struct mutex smack_known_lock;
extern struct list_head smack_known_list;
extern struct list_head smk_netlbladdr_list;
+/* Cache for fast and thrifty allocations */
+extern struct kmem_cache *smack_rule_cache;
+
extern struct security_operations smack_ops;
/*
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index d52c780..7aa696a 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3564,6 +3564,9 @@ static __init void init_smack_known_list(void)
list_add(&smack_known_web.list, &smack_known_list);
}
+/* KMEM caches for fast and thrifty allocations */
+struct kmem_cache *smack_rule_cache;
+
/**
* smack_init - initialize the smack system
*
@@ -3577,10 +3580,16 @@ static __init int smack_init(void)
if (!security_module_enable(&smack_ops))
return 0;
+ smack_rule_cache = KMEM_CACHE(smack_rule, 0);
+ if (!smack_rule_cache)
+ return -ENOMEM;
+
tsp = new_task_smack(smack_known_floor.smk_known,
smack_known_floor.smk_known, GFP_KERNEL);
- if (tsp == NULL)
+ if (tsp == NULL) {
+ kmem_cache_destroy(smack_rule_cache);
return -ENOMEM;
+ }
printk(KERN_INFO "Smack: Initializing.\n");
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 34d146b..4615242 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -217,7 +217,7 @@ static int smk_set_access(struct smack_parsed_rule *srp,
}
if (found == 0) {
- sp = kzalloc(sizeof(*sp), GFP_KERNEL);
+ sp = kmem_cache_zalloc(smack_rule_cache, GFP_KERNEL);
if (sp == NULL) {
rc = -ENOMEM;
goto out;
--
1.7.9.5