Messages in this thread | | | Date | Sat, 15 Jun 2013 12:41:13 -0700 | From | Casey Schaufler <> | Subject | Re: [RFC 2/5] security: smack: avoid kmalloc() in smk_parse_long_rule() |
| |
On 6/13/2013 8:29 AM, Tomasz Stanislawski wrote: > 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. > > The buffer is allocated on stack what is slightly faster than kmalloc(). > > Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
There is hope for this patch, but it will need changes.
> --- > security/smack/smackfs.c | 67 +++++++++++++++++++++++----------------------- > 1 file changed, 33 insertions(+), 34 deletions(-) > > diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c > index 9a3cd0d..46f111e 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,20 @@ static int smk_parse_rule(const char *data, struct smack_parsed_rule *rule, > static int smk_parse_long_rule(const char *data, astruct smack_parsed_rule *rule, > int import, int change) > { > - char *subject; > - char *object; > - char *access1; > - char *access2; > - int datalen; > + char tmp[SMK_LOAD2LEN + 1];
As mentioned in patch 1 of this set, you can't put something this large on the stack. You could however use the same logic below on a single allocated buffer and reduce the number of kzallocs from four to one. That would get most of the improvement you're looking for.
> + char *args[4]; > int rc = -1; > > - /* This is inefficient */ > - datalen = strlen(data); > - > - /* Our first element can be 64 + \0 with no spaces */ > - subject = kzalloc(datalen + 1, GFP_KERNEL); > - if (subject == NULL) > - 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, tmp, 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, tmp, 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); > return rc; > } >
| |