Message-ID: <51BCC359.2070300@schaufler-ca.com>
Date: Sat, 15 Jun 2013 12:41:13 -0700
From: Casey Schaufler <casey@schaufler-ca.com>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130509 Thunderbird/17.0.6
MIME-Version: 1.0
To: Tomasz Stanislawski <t.stanislaws@samsung.com>
CC: linux-security-module@vger.kernel.org, m.szyprowski@samsung.com,
        kyungmin.park@samsung.com, r.krypa@samsung.com,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC 2/5] security: smack: avoid kmalloc() in smk_parse_long_rule()
References: <1371137352-31273-1-git-send-email-t.stanislaws@samsung.com> <1371137352-31273-3-git-send-email-t.stanislaws@samsung.com>
In-Reply-To: <1371137352-31273-3-git-send-email-t.stanislaws@samsung.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4218
Lines: 127

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;
>  }
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/