linux-kernel - Re: [PATCH 1/5] Move sleeping operations to outside the semaphore.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 1 Jun 2009 21:40:31 -0500
From:	"Serge E. Hallyn" <serge@...lyn.com>
To:	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] Move sleeping operations to outside the semaphore.

Quoting Tetsuo Handa (penguin-kernel@...ove.sakura.ne.jp):
> TOMOYO is using rw_semaphore for protecting list elements.
> But TOMOYO is doing operations which might sleep inside down_write().
> This patch makes TOMOYO's sleeping operations go outside down_write().
> 
> Signed-off-by: Kentaro Takeda <takedakn@...data.co.jp>
> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> Signed-off-by: Toshiharu Harada <haradats@...data.co.jp>
> ---
>  security/tomoyo/common.c   |   80 ++++++---------------
>  security/tomoyo/common.h   |    2 
>  security/tomoyo/domain.c   |  107 ++++++++++++++--------------
>  security/tomoyo/file.c     |  133 ++++++++++++++++++-----------------
>  security/tomoyo/realpath.c |  169 ++++++++++++++-------------------------------
>  security/tomoyo/realpath.h |    7 -
>  6 files changed, 205 insertions(+), 293 deletions(-)
> 
> --- security-testing-2.6.git.orig/security/tomoyo/common.c
> +++ security-testing-2.6.git/security/tomoyo/common.c
> @@ -861,26 +861,27 @@ static struct tomoyo_profile *tomoyo_fin
>  								int profile)
>  {
>  	static DEFINE_MUTEX(lock);
> -	struct tomoyo_profile *ptr = NULL;
> -	int i;
> +	struct tomoyo_profile *new_ptr = NULL;
> +	struct tomoyo_profile *ptr;
>  
>  	if (profile >= TOMOYO_MAX_PROFILES)
>  		return NULL;
> +	new_ptr = kmalloc(sizeof(*new_ptr), GFP_KERNEL);
>  	/***** EXCLUSIVE SECTION START *****/
>  	mutex_lock(&lock);
>  	ptr = tomoyo_profile_ptr[profile];
> -	if (ptr)
> -		goto ok;
> -	ptr = tomoyo_alloc_element(sizeof(*ptr));
> -	if (!ptr)
> -		goto ok;
> -	for (i = 0; i < TOMOYO_MAX_CONTROL_INDEX; i++)
> -		ptr->value[i] = tomoyo_control_array[i].current_value;
> -	mb(); /* Avoid out-of-order execution. */
> -	tomoyo_profile_ptr[profile] = ptr;
> - ok:
> +	if (!ptr && tomoyo_memory_ok(new_ptr)) {
> +		int i;
> +		ptr = new_ptr;
> +		new_ptr = NULL;
> +		for (i = 0; i < TOMOYO_MAX_CONTROL_INDEX; i++)
> +			ptr->value[i] = tomoyo_control_array[i].current_value;
> +		mb(); /* Avoid out-of-order execution. */
> +		tomoyo_profile_ptr[profile] = ptr;
> +	}
>  	mutex_unlock(&lock);
>  	/***** EXCLUSIVE SECTION END *****/
> +	kfree(new_ptr);
>  	return ptr;
>  }

Again I feel (no offense) like I'm reading Ada code here...

Focusing just on this one function,

1. the mutex lock belonging to this function really is just protecting
writes to elements of tomoyo_profile_ptr.  It should be defined,
with a descriptive name and comment, next to tomoyo_profile_ptr
at common.c:46.

2. I see no reason for this not to be a fast spinlock at this
point.

3. Once it's a fast checkpoint, you can change the flow a bit
(unless there is good reason not to) to do:

	spin_lock(&profile_lock);
	ptr = tomoyo_profile_ptr[profile];
	spin_unlock(&profile_lock);

	if (ptr)
		return ptr;

	new_ptr = kmalloc(sizeof(*new_ptr), GFP_KERNEL);
	if (!new_ptr)
		return ERR_PTR(-ENOMEM);

	spin_lock(&profile_lock);
	if (!tomoyo_profile_ptr[profile]) {
		/* do your initialization as above */
		ptr = tomoyo_profile_ptr[profile] = new_ptr;
		new_ptr = NULL;
	}
	spin_unlock(&profile_lock);
	kfree(newptr);
	return ptr;

which avoids the kmalloc in the common case.

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