[PATCH 2/5] TOMOYO: Clarify lock protected section.

[PATCH 2/5] TOMOYO: Clarify lock protected section.

[Tetsuo Handa]

Enclose reader section in
	/***** READER SECTION START *****/
and
	/***** READER SECTION END *****/
and writer section in
	/***** WRITER SECTION START *****/
and
	/***** WRITER SECTION END *****/
in order to avoid oversighting lock protected section.

Signed-off-by: Kentaro Takeda <takedakn@nttdata.co.jp>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Toshiharu Harada <haradats@nttdata.co.jp>
---
 security/tomoyo/common.c   |   30 ++++++++++++++++++++++++++++--
 security/tomoyo/domain.c   |   34 ++++++++++++++++++++++++----------
 security/tomoyo/file.c     |   36 ++++++++++++++++++++++++++----------
 security/tomoyo/realpath.c |    2 ++
 4 files changed, 80 insertions(+), 22 deletions(-)

--- security-testing-2.6.git.orig/security/tomoyo/common.c
+++ security-testing-2.6.git/security/tomoyo/common.c
@@ -706,6 +706,7 @@ static const char *tomoyo_get_exe(void)
 
 	if (!mm)
 		return NULL;
+	/***** READER SECTION START *****/
 	down_read(&mm->mmap_sem);
 	for (vma = mm->mmap; vma; vma = vma->vm_next) {
 		if ((vma->vm_flags & VM_EXECUTABLE) && vma->vm_file) {
@@ -714,6 +715,7 @@ static const char *tomoyo_get_exe(void)
 		}
 	}
 	up_read(&mm->mmap_sem);
+	/***** READER SECTION END *****/
 	return cp;
 }
 
@@ -784,6 +786,7 @@ bool tomoyo_domain_quota_is_ok(struct to
 
 	if (!domain)
 		return true;
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_domain_acl_info_list_lock);
 	list_for_each_entry(ptr, &domain->acl_info_list, list) {
 		if (ptr->type & TOMOYO_ACL_DELETED)
@@ -839,6 +842,7 @@ bool tomoyo_domain_quota_is_ok(struct to
 		}
 	}
 	up_read(&tomoyo_domain_acl_info_list_lock);
+	/***** READER SECTION END *****/
 	if (count < tomoyo_check_flags(domain, TOMOYO_MAX_ACCEPT_ENTRY))
 		return true;
 	if (!domain->quota_warned) {
@@ -1053,7 +1057,7 @@ static int tomoyo_update_manager_entry(c
 		return -ENOMEM;
 	if (!is_delete)
 		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
-	/***** EXCLUSIVE SECTION START *****/
+	/***** WRITER SECTION START *****/
 	down_write(&tomoyo_policy_manager_list_lock);
 	list_for_each_entry(ptr, &tomoyo_policy_manager_list, list) {
 		if (ptr->manager != saved_manager)
@@ -1070,7 +1074,7 @@ static int tomoyo_update_manager_entry(c
 		error = 0;
 	}
 	up_write(&tomoyo_policy_manager_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
+	/***** WRITER SECTION END *****/
 	kfree(new_entry);
 	return error;
 }
@@ -1108,6 +1112,7 @@ static int tomoyo_read_manager_policy(st
 
 	if (head->read_eof)
 		return 0;
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_policy_manager_list_lock);
 	list_for_each_cookie(pos, head->read_var2,
 			     &tomoyo_policy_manager_list) {
@@ -1122,6 +1127,7 @@ static int tomoyo_read_manager_policy(st
 		}
 	}
 	up_read(&tomoyo_policy_manager_list_lock);
+	/***** READER SECTION END *****/
 	head->read_eof = done;
 	return 0;
 }
@@ -1144,6 +1150,7 @@ static bool tomoyo_is_policy_manager(voi
 		return true;
 	if (!tomoyo_manage_by_non_root && (task->cred->uid || task->cred->euid))
 		return false;
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_policy_manager_list_lock);
 	list_for_each_entry(ptr, &tomoyo_policy_manager_list, list) {
 		if (!ptr->is_deleted && ptr->is_domain
@@ -1153,11 +1160,13 @@ static bool tomoyo_is_policy_manager(voi
 		}
 	}
 	up_read(&tomoyo_policy_manager_list_lock);
+	/***** READER SECTION END *****/
 	if (found)
 		return true;
 	exe = tomoyo_get_exe();
 	if (!exe)
 		return false;
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_policy_manager_list_lock);
 	list_for_each_entry(ptr, &tomoyo_policy_manager_list, list) {
 		if (!ptr->is_deleted && !ptr->is_domain
@@ -1167,6 +1176,7 @@ static bool tomoyo_is_policy_manager(voi
 		}
 	}
 	up_read(&tomoyo_policy_manager_list_lock);
+	/***** READER SECTION END *****/
 	if (!found) { /* Reduce error messages. */
 		static pid_t last_pid;
 		const pid_t pid = current->pid;
@@ -1205,9 +1215,11 @@ static bool tomoyo_is_select_one(struct 
 		/***** CRITICAL SECTION END *****/
 	} else if (!strncmp(data, "domain=", 7)) {
 		if (tomoyo_is_domain_def(data + 7)) {
+			/***** READER SECTION START *****/
 			down_read(&tomoyo_domain_list_lock);
 			domain = tomoyo_find_domain(data + 7);
 			up_read(&tomoyo_domain_list_lock);
+			/***** READER SECTION END *****/
 		}
 	} else
 		return false;
@@ -1222,6 +1234,7 @@ static bool tomoyo_is_select_one(struct 
 	if (domain) {
 		struct tomoyo_domain_info *d;
 		head->read_var1 = NULL;
+		/***** READER SECTION START *****/
 		down_read(&tomoyo_domain_list_lock);
 		list_for_each_entry(d, &tomoyo_domain_list, list) {
 			if (d == domain)
@@ -1229,6 +1242,7 @@ static bool tomoyo_is_select_one(struct 
 			head->read_var1 = &d->list;
 		}
 		up_read(&tomoyo_domain_list_lock);
+		/***** READER SECTION END *****/
 		head->read_var2 = NULL;
 		head->read_bit = 0;
 		head->read_step = 0;
@@ -1267,9 +1281,11 @@ static int tomoyo_write_domain_policy(st
 		if (is_delete)
 			tomoyo_delete_domain(data);
 		else if (is_select) {
+			/***** READER SECTION START *****/
 			down_read(&tomoyo_domain_list_lock);
 			domain = tomoyo_find_domain(data);
 			up_read(&tomoyo_domain_list_lock);
+			/***** READER SECTION END *****/
 		} else
 			domain = tomoyo_find_or_assign_new_domain(data, 0);
 		head->write_var1 = domain;
@@ -1426,6 +1442,7 @@ static int tomoyo_read_domain_policy(str
 		return 0;
 	if (head->read_step == 0)
 		head->read_step = 1;
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_domain_list_lock);
 	list_for_each_cookie(dpos, head->read_var1, &tomoyo_domain_list) {
 		struct tomoyo_domain_info *domain;
@@ -1460,6 +1477,7 @@ acl_loop:
 		if (head->read_step == 3)
 			goto tail_mark;
 		/* Print ACL entries in the domain. */
+		/***** READER SECTION START *****/
 		down_read(&tomoyo_domain_acl_info_list_lock);
 		list_for_each_cookie(apos, head->read_var2,
 				      &domain->acl_info_list) {
@@ -1472,6 +1490,7 @@ acl_loop:
 			}
 		}
 		up_read(&tomoyo_domain_acl_info_list_lock);
+		/***** READER SECTION END *****/
 		if (!done)
 			break;
 		head->read_step = 3;
@@ -1485,6 +1504,7 @@ tail_mark:
 			break;
 	}
 	up_read(&tomoyo_domain_list_lock);
+	/***** READER SECTION END *****/
 	head->read_eof = done;
 	return 0;
 }
@@ -1511,9 +1531,11 @@ static int tomoyo_write_domain_profile(s
 	if (!cp)
 		return -EINVAL;
 	*cp = '\0';
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_domain_list_lock);
 	domain = tomoyo_find_domain(cp + 1);
 	up_read(&tomoyo_domain_list_lock);
+	/***** READER SECTION END *****/
 	if (strict_strtoul(data, 10, &profile))
 		return -EINVAL;
 	if (domain && profile < TOMOYO_MAX_PROFILES
@@ -1543,6 +1565,7 @@ static int tomoyo_read_domain_profile(st
 
 	if (head->read_eof)
 		return 0;
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_domain_list_lock);
 	list_for_each_cookie(pos, head->read_var1, &tomoyo_domain_list) {
 		struct tomoyo_domain_info *domain;
@@ -1556,6 +1579,7 @@ static int tomoyo_read_domain_profile(st
 		}
 	}
 	up_read(&tomoyo_domain_list_lock);
+	/***** READER SECTION END *****/
 	head->read_eof = done;
 	return 0;
 }
@@ -1777,6 +1801,7 @@ void tomoyo_load_policy(const char *file
 	tomoyo_policy_loaded = true;
 	{ /* Check all profiles currently assigned to domains are defined. */
 		struct tomoyo_domain_info *domain;
+		/***** READER SECTION START *****/
 		down_read(&tomoyo_domain_list_lock);
 		list_for_each_entry(domain, &tomoyo_domain_list, list) {
 			const u8 profile = domain->profile;
@@ -1786,6 +1811,7 @@ void tomoyo_load_policy(const char *file
 			      profile, domain->domainname->name);
 		}
 		up_read(&tomoyo_domain_list_lock);
+		/***** READER SECTION END *****/
 	}
 }
 
--- security-testing-2.6.git.orig/security/tomoyo/domain.c
+++ security-testing-2.6.git/security/tomoyo/domain.c
@@ -137,7 +137,7 @@ static int tomoyo_update_domain_initiali
 		return -ENOMEM;
 	if (!is_delete)
 		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
-	/***** EXCLUSIVE SECTION START *****/
+	/***** WRITER SECTION START *****/
 	down_write(&tomoyo_domain_initializer_list_lock);
 	list_for_each_entry(ptr, &tomoyo_domain_initializer_list, list) {
 		if (ptr->is_not != is_not ||
@@ -159,7 +159,7 @@ static int tomoyo_update_domain_initiali
 		error = 0;
 	}
 	up_write(&tomoyo_domain_initializer_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
+	/***** WRITER SECTION END *****/
 	kfree(new_entry);
 	return error;
 }
@@ -176,6 +176,7 @@ bool tomoyo_read_domain_initializer_poli
 	struct list_head *pos;
 	bool done = true;
 
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_domain_initializer_list_lock);
 	list_for_each_cookie(pos, head->read_var2,
 			     &tomoyo_domain_initializer_list) {
@@ -201,6 +202,7 @@ bool tomoyo_read_domain_initializer_poli
 		}
 	}
 	up_read(&tomoyo_domain_initializer_list_lock);
+	/***** READER SECTION END *****/
 	return done;
 }
 
@@ -247,6 +249,7 @@ static bool tomoyo_is_domain_initializer
 	struct tomoyo_domain_initializer_entry *ptr;
 	bool flag = false;
 
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_domain_initializer_list_lock);
 	list_for_each_entry(ptr,  &tomoyo_domain_initializer_list, list) {
 		if (ptr->is_deleted)
@@ -269,6 +272,7 @@ static bool tomoyo_is_domain_initializer
 		flag = true;
 	}
 	up_read(&tomoyo_domain_initializer_list_lock);
+	/***** READER SECTION END *****/
 	return flag;
 }
 
@@ -316,7 +320,7 @@ static int tomoyo_update_domain_keeper_e
 		return -ENOMEM;
 	if (!is_delete)
 		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
-	/***** EXCLUSIVE SECTION START *****/
+	/***** WRITER SECTION START *****/
 	down_write(&tomoyo_domain_keeper_list_lock);
 	list_for_each_entry(ptr, &tomoyo_domain_keeper_list, list) {
 		if (ptr->is_not != is_not ||
@@ -337,7 +341,7 @@ static int tomoyo_update_domain_keeper_e
 		error = 0;
 	}
 	up_write(&tomoyo_domain_keeper_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
+	/***** WRITER SECTION END *****/
 	kfree(new_entry);
 	return error;
 }
@@ -375,6 +379,7 @@ bool tomoyo_read_domain_keeper_policy(st
 	struct list_head *pos;
 	bool done = true;
 
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_domain_keeper_list_lock);
 	list_for_each_cookie(pos, head->read_var2,
 			     &tomoyo_domain_keeper_list) {
@@ -400,6 +405,7 @@ bool tomoyo_read_domain_keeper_policy(st
 		}
 	}
 	up_read(&tomoyo_domain_keeper_list_lock);
+	/***** READER SECTION END *****/
 	return done;
 }
 
@@ -420,6 +426,7 @@ static bool tomoyo_is_domain_keeper(cons
 	struct tomoyo_domain_keeper_entry *ptr;
 	bool flag = false;
 
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_domain_keeper_list_lock);
 	list_for_each_entry(ptr, &tomoyo_domain_keeper_list, list) {
 		if (ptr->is_deleted)
@@ -440,6 +447,7 @@ static bool tomoyo_is_domain_keeper(cons
 		flag = true;
 	}
 	up_read(&tomoyo_domain_keeper_list_lock);
+	/***** READER SECTION END *****/
 	return flag;
 }
 
@@ -475,7 +483,7 @@ static int tomoyo_update_alias_entry(con
 		return -ENOMEM;
 	if (!is_delete)
 		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
-	/***** EXCLUSIVE SECTION START *****/
+	/***** WRITER SECTION START *****/
 	down_write(&tomoyo_alias_list_lock);
 	list_for_each_entry(ptr, &tomoyo_alias_list, list) {
 		if (ptr->original_name != saved_original_name ||
@@ -493,7 +501,7 @@ static int tomoyo_update_alias_entry(con
 		error = 0;
 	}
 	up_write(&tomoyo_alias_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
+	/***** WRITER SECTION END *****/
 	kfree(new_entry);
 	return error;
 }
@@ -510,6 +518,7 @@ bool tomoyo_read_alias_policy(struct tom
 	struct list_head *pos;
 	bool done = true;
 
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_alias_list_lock);
 	list_for_each_cookie(pos, head->read_var2, &tomoyo_alias_list) {
 		struct tomoyo_alias_entry *ptr;
@@ -525,6 +534,7 @@ bool tomoyo_read_alias_policy(struct tom
 		}
 	}
 	up_read(&tomoyo_alias_list_lock);
+	/***** READER SECTION END *****/
 	return done;
 }
 
@@ -562,7 +572,7 @@ int tomoyo_delete_domain(char *domainnam
 
 	name.name = domainname;
 	tomoyo_fill_path_info(&name);
-	/***** EXCLUSIVE SECTION START *****/
+	/***** WRITER SECTION START *****/
 	down_write(&tomoyo_domain_list_lock);
 	/* Is there an active domain? */
 	list_for_each_entry(domain, &tomoyo_domain_list, list) {
@@ -576,7 +586,7 @@ int tomoyo_delete_domain(char *domainnam
 		break;
 	}
 	up_write(&tomoyo_domain_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
+	/***** WRITER SECTION END *****/
 	return 0;
 }
 
@@ -602,7 +612,7 @@ struct tomoyo_domain_info *tomoyo_find_o
 	if (!saved_domainname)
 		return NULL;
 	new_domain = kmalloc(sizeof(*new_domain), GFP_KERNEL);
-	/***** EXCLUSIVE SECTION START *****/
+	/***** WRITER SECTION START *****/
 	down_write(&tomoyo_domain_list_lock);
 	domain = tomoyo_find_domain(domainname);
 	if (domain)
@@ -649,7 +659,7 @@ struct tomoyo_domain_info *tomoyo_find_o
 	}
  out:
 	up_write(&tomoyo_domain_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
+	/***** WRITER SECTION END *****/
 	kfree(new_domain);
 	return domain;
 }
@@ -722,6 +732,7 @@ int tomoyo_find_next_domain(struct linux
 	if (tomoyo_pathcmp(&r, &s)) {
 		struct tomoyo_alias_entry *ptr;
 		/* Is this program allowed to be called via symbolic links? */
+		/***** READER SECTION START *****/
 		down_read(&tomoyo_alias_list_lock);
 		list_for_each_entry(ptr, &tomoyo_alias_list, list) {
 			if (ptr->is_deleted ||
@@ -735,6 +746,7 @@ int tomoyo_find_next_domain(struct linux
 			break;
 		}
 		up_read(&tomoyo_alias_list_lock);
+		/***** READER SECTION END *****/
 	}
 
 	/* Check execute permission. */
@@ -765,9 +777,11 @@ int tomoyo_find_next_domain(struct linux
 	}
 	if (domain || strlen(new_domain_name) >= TOMOYO_MAX_PATHNAME_LEN)
 		goto done;
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_domain_list_lock);
 	domain = tomoyo_find_domain(new_domain_name);
 	up_read(&tomoyo_domain_list_lock);
+	/***** READER SECTION END *****/
 	if (domain)
 		goto done;
 	if (is_enforce)
--- security-testing-2.6.git.orig/security/tomoyo/file.c
+++ security-testing-2.6.git/security/tomoyo/file.c
@@ -168,7 +168,7 @@ static int tomoyo_update_globally_readab
 		return -ENOMEM;
 	if (!is_delete)
 		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
-	/***** EXCLUSIVE SECTION START *****/
+	/***** WRITER SECTION START *****/
 	down_write(&tomoyo_globally_readable_list_lock);
 	list_for_each_entry(ptr, &tomoyo_globally_readable_list, list) {
 		if (ptr->filename != saved_filename)
@@ -184,7 +184,7 @@ static int tomoyo_update_globally_readab
 		error = 0;
 	}
 	up_write(&tomoyo_globally_readable_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
+	/***** WRITER SECTION END *****/
 	kfree(new_entry);
 	return error;
 }
@@ -201,6 +201,7 @@ static bool tomoyo_is_globally_readable_
 {
 	struct tomoyo_globally_readable_file_entry *ptr;
 	bool found = false;
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_globally_readable_list_lock);
 	list_for_each_entry(ptr, &tomoyo_globally_readable_list, list) {
 		if (!ptr->is_deleted &&
@@ -210,6 +211,7 @@ static bool tomoyo_is_globally_readable_
 		}
 	}
 	up_read(&tomoyo_globally_readable_list_lock);
+	/***** READER SECTION END *****/
 	return found;
 }
 
@@ -238,6 +240,7 @@ bool tomoyo_read_globally_readable_polic
 	struct list_head *pos;
 	bool done = true;
 
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_globally_readable_list_lock);
 	list_for_each_cookie(pos, head->read_var2,
 			     &tomoyo_globally_readable_list) {
@@ -254,6 +257,7 @@ bool tomoyo_read_globally_readable_polic
 		}
 	}
 	up_read(&tomoyo_globally_readable_list_lock);
+	/***** READER SECTION END *****/
 	return done;
 }
 
@@ -284,7 +288,7 @@ static int tomoyo_update_file_pattern_en
 		return -ENOMEM;
 	if (!is_delete)
 		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
-	/***** EXCLUSIVE SECTION START *****/
+	/***** WRITER SECTION START *****/
 	down_write(&tomoyo_pattern_list_lock);
 	list_for_each_entry(ptr, &tomoyo_pattern_list, list) {
 		if (saved_pattern != ptr->pattern)
@@ -300,7 +304,7 @@ static int tomoyo_update_file_pattern_en
 		error = 0;
 	}
 	up_write(&tomoyo_pattern_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
+	/***** WRITER SECTION END *****/
 	kfree(new_entry);
 	return error;
 }
@@ -318,6 +322,7 @@ tomoyo_get_file_pattern(const struct tom
 	struct tomoyo_pattern_entry *ptr;
 	const struct tomoyo_path_info *pattern = NULL;
 
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_pattern_list_lock);
 	list_for_each_entry(ptr, &tomoyo_pattern_list, list) {
 		if (ptr->is_deleted)
@@ -333,6 +338,7 @@ tomoyo_get_file_pattern(const struct tom
 		}
 	}
 	up_read(&tomoyo_pattern_list_lock);
+	/***** READER SECTION END *****/
 	if (pattern)
 		filename = pattern;
 	return filename;
@@ -363,6 +369,7 @@ bool tomoyo_read_file_pattern(struct tom
 	struct list_head *pos;
 	bool done = true;
 
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_pattern_list_lock);
 	list_for_each_cookie(pos, head->read_var2, &tomoyo_pattern_list) {
 		struct tomoyo_pattern_entry *ptr;
@@ -376,6 +383,7 @@ bool tomoyo_read_file_pattern(struct tom
 		}
 	}
 	up_read(&tomoyo_pattern_list_lock);
+	/***** READER SECTION END *****/
 	return done;
 }
 
@@ -406,7 +414,7 @@ static int tomoyo_update_no_rewrite_entr
 		return -ENOMEM;
 	if (!is_delete)
 		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
-	/***** EXCLUSIVE SECTION START *****/
+	/***** WRITER SECTION START *****/
 	down_write(&tomoyo_no_rewrite_list_lock);
 	list_for_each_entry(ptr, &tomoyo_no_rewrite_list, list) {
 		if (ptr->pattern != saved_pattern)
@@ -422,7 +430,7 @@ static int tomoyo_update_no_rewrite_entr
 		error = 0;
 	}
 	up_write(&tomoyo_no_rewrite_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
+	/***** WRITER SECTION END *****/
 	kfree(new_entry);
 	return error;
 }
@@ -440,6 +448,7 @@ static bool tomoyo_is_no_rewrite_file(co
 	struct tomoyo_no_rewrite_entry *ptr;
 	bool found = false;
 
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_no_rewrite_list_lock);
 	list_for_each_entry(ptr, &tomoyo_no_rewrite_list, list) {
 		if (ptr->is_deleted)
@@ -450,6 +459,7 @@ static bool tomoyo_is_no_rewrite_file(co
 		break;
 	}
 	up_read(&tomoyo_no_rewrite_list_lock);
+	/***** READER SECTION END *****/
 	return found;
 }
 
@@ -478,6 +488,7 @@ bool tomoyo_read_no_rewrite_policy(struc
 	struct list_head *pos;
 	bool done = true;
 
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_no_rewrite_list_lock);
 	list_for_each_cookie(pos, head->read_var2, &tomoyo_no_rewrite_list) {
 		struct tomoyo_no_rewrite_entry *ptr;
@@ -491,6 +502,7 @@ bool tomoyo_read_no_rewrite_policy(struc
 		}
 	}
 	up_read(&tomoyo_no_rewrite_list_lock);
+	/***** READER SECTION END *****/
 	return done;
 }
 
@@ -556,6 +568,7 @@ static int tomoyo_check_single_path_acl2
 	struct tomoyo_acl_info *ptr;
 	int error = -EPERM;
 
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_domain_acl_info_list_lock);
 	list_for_each_entry(ptr, &domain->acl_info_list, list) {
 		struct tomoyo_single_path_acl_record *acl;
@@ -576,6 +589,7 @@ static int tomoyo_check_single_path_acl2
 		break;
 	}
 	up_read(&tomoyo_domain_acl_info_list_lock);
+	/***** READER SECTION END *****/
 	return error;
 }
 
@@ -742,7 +756,7 @@ static int tomoyo_update_single_path_acl
 		return -ENOMEM;
 	if (!is_delete)
 		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
-	/***** EXCLUSIVE SECTION START *****/
+	/***** WRITER SECTION START *****/
 	down_write(&tomoyo_domain_acl_info_list_lock);
 	if (is_delete)
 		goto delete;
@@ -799,7 +813,7 @@ static int tomoyo_update_single_path_acl
 	}
  out:
 	up_write(&tomoyo_domain_acl_info_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
+	/***** WRITER SECTION END *****/
 	kfree(new_entry);
 	return error;
 }
@@ -838,7 +852,7 @@ static int tomoyo_update_double_path_acl
 		return -ENOMEM;
 	if (!is_delete)
 		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
-	/***** EXCLUSIVE SECTION START *****/
+	/***** WRITER SECTION START *****/
 	down_write(&tomoyo_domain_acl_info_list_lock);
 	if (is_delete)
 		goto delete;
@@ -888,7 +902,7 @@ static int tomoyo_update_double_path_acl
 	}
  out:
 	up_write(&tomoyo_domain_acl_info_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
+	/***** WRITER SECTION END *****/
 	kfree(new_entry);
 	return error;
 }
@@ -934,6 +948,7 @@ static int tomoyo_check_double_path_acl(
 
 	if (!tomoyo_check_flags(domain, TOMOYO_MAC_FOR_FILE))
 		return 0;
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_domain_acl_info_list_lock);
 	list_for_each_entry(ptr, &domain->acl_info_list, list) {
 		struct tomoyo_double_path_acl_record *acl;
@@ -951,6 +966,7 @@ static int tomoyo_check_double_path_acl(
 		break;
 	}
 	up_read(&tomoyo_domain_acl_info_list_lock);
+	/***** READER SECTION END *****/
 	return error;
 }
 
--- security-testing-2.6.git.orig/security/tomoyo/realpath.c
+++ security-testing-2.6.git/security/tomoyo/realpath.c
@@ -326,10 +326,12 @@ void __init tomoyo_realpath_init(void)
 	INIT_LIST_HEAD(&tomoyo_kernel_domain.acl_info_list);
 	tomoyo_kernel_domain.domainname = tomoyo_save_name(TOMOYO_ROOT_NAME);
 	list_add_tail(&tomoyo_kernel_domain.list, &tomoyo_domain_list);
+	/***** READER SECTION START *****/
 	down_read(&tomoyo_domain_list_lock);
 	if (tomoyo_find_domain(TOMOYO_ROOT_NAME) != &tomoyo_kernel_domain)
 		panic("Can't register tomoyo_kernel_domain");
 	up_read(&tomoyo_domain_list_lock);
+	/***** READER SECTION END *****/
 }
 
 /* Memory allocated for temporary purpose. */

Re: [PATCH 2/5] TOMOYO: Clarify lock protected section.

[Li Zefan]

Tetsuo Handa wrote:
> Enclose reader section in
> 	/***** READER SECTION START *****/
> and
> 	/***** READER SECTION END *****/
> and writer section in
> 	/***** WRITER SECTION START *****/
> and
> 	/***** WRITER SECTION END *****/
> in order to avoid oversighting lock protected section.
> 

This makes me a bit uncomfortable..

IMHO this seems ugly, useless, and even harmful. If it's helpful,
we'd be doing this for the whole kernel tree, which is crazy..

Or does tomoyo do this for it's special reason?

> Signed-off-by: Kentaro Takeda <takedakn@nttdata.co.jp>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Toshiharu Harada <haradats@nttdata.co.jp>
> ---
>  security/tomoyo/common.c   |   30 ++++++++++++++++++++++++++++--
>  security/tomoyo/domain.c   |   34 ++++++++++++++++++++++++----------
>  security/tomoyo/file.c     |   36 ++++++++++++++++++++++++++----------
>  security/tomoyo/realpath.c |    2 ++
>  4 files changed, 80 insertions(+), 22 deletions(-)
> 
> --- security-testing-2.6.git.orig/security/tomoyo/common.c
> +++ security-testing-2.6.git/security/tomoyo/common.c
> @@ -706,6 +706,7 @@ static const char *tomoyo_get_exe(void)
>  
>  	if (!mm)
>  		return NULL;
> +	/***** READER SECTION START *****/
>  	down_read(&mm->mmap_sem);
>  	for (vma = mm->mmap; vma; vma = vma->vm_next) {
>  		if ((vma->vm_flags & VM_EXECUTABLE) && vma->vm_file) {
> @@ -714,6 +715,7 @@ static const char *tomoyo_get_exe(void)
>  		}
>  	}
>  	up_read(&mm->mmap_sem);
> +	/***** READER SECTION END *****/
>  	return cp;
>  }
>  
> @@ -784,6 +786,7 @@ bool tomoyo_domain_quota_is_ok(struct to
>  
>  	if (!domain)
>  		return true;
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_domain_acl_info_list_lock);
>  	list_for_each_entry(ptr, &domain->acl_info_list, list) {
>  		if (ptr->type & TOMOYO_ACL_DELETED)
> @@ -839,6 +842,7 @@ bool tomoyo_domain_quota_is_ok(struct to
>  		}
>  	}
>  	up_read(&tomoyo_domain_acl_info_list_lock);
> +	/***** READER SECTION END *****/
>  	if (count < tomoyo_check_flags(domain, TOMOYO_MAX_ACCEPT_ENTRY))
>  		return true;
>  	if (!domain->quota_warned) {
> @@ -1053,7 +1057,7 @@ static int tomoyo_update_manager_entry(c
>  		return -ENOMEM;
>  	if (!is_delete)
>  		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
> -	/***** EXCLUSIVE SECTION START *****/
> +	/***** WRITER SECTION START *****/
>  	down_write(&tomoyo_policy_manager_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_policy_manager_list, list) {
>  		if (ptr->manager != saved_manager)
> @@ -1070,7 +1074,7 @@ static int tomoyo_update_manager_entry(c
>  		error = 0;
>  	}
>  	up_write(&tomoyo_policy_manager_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
> +	/***** WRITER SECTION END *****/
>  	kfree(new_entry);
>  	return error;
>  }
> @@ -1108,6 +1112,7 @@ static int tomoyo_read_manager_policy(st
>  
>  	if (head->read_eof)
>  		return 0;
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_policy_manager_list_lock);
>  	list_for_each_cookie(pos, head->read_var2,
>  			     &tomoyo_policy_manager_list) {
> @@ -1122,6 +1127,7 @@ static int tomoyo_read_manager_policy(st
>  		}
>  	}
>  	up_read(&tomoyo_policy_manager_list_lock);
> +	/***** READER SECTION END *****/
>  	head->read_eof = done;
>  	return 0;
>  }
> @@ -1144,6 +1150,7 @@ static bool tomoyo_is_policy_manager(voi
>  		return true;
>  	if (!tomoyo_manage_by_non_root && (task->cred->uid || task->cred->euid))
>  		return false;
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_policy_manager_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_policy_manager_list, list) {
>  		if (!ptr->is_deleted && ptr->is_domain
> @@ -1153,11 +1160,13 @@ static bool tomoyo_is_policy_manager(voi
>  		}
>  	}
>  	up_read(&tomoyo_policy_manager_list_lock);
> +	/***** READER SECTION END *****/
>  	if (found)
>  		return true;
>  	exe = tomoyo_get_exe();
>  	if (!exe)
>  		return false;
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_policy_manager_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_policy_manager_list, list) {
>  		if (!ptr->is_deleted && !ptr->is_domain
> @@ -1167,6 +1176,7 @@ static bool tomoyo_is_policy_manager(voi
>  		}
>  	}
>  	up_read(&tomoyo_policy_manager_list_lock);
> +	/***** READER SECTION END *****/
>  	if (!found) { /* Reduce error messages. */
>  		static pid_t last_pid;
>  		const pid_t pid = current->pid;
> @@ -1205,9 +1215,11 @@ static bool tomoyo_is_select_one(struct 
>  		/***** CRITICAL SECTION END *****/
>  	} else if (!strncmp(data, "domain=", 7)) {
>  		if (tomoyo_is_domain_def(data + 7)) {
> +			/***** READER SECTION START *****/
>  			down_read(&tomoyo_domain_list_lock);
>  			domain = tomoyo_find_domain(data + 7);
>  			up_read(&tomoyo_domain_list_lock);
> +			/***** READER SECTION END *****/
>  		}
>  	} else
>  		return false;
> @@ -1222,6 +1234,7 @@ static bool tomoyo_is_select_one(struct 
>  	if (domain) {
>  		struct tomoyo_domain_info *d;
>  		head->read_var1 = NULL;
> +		/***** READER SECTION START *****/
>  		down_read(&tomoyo_domain_list_lock);
>  		list_for_each_entry(d, &tomoyo_domain_list, list) {
>  			if (d == domain)
> @@ -1229,6 +1242,7 @@ static bool tomoyo_is_select_one(struct 
>  			head->read_var1 = &d->list;
>  		}
>  		up_read(&tomoyo_domain_list_lock);
> +		/***** READER SECTION END *****/
>  		head->read_var2 = NULL;
>  		head->read_bit = 0;
>  		head->read_step = 0;
> @@ -1267,9 +1281,11 @@ static int tomoyo_write_domain_policy(st
>  		if (is_delete)
>  			tomoyo_delete_domain(data);
>  		else if (is_select) {
> +			/***** READER SECTION START *****/
>  			down_read(&tomoyo_domain_list_lock);
>  			domain = tomoyo_find_domain(data);
>  			up_read(&tomoyo_domain_list_lock);
> +			/***** READER SECTION END *****/
>  		} else
>  			domain = tomoyo_find_or_assign_new_domain(data, 0);
>  		head->write_var1 = domain;
> @@ -1426,6 +1442,7 @@ static int tomoyo_read_domain_policy(str
>  		return 0;
>  	if (head->read_step == 0)
>  		head->read_step = 1;
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_domain_list_lock);
>  	list_for_each_cookie(dpos, head->read_var1, &tomoyo_domain_list) {
>  		struct tomoyo_domain_info *domain;
> @@ -1460,6 +1477,7 @@ acl_loop:
>  		if (head->read_step == 3)
>  			goto tail_mark;
>  		/* Print ACL entries in the domain. */
> +		/***** READER SECTION START *****/
>  		down_read(&tomoyo_domain_acl_info_list_lock);
>  		list_for_each_cookie(apos, head->read_var2,
>  				      &domain->acl_info_list) {
> @@ -1472,6 +1490,7 @@ acl_loop:
>  			}
>  		}
>  		up_read(&tomoyo_domain_acl_info_list_lock);
> +		/***** READER SECTION END *****/
>  		if (!done)
>  			break;
>  		head->read_step = 3;
> @@ -1485,6 +1504,7 @@ tail_mark:
>  			break;
>  	}
>  	up_read(&tomoyo_domain_list_lock);
> +	/***** READER SECTION END *****/
>  	head->read_eof = done;
>  	return 0;
>  }
> @@ -1511,9 +1531,11 @@ static int tomoyo_write_domain_profile(s
>  	if (!cp)
>  		return -EINVAL;
>  	*cp = '\0';
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_domain_list_lock);
>  	domain = tomoyo_find_domain(cp + 1);
>  	up_read(&tomoyo_domain_list_lock);
> +	/***** READER SECTION END *****/
>  	if (strict_strtoul(data, 10, &profile))
>  		return -EINVAL;
>  	if (domain && profile < TOMOYO_MAX_PROFILES
> @@ -1543,6 +1565,7 @@ static int tomoyo_read_domain_profile(st
>  
>  	if (head->read_eof)
>  		return 0;
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_domain_list_lock);
>  	list_for_each_cookie(pos, head->read_var1, &tomoyo_domain_list) {
>  		struct tomoyo_domain_info *domain;
> @@ -1556,6 +1579,7 @@ static int tomoyo_read_domain_profile(st
>  		}
>  	}
>  	up_read(&tomoyo_domain_list_lock);
> +	/***** READER SECTION END *****/
>  	head->read_eof = done;
>  	return 0;
>  }
> @@ -1777,6 +1801,7 @@ void tomoyo_load_policy(const char *file
>  	tomoyo_policy_loaded = true;
>  	{ /* Check all profiles currently assigned to domains are defined. */
>  		struct tomoyo_domain_info *domain;
> +		/***** READER SECTION START *****/
>  		down_read(&tomoyo_domain_list_lock);
>  		list_for_each_entry(domain, &tomoyo_domain_list, list) {
>  			const u8 profile = domain->profile;
> @@ -1786,6 +1811,7 @@ void tomoyo_load_policy(const char *file
>  			      profile, domain->domainname->name);
>  		}
>  		up_read(&tomoyo_domain_list_lock);
> +		/***** READER SECTION END *****/
>  	}
>  }
>  
> --- security-testing-2.6.git.orig/security/tomoyo/domain.c
> +++ security-testing-2.6.git/security/tomoyo/domain.c
> @@ -137,7 +137,7 @@ static int tomoyo_update_domain_initiali
>  		return -ENOMEM;
>  	if (!is_delete)
>  		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
> -	/***** EXCLUSIVE SECTION START *****/
> +	/***** WRITER SECTION START *****/
>  	down_write(&tomoyo_domain_initializer_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_domain_initializer_list, list) {
>  		if (ptr->is_not != is_not ||
> @@ -159,7 +159,7 @@ static int tomoyo_update_domain_initiali
>  		error = 0;
>  	}
>  	up_write(&tomoyo_domain_initializer_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
> +	/***** WRITER SECTION END *****/
>  	kfree(new_entry);
>  	return error;
>  }
> @@ -176,6 +176,7 @@ bool tomoyo_read_domain_initializer_poli
>  	struct list_head *pos;
>  	bool done = true;
>  
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_domain_initializer_list_lock);
>  	list_for_each_cookie(pos, head->read_var2,
>  			     &tomoyo_domain_initializer_list) {
> @@ -201,6 +202,7 @@ bool tomoyo_read_domain_initializer_poli
>  		}
>  	}
>  	up_read(&tomoyo_domain_initializer_list_lock);
> +	/***** READER SECTION END *****/
>  	return done;
>  }
>  
> @@ -247,6 +249,7 @@ static bool tomoyo_is_domain_initializer
>  	struct tomoyo_domain_initializer_entry *ptr;
>  	bool flag = false;
>  
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_domain_initializer_list_lock);
>  	list_for_each_entry(ptr,  &tomoyo_domain_initializer_list, list) {
>  		if (ptr->is_deleted)
> @@ -269,6 +272,7 @@ static bool tomoyo_is_domain_initializer
>  		flag = true;
>  	}
>  	up_read(&tomoyo_domain_initializer_list_lock);
> +	/***** READER SECTION END *****/
>  	return flag;
>  }
>  
> @@ -316,7 +320,7 @@ static int tomoyo_update_domain_keeper_e
>  		return -ENOMEM;
>  	if (!is_delete)
>  		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
> -	/***** EXCLUSIVE SECTION START *****/
> +	/***** WRITER SECTION START *****/
>  	down_write(&tomoyo_domain_keeper_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_domain_keeper_list, list) {
>  		if (ptr->is_not != is_not ||
> @@ -337,7 +341,7 @@ static int tomoyo_update_domain_keeper_e
>  		error = 0;
>  	}
>  	up_write(&tomoyo_domain_keeper_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
> +	/***** WRITER SECTION END *****/
>  	kfree(new_entry);
>  	return error;
>  }
> @@ -375,6 +379,7 @@ bool tomoyo_read_domain_keeper_policy(st
>  	struct list_head *pos;
>  	bool done = true;
>  
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_domain_keeper_list_lock);
>  	list_for_each_cookie(pos, head->read_var2,
>  			     &tomoyo_domain_keeper_list) {
> @@ -400,6 +405,7 @@ bool tomoyo_read_domain_keeper_policy(st
>  		}
>  	}
>  	up_read(&tomoyo_domain_keeper_list_lock);
> +	/***** READER SECTION END *****/
>  	return done;
>  }
>  
> @@ -420,6 +426,7 @@ static bool tomoyo_is_domain_keeper(cons
>  	struct tomoyo_domain_keeper_entry *ptr;
>  	bool flag = false;
>  
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_domain_keeper_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_domain_keeper_list, list) {
>  		if (ptr->is_deleted)
> @@ -440,6 +447,7 @@ static bool tomoyo_is_domain_keeper(cons
>  		flag = true;
>  	}
>  	up_read(&tomoyo_domain_keeper_list_lock);
> +	/***** READER SECTION END *****/
>  	return flag;
>  }
>  
> @@ -475,7 +483,7 @@ static int tomoyo_update_alias_entry(con
>  		return -ENOMEM;
>  	if (!is_delete)
>  		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
> -	/***** EXCLUSIVE SECTION START *****/
> +	/***** WRITER SECTION START *****/
>  	down_write(&tomoyo_alias_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_alias_list, list) {
>  		if (ptr->original_name != saved_original_name ||
> @@ -493,7 +501,7 @@ static int tomoyo_update_alias_entry(con
>  		error = 0;
>  	}
>  	up_write(&tomoyo_alias_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
> +	/***** WRITER SECTION END *****/
>  	kfree(new_entry);
>  	return error;
>  }
> @@ -510,6 +518,7 @@ bool tomoyo_read_alias_policy(struct tom
>  	struct list_head *pos;
>  	bool done = true;
>  
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_alias_list_lock);
>  	list_for_each_cookie(pos, head->read_var2, &tomoyo_alias_list) {
>  		struct tomoyo_alias_entry *ptr;
> @@ -525,6 +534,7 @@ bool tomoyo_read_alias_policy(struct tom
>  		}
>  	}
>  	up_read(&tomoyo_alias_list_lock);
> +	/***** READER SECTION END *****/
>  	return done;
>  }
>  
> @@ -562,7 +572,7 @@ int tomoyo_delete_domain(char *domainnam
>  
>  	name.name = domainname;
>  	tomoyo_fill_path_info(&name);
> -	/***** EXCLUSIVE SECTION START *****/
> +	/***** WRITER SECTION START *****/
>  	down_write(&tomoyo_domain_list_lock);
>  	/* Is there an active domain? */
>  	list_for_each_entry(domain, &tomoyo_domain_list, list) {
> @@ -576,7 +586,7 @@ int tomoyo_delete_domain(char *domainnam
>  		break;
>  	}
>  	up_write(&tomoyo_domain_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
> +	/***** WRITER SECTION END *****/
>  	return 0;
>  }
>  
> @@ -602,7 +612,7 @@ struct tomoyo_domain_info *tomoyo_find_o
>  	if (!saved_domainname)
>  		return NULL;
>  	new_domain = kmalloc(sizeof(*new_domain), GFP_KERNEL);
> -	/***** EXCLUSIVE SECTION START *****/
> +	/***** WRITER SECTION START *****/
>  	down_write(&tomoyo_domain_list_lock);
>  	domain = tomoyo_find_domain(domainname);
>  	if (domain)
> @@ -649,7 +659,7 @@ struct tomoyo_domain_info *tomoyo_find_o
>  	}
>   out:
>  	up_write(&tomoyo_domain_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
> +	/***** WRITER SECTION END *****/
>  	kfree(new_domain);
>  	return domain;
>  }
> @@ -722,6 +732,7 @@ int tomoyo_find_next_domain(struct linux
>  	if (tomoyo_pathcmp(&r, &s)) {
>  		struct tomoyo_alias_entry *ptr;
>  		/* Is this program allowed to be called via symbolic links? */
> +		/***** READER SECTION START *****/
>  		down_read(&tomoyo_alias_list_lock);
>  		list_for_each_entry(ptr, &tomoyo_alias_list, list) {
>  			if (ptr->is_deleted ||
> @@ -735,6 +746,7 @@ int tomoyo_find_next_domain(struct linux
>  			break;
>  		}
>  		up_read(&tomoyo_alias_list_lock);
> +		/***** READER SECTION END *****/
>  	}
>  
>  	/* Check execute permission. */
> @@ -765,9 +777,11 @@ int tomoyo_find_next_domain(struct linux
>  	}
>  	if (domain || strlen(new_domain_name) >= TOMOYO_MAX_PATHNAME_LEN)
>  		goto done;
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_domain_list_lock);
>  	domain = tomoyo_find_domain(new_domain_name);
>  	up_read(&tomoyo_domain_list_lock);
> +	/***** READER SECTION END *****/
>  	if (domain)
>  		goto done;
>  	if (is_enforce)
> --- security-testing-2.6.git.orig/security/tomoyo/file.c
> +++ security-testing-2.6.git/security/tomoyo/file.c
> @@ -168,7 +168,7 @@ static int tomoyo_update_globally_readab
>  		return -ENOMEM;
>  	if (!is_delete)
>  		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
> -	/***** EXCLUSIVE SECTION START *****/
> +	/***** WRITER SECTION START *****/
>  	down_write(&tomoyo_globally_readable_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_globally_readable_list, list) {
>  		if (ptr->filename != saved_filename)
> @@ -184,7 +184,7 @@ static int tomoyo_update_globally_readab
>  		error = 0;
>  	}
>  	up_write(&tomoyo_globally_readable_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
> +	/***** WRITER SECTION END *****/
>  	kfree(new_entry);
>  	return error;
>  }
> @@ -201,6 +201,7 @@ static bool tomoyo_is_globally_readable_
>  {
>  	struct tomoyo_globally_readable_file_entry *ptr;
>  	bool found = false;
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_globally_readable_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_globally_readable_list, list) {
>  		if (!ptr->is_deleted &&
> @@ -210,6 +211,7 @@ static bool tomoyo_is_globally_readable_
>  		}
>  	}
>  	up_read(&tomoyo_globally_readable_list_lock);
> +	/***** READER SECTION END *****/
>  	return found;
>  }
>  
> @@ -238,6 +240,7 @@ bool tomoyo_read_globally_readable_polic
>  	struct list_head *pos;
>  	bool done = true;
>  
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_globally_readable_list_lock);
>  	list_for_each_cookie(pos, head->read_var2,
>  			     &tomoyo_globally_readable_list) {
> @@ -254,6 +257,7 @@ bool tomoyo_read_globally_readable_polic
>  		}
>  	}
>  	up_read(&tomoyo_globally_readable_list_lock);
> +	/***** READER SECTION END *****/
>  	return done;
>  }
>  
> @@ -284,7 +288,7 @@ static int tomoyo_update_file_pattern_en
>  		return -ENOMEM;
>  	if (!is_delete)
>  		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
> -	/***** EXCLUSIVE SECTION START *****/
> +	/***** WRITER SECTION START *****/
>  	down_write(&tomoyo_pattern_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_pattern_list, list) {
>  		if (saved_pattern != ptr->pattern)
> @@ -300,7 +304,7 @@ static int tomoyo_update_file_pattern_en
>  		error = 0;
>  	}
>  	up_write(&tomoyo_pattern_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
> +	/***** WRITER SECTION END *****/
>  	kfree(new_entry);
>  	return error;
>  }
> @@ -318,6 +322,7 @@ tomoyo_get_file_pattern(const struct tom
>  	struct tomoyo_pattern_entry *ptr;
>  	const struct tomoyo_path_info *pattern = NULL;
>  
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_pattern_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_pattern_list, list) {
>  		if (ptr->is_deleted)
> @@ -333,6 +338,7 @@ tomoyo_get_file_pattern(const struct tom
>  		}
>  	}
>  	up_read(&tomoyo_pattern_list_lock);
> +	/***** READER SECTION END *****/
>  	if (pattern)
>  		filename = pattern;
>  	return filename;
> @@ -363,6 +369,7 @@ bool tomoyo_read_file_pattern(struct tom
>  	struct list_head *pos;
>  	bool done = true;
>  
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_pattern_list_lock);
>  	list_for_each_cookie(pos, head->read_var2, &tomoyo_pattern_list) {
>  		struct tomoyo_pattern_entry *ptr;
> @@ -376,6 +383,7 @@ bool tomoyo_read_file_pattern(struct tom
>  		}
>  	}
>  	up_read(&tomoyo_pattern_list_lock);
> +	/***** READER SECTION END *****/
>  	return done;
>  }
>  
> @@ -406,7 +414,7 @@ static int tomoyo_update_no_rewrite_entr
>  		return -ENOMEM;
>  	if (!is_delete)
>  		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
> -	/***** EXCLUSIVE SECTION START *****/
> +	/***** WRITER SECTION START *****/
>  	down_write(&tomoyo_no_rewrite_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_no_rewrite_list, list) {
>  		if (ptr->pattern != saved_pattern)
> @@ -422,7 +430,7 @@ static int tomoyo_update_no_rewrite_entr
>  		error = 0;
>  	}
>  	up_write(&tomoyo_no_rewrite_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
> +	/***** WRITER SECTION END *****/
>  	kfree(new_entry);
>  	return error;
>  }
> @@ -440,6 +448,7 @@ static bool tomoyo_is_no_rewrite_file(co
>  	struct tomoyo_no_rewrite_entry *ptr;
>  	bool found = false;
>  
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_no_rewrite_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_no_rewrite_list, list) {
>  		if (ptr->is_deleted)
> @@ -450,6 +459,7 @@ static bool tomoyo_is_no_rewrite_file(co
>  		break;
>  	}
>  	up_read(&tomoyo_no_rewrite_list_lock);
> +	/***** READER SECTION END *****/
>  	return found;
>  }
>  
> @@ -478,6 +488,7 @@ bool tomoyo_read_no_rewrite_policy(struc
>  	struct list_head *pos;
>  	bool done = true;
>  
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_no_rewrite_list_lock);
>  	list_for_each_cookie(pos, head->read_var2, &tomoyo_no_rewrite_list) {
>  		struct tomoyo_no_rewrite_entry *ptr;
> @@ -491,6 +502,7 @@ bool tomoyo_read_no_rewrite_policy(struc
>  		}
>  	}
>  	up_read(&tomoyo_no_rewrite_list_lock);
> +	/***** READER SECTION END *****/
>  	return done;
>  }
>  
> @@ -556,6 +568,7 @@ static int tomoyo_check_single_path_acl2
>  	struct tomoyo_acl_info *ptr;
>  	int error = -EPERM;
>  
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_domain_acl_info_list_lock);
>  	list_for_each_entry(ptr, &domain->acl_info_list, list) {
>  		struct tomoyo_single_path_acl_record *acl;
> @@ -576,6 +589,7 @@ static int tomoyo_check_single_path_acl2
>  		break;
>  	}
>  	up_read(&tomoyo_domain_acl_info_list_lock);
> +	/***** READER SECTION END *****/
>  	return error;
>  }
>  
> @@ -742,7 +756,7 @@ static int tomoyo_update_single_path_acl
>  		return -ENOMEM;
>  	if (!is_delete)
>  		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
> -	/***** EXCLUSIVE SECTION START *****/
> +	/***** WRITER SECTION START *****/
>  	down_write(&tomoyo_domain_acl_info_list_lock);
>  	if (is_delete)
>  		goto delete;
> @@ -799,7 +813,7 @@ static int tomoyo_update_single_path_acl
>  	}
>   out:
>  	up_write(&tomoyo_domain_acl_info_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
> +	/***** WRITER SECTION END *****/
>  	kfree(new_entry);
>  	return error;
>  }
> @@ -838,7 +852,7 @@ static int tomoyo_update_double_path_acl
>  		return -ENOMEM;
>  	if (!is_delete)
>  		new_entry = kmalloc(sizeof(*new_entry), GFP_KERNEL);
> -	/***** EXCLUSIVE SECTION START *****/
> +	/***** WRITER SECTION START *****/
>  	down_write(&tomoyo_domain_acl_info_list_lock);
>  	if (is_delete)
>  		goto delete;
> @@ -888,7 +902,7 @@ static int tomoyo_update_double_path_acl
>  	}
>   out:
>  	up_write(&tomoyo_domain_acl_info_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
> +	/***** WRITER SECTION END *****/
>  	kfree(new_entry);
>  	return error;
>  }
> @@ -934,6 +948,7 @@ static int tomoyo_check_double_path_acl(
>  
>  	if (!tomoyo_check_flags(domain, TOMOYO_MAC_FOR_FILE))
>  		return 0;
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_domain_acl_info_list_lock);
>  	list_for_each_entry(ptr, &domain->acl_info_list, list) {
>  		struct tomoyo_double_path_acl_record *acl;
> @@ -951,6 +966,7 @@ static int tomoyo_check_double_path_acl(
>  		break;
>  	}
>  	up_read(&tomoyo_domain_acl_info_list_lock);
> +	/***** READER SECTION END *****/
>  	return error;
>  }
>  
> --- security-testing-2.6.git.orig/security/tomoyo/realpath.c
> +++ security-testing-2.6.git/security/tomoyo/realpath.c
> @@ -326,10 +326,12 @@ void __init tomoyo_realpath_init(void)
>  	INIT_LIST_HEAD(&tomoyo_kernel_domain.acl_info_list);
>  	tomoyo_kernel_domain.domainname = tomoyo_save_name(TOMOYO_ROOT_NAME);
>  	list_add_tail(&tomoyo_kernel_domain.list, &tomoyo_domain_list);
> +	/***** READER SECTION START *****/
>  	down_read(&tomoyo_domain_list_lock);
>  	if (tomoyo_find_domain(TOMOYO_ROOT_NAME) != &tomoyo_kernel_domain)
>  		panic("Can't register tomoyo_kernel_domain");
>  	up_read(&tomoyo_domain_list_lock);
> +	/***** READER SECTION END *****/
>  }
>  
>  /* Memory allocated for temporary purpose. */

Re: [PATCH 2/5] TOMOYO: Clarify lock protected section.

[Tetsuo Handa]

Li Zefan wrote:
> Tetsuo Handa wrote:
> > Enclose reader section in
> > 	/***** READER SECTION START *****/
> > and
> > 	/***** READER SECTION END *****/
> > and writer section in
> > 	/***** WRITER SECTION START *****/
> > and
> > 	/***** WRITER SECTION END *****/
> > in order to avoid oversighting lock protected section.
> > 
> 
> This makes me a bit uncomfortable..
> 
> IMHO this seems ugly, useless, and even harmful. If it's helpful,
> we'd be doing this for the whole kernel tree, which is crazy..
> 
> Or does tomoyo do this for it's special reason?

I intended to help reviewers to visualize the range of protected section
at a glance. But if reviewers feel noisy, I can remove these markers.

Thanks.

Re: [PATCH 2/5] TOMOYO: Clarify lock protected section.

[Li Zefan]

Tetsuo Handa wrote:
> Li Zefan wrote:
>> Tetsuo Handa wrote:
>>> Enclose reader section in
>>> 	/***** READER SECTION START *****/
>>> and
>>> 	/***** READER SECTION END *****/
>>> and writer section in
>>> 	/***** WRITER SECTION START *****/
>>> and
>>> 	/***** WRITER SECTION END *****/
>>> in order to avoid oversighting lock protected section.
>>>
>> This makes me a bit uncomfortable..
>>
>> IMHO this seems ugly, useless, and even harmful. If it's helpful,
>> we'd be doing this for the whole kernel tree, which is crazy..
>>
>> Or does tomoyo do this for it's special reason?
> 
> I intended to help reviewers to visualize the range of protected section
> at a glance. But if reviewers feel noisy, I can remove these markers.
> 

I don't think it help review, but instead those '*'+UPPERCASE comments
are disturbing.

Re: [PATCH 2/5] TOMOYO: Clarify lock protected section.

[Serge E. Hallyn]

Quoting Tetsuo Handa (penguin-kernel@i-love.sakura.ne.jp):
> Li Zefan wrote:
> > Tetsuo Handa wrote:
> > > Enclose reader section in
> > > 	/***** READER SECTION START *****/
> > > and
> > > 	/***** READER SECTION END *****/
> > > and writer section in
> > > 	/***** WRITER SECTION START *****/
> > > and
> > > 	/***** WRITER SECTION END *****/
> > > in order to avoid oversighting lock protected section.
> > > 
> > 
> > This makes me a bit uncomfortable..
> > 
> > IMHO this seems ugly, useless, and even harmful. If it's helpful,
> > we'd be doing this for the whole kernel tree, which is crazy..
> > 
> > Or does tomoyo do this for it's special reason?
> 
> I intended to help reviewers to visualize the range of protected section
> at a glance. But if reviewers feel noisy, I can remove these markers.

No, the real problem is that you have "protected sections" at all.

You should be locking data, not code.  (Apparently a quote to be attributed
to Alan Cox - huh)

See the bottom of:
http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/x376.html

-serge

Re: [PATCH 2/5] TOMOYO: Clarify lock protected section.

[Tetsuo Handa]

Serge E. Hallyn wrote:
> Again I feel (no offense) like I'm reading Ada code here...
I don't know much about the Linux kernel's way of coding.
I'm making a lot of out-of-conventional coding.
Please mention without hesitating.

> 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.

(1) Declare a variable and a lock for that variable together.

I've heard (1) in the past.

On the other hand, I think there is another rule

(2) Declare variables inside a function if they are used only within
    that function.

If I bring the declaration of the lock to outside the function, it widens
the scope of the lock.

But Linux kernel's way is to follow (1), isn't it?

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

I'm thinking that
(1) If I use mutex and rw_semaphore, the CPU which is waiting for the lock
    can spend it's power for doing other process's jobs.
(2) If I use spinlock, the CPU's power is merely wasted, even though
    the CPU can spend it's power for doing other process's jobs.
and therefore I'm using mutex and rw_semaphore if sleeping is permitted.

Should I use spinlock rather than mutex and rw_semaphore whenever possible?

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

Thanks.

Re: [PATCH 2/5] TOMOYO: Clarify lock protected section.

[James Morris]

On Tue, 2 Jun 2009, Tetsuo Handa wrote:

> I intended to help reviewers to visualize the range of protected section
> at a glance. But if reviewers feel noisy, I can remove these markers.

The lock functions act as annotations, you don't need to add any more.  

Only add comments if the code is not obvious, but also first ask whether 
this might mean that there's something wrong.

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 2/5] TOMOYO: Clarify lock protected section.

[Tetsuo Handa]

James Morris wrote:
> The lock functions act as annotations, you don't need to add any more.  

I see. I'll remake patchset.

Thanks.
--------------------
Subject: TOMOYO: Remove redundant markers.

Remove '/***** START/STOP *****/' markers.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/tomoyo/common.c   |    8 --------
 security/tomoyo/domain.c   |   14 --------------
 security/tomoyo/file.c     |   10 ----------
 security/tomoyo/realpath.c |    4 ----
 4 files changed, 36 deletions(-)

--- security-testing-2.6.git.orig/security/tomoyo/common.c
+++ security-testing-2.6.git/security/tomoyo/common.c
@@ -866,7 +866,6 @@ static struct tomoyo_profile *tomoyo_fin
 
 	if (profile >= TOMOYO_MAX_PROFILES)
 		return NULL;
-	/***** EXCLUSIVE SECTION START *****/
 	mutex_lock(&lock);
 	ptr = tomoyo_profile_ptr[profile];
 	if (ptr)
@@ -880,7 +879,6 @@ static struct tomoyo_profile *tomoyo_fin
 	tomoyo_profile_ptr[profile] = ptr;
  ok:
 	mutex_unlock(&lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return ptr;
 }
 
@@ -1050,7 +1048,6 @@ static int tomoyo_update_manager_entry(c
 	saved_manager = tomoyo_save_name(manager);
 	if (!saved_manager)
 		return -ENOMEM;
-	/***** EXCLUSIVE SECTION START *****/
 	down_write(&tomoyo_policy_manager_list_lock);
 	list_for_each_entry(ptr, &tomoyo_policy_manager_list, list) {
 		if (ptr->manager != saved_manager)
@@ -1072,7 +1069,6 @@ static int tomoyo_update_manager_entry(c
 	error = 0;
  out:
 	up_write(&tomoyo_policy_manager_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return error;
 }
 
@@ -1197,13 +1193,11 @@ static bool tomoyo_is_select_one(struct 
 
 	if (sscanf(data, "pid=%u", &pid) == 1) {
 		struct task_struct *p;
-		/***** CRITICAL SECTION START *****/
 		read_lock(&tasklist_lock);
 		p = find_task_by_vpid(pid);
 		if (p)
 			domain = tomoyo_real_domain(p);
 		read_unlock(&tasklist_lock);
-		/***** CRITICAL SECTION END *****/
 	} else if (!strncmp(data, "domain=", 7)) {
 		if (tomoyo_is_domain_def(data + 7)) {
 			down_read(&tomoyo_domain_list_lock);
@@ -1594,13 +1588,11 @@ static int tomoyo_read_pid(struct tomoyo
 		const int pid = head->read_step;
 		struct task_struct *p;
 		struct tomoyo_domain_info *domain = NULL;
-		/***** CRITICAL SECTION START *****/
 		read_lock(&tasklist_lock);
 		p = find_task_by_vpid(pid);
 		if (p)
 			domain = tomoyo_real_domain(p);
 		read_unlock(&tasklist_lock);
-		/***** CRITICAL SECTION END *****/
 		if (domain)
 			tomoyo_io_printf(head, "%d %u %s", pid, domain->profile,
 					 domain->domainname->name);
--- security-testing-2.6.git.orig/security/tomoyo/domain.c
+++ security-testing-2.6.git/security/tomoyo/domain.c
@@ -67,14 +67,12 @@ void tomoyo_set_domain_flag(struct tomoy
 {
 	/* We need to serialize because this is bitfield operation. */
 	static DEFINE_SPINLOCK(lock);
-	/***** CRITICAL SECTION START *****/
 	spin_lock(&lock);
 	if (!is_delete)
 		domain->flags |= flags;
 	else
 		domain->flags &= ~flags;
 	spin_unlock(&lock);
-	/***** CRITICAL SECTION END *****/
 }
 
 /**
@@ -135,7 +133,6 @@ static int tomoyo_update_domain_initiali
 	saved_program = tomoyo_save_name(program);
 	if (!saved_program)
 		return -ENOMEM;
-	/***** EXCLUSIVE SECTION START *****/
 	down_write(&tomoyo_domain_initializer_list_lock);
 	list_for_each_entry(ptr, &tomoyo_domain_initializer_list, list) {
 		if (ptr->is_not != is_not ||
@@ -161,7 +158,6 @@ static int tomoyo_update_domain_initiali
 	error = 0;
  out:
 	up_write(&tomoyo_domain_initializer_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return error;
 }
 
@@ -315,7 +311,6 @@ static int tomoyo_update_domain_keeper_e
 	saved_domainname = tomoyo_save_name(domainname);
 	if (!saved_domainname)
 		return -ENOMEM;
-	/***** EXCLUSIVE SECTION START *****/
 	down_write(&tomoyo_domain_keeper_list_lock);
 	list_for_each_entry(ptr, &tomoyo_domain_keeper_list, list) {
 		if (ptr->is_not != is_not ||
@@ -341,7 +336,6 @@ static int tomoyo_update_domain_keeper_e
 	error = 0;
  out:
 	up_write(&tomoyo_domain_keeper_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return error;
 }
 
@@ -476,7 +470,6 @@ static int tomoyo_update_alias_entry(con
 	saved_aliased_name = tomoyo_save_name(aliased_name);
 	if (!saved_original_name || !saved_aliased_name)
 		return -ENOMEM;
-	/***** EXCLUSIVE SECTION START *****/
 	down_write(&tomoyo_alias_list_lock);
 	list_for_each_entry(ptr, &tomoyo_alias_list, list) {
 		if (ptr->original_name != saved_original_name ||
@@ -499,7 +492,6 @@ static int tomoyo_update_alias_entry(con
 	error = 0;
  out:
 	up_write(&tomoyo_alias_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return error;
 }
 
@@ -567,7 +559,6 @@ int tomoyo_delete_domain(char *domainnam
 
 	name.name = domainname;
 	tomoyo_fill_path_info(&name);
-	/***** EXCLUSIVE SECTION START *****/
 	down_write(&tomoyo_domain_list_lock);
 	/* Is there an active domain? */
 	list_for_each_entry(domain, &tomoyo_domain_list, list) {
@@ -581,7 +572,6 @@ int tomoyo_delete_domain(char *domainnam
 		break;
 	}
 	up_write(&tomoyo_domain_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return 0;
 }
 
@@ -600,7 +590,6 @@ struct tomoyo_domain_info *tomoyo_find_o
 	struct tomoyo_domain_info *domain = NULL;
 	const struct tomoyo_path_info *saved_domainname;
 
-	/***** EXCLUSIVE SECTION START *****/
 	down_write(&tomoyo_domain_list_lock);
 	domain = tomoyo_find_domain(domainname);
 	if (domain)
@@ -619,7 +608,6 @@ struct tomoyo_domain_info *tomoyo_find_o
 		    domain->domainname != saved_domainname)
 			continue;
 		flag = false;
-		/***** CRITICAL SECTION START *****/
 		read_lock(&tasklist_lock);
 		for_each_process(p) {
 			if (tomoyo_real_domain(p) != domain)
@@ -628,7 +616,6 @@ struct tomoyo_domain_info *tomoyo_find_o
 			break;
 		}
 		read_unlock(&tasklist_lock);
-		/***** CRITICAL SECTION END *****/
 		if (flag)
 			continue;
 		list_for_each_entry(ptr, &domain->acl_info_list, list) {
@@ -651,7 +638,6 @@ struct tomoyo_domain_info *tomoyo_find_o
 	}
  out:
 	up_write(&tomoyo_domain_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return domain;
 }
 
--- security-testing-2.6.git.orig/security/tomoyo/file.c
+++ security-testing-2.6.git/security/tomoyo/file.c
@@ -166,7 +166,6 @@ static int tomoyo_update_globally_readab
 	saved_filename = tomoyo_save_name(filename);
 	if (!saved_filename)
 		return -ENOMEM;
-	/***** EXCLUSIVE SECTION START *****/
 	down_write(&tomoyo_globally_readable_list_lock);
 	list_for_each_entry(ptr, &tomoyo_globally_readable_list, list) {
 		if (ptr->filename != saved_filename)
@@ -187,7 +186,6 @@ static int tomoyo_update_globally_readab
 	error = 0;
  out:
 	up_write(&tomoyo_globally_readable_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return error;
 }
 
@@ -284,7 +282,6 @@ static int tomoyo_update_file_pattern_en
 	saved_pattern = tomoyo_save_name(pattern);
 	if (!saved_pattern)
 		return -ENOMEM;
-	/***** EXCLUSIVE SECTION START *****/
 	down_write(&tomoyo_pattern_list_lock);
 	list_for_each_entry(ptr, &tomoyo_pattern_list, list) {
 		if (saved_pattern != ptr->pattern)
@@ -305,7 +302,6 @@ static int tomoyo_update_file_pattern_en
 	error = 0;
  out:
 	up_write(&tomoyo_pattern_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return error;
 }
 
@@ -407,7 +403,6 @@ static int tomoyo_update_no_rewrite_entr
 	saved_pattern = tomoyo_save_name(pattern);
 	if (!saved_pattern)
 		return -ENOMEM;
-	/***** EXCLUSIVE SECTION START *****/
 	down_write(&tomoyo_no_rewrite_list_lock);
 	list_for_each_entry(ptr, &tomoyo_no_rewrite_list, list) {
 		if (ptr->pattern != saved_pattern)
@@ -428,7 +423,6 @@ static int tomoyo_update_no_rewrite_entr
 	error = 0;
  out:
 	up_write(&tomoyo_no_rewrite_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return error;
 }
 
@@ -745,7 +739,6 @@ static int tomoyo_update_single_path_acl
 	saved_filename = tomoyo_save_name(filename);
 	if (!saved_filename)
 		return -ENOMEM;
-	/***** EXCLUSIVE SECTION START *****/
 	down_write(&tomoyo_domain_acl_info_list_lock);
 	if (is_delete)
 		goto delete;
@@ -800,7 +793,6 @@ static int tomoyo_update_single_path_acl
 	}
  out:
 	up_write(&tomoyo_domain_acl_info_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return error;
 }
 
@@ -836,7 +828,6 @@ static int tomoyo_update_double_path_acl
 	saved_filename2 = tomoyo_save_name(filename2);
 	if (!saved_filename1 || !saved_filename2)
 		return -ENOMEM;
-	/***** EXCLUSIVE SECTION START *****/
 	down_write(&tomoyo_domain_acl_info_list_lock);
 	if (is_delete)
 		goto delete;
@@ -884,7 +875,6 @@ static int tomoyo_update_double_path_acl
 	}
  out:
 	up_write(&tomoyo_domain_acl_info_list_lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return error;
 }
 
--- security-testing-2.6.git.orig/security/tomoyo/realpath.c
+++ security-testing-2.6.git/security/tomoyo/realpath.c
@@ -220,7 +220,6 @@ void *tomoyo_alloc_element(const unsigne
 		= roundup(size, max(sizeof(void *), sizeof(long)));
 	if (word_aligned_size > PATH_MAX)
 		return NULL;
-	/***** EXCLUSIVE SECTION START *****/
 	mutex_lock(&lock);
 	if (buf_used_len + word_aligned_size > PATH_MAX) {
 		if (!tomoyo_quota_for_elements ||
@@ -251,7 +250,6 @@ void *tomoyo_alloc_element(const unsigne
 		}
 	}
 	mutex_unlock(&lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return ptr;
 }
 
@@ -318,7 +316,6 @@ const struct tomoyo_path_info *tomoyo_sa
 		return NULL;
 	}
 	hash = full_name_hash((const unsigned char *) name, len - 1);
-	/***** EXCLUSIVE SECTION START *****/
 	mutex_lock(&lock);
 	list_for_each_entry(ptr, &tomoyo_name_list[hash % TOMOYO_MAX_HASH],
 			     list) {
@@ -366,7 +363,6 @@ const struct tomoyo_path_info *tomoyo_sa
 	}
  out:
 	mutex_unlock(&lock);
-	/***** EXCLUSIVE SECTION END *****/
 	return ptr ? &ptr->entry : NULL;
 }
 

Re: [PATCH 2/5] TOMOYO: Clarify lock protected section.

[James Morris]

On Tue, 2 Jun 2009, Tetsuo Handa wrote:

> James Morris wrote:
> > The lock functions act as annotations, you don't need to add any more.  
> 
> I see. I'll remake patchset.
> 
> Thanks.

Please send new or updates patches as new self-contained emails.

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next



> --------------------
> Subject: TOMOYO: Remove redundant markers.
> 
> Remove '/***** START/STOP *****/' markers.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  security/tomoyo/common.c   |    8 --------
>  security/tomoyo/domain.c   |   14 --------------
>  security/tomoyo/file.c     |   10 ----------
>  security/tomoyo/realpath.c |    4 ----
>  4 files changed, 36 deletions(-)
> 
> --- security-testing-2.6.git.orig/security/tomoyo/common.c
> +++ security-testing-2.6.git/security/tomoyo/common.c
> @@ -866,7 +866,6 @@ static struct tomoyo_profile *tomoyo_fin
>  
>  	if (profile >= TOMOYO_MAX_PROFILES)
>  		return NULL;
> -	/***** EXCLUSIVE SECTION START *****/
>  	mutex_lock(&lock);
>  	ptr = tomoyo_profile_ptr[profile];
>  	if (ptr)
> @@ -880,7 +879,6 @@ static struct tomoyo_profile *tomoyo_fin
>  	tomoyo_profile_ptr[profile] = ptr;
>   ok:
>  	mutex_unlock(&lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return ptr;
>  }
>  
> @@ -1050,7 +1048,6 @@ static int tomoyo_update_manager_entry(c
>  	saved_manager = tomoyo_save_name(manager);
>  	if (!saved_manager)
>  		return -ENOMEM;
> -	/***** EXCLUSIVE SECTION START *****/
>  	down_write(&tomoyo_policy_manager_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_policy_manager_list, list) {
>  		if (ptr->manager != saved_manager)
> @@ -1072,7 +1069,6 @@ static int tomoyo_update_manager_entry(c
>  	error = 0;
>   out:
>  	up_write(&tomoyo_policy_manager_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return error;
>  }
>  
> @@ -1197,13 +1193,11 @@ static bool tomoyo_is_select_one(struct 
>  
>  	if (sscanf(data, "pid=%u", &pid) == 1) {
>  		struct task_struct *p;
> -		/***** CRITICAL SECTION START *****/
>  		read_lock(&tasklist_lock);
>  		p = find_task_by_vpid(pid);
>  		if (p)
>  			domain = tomoyo_real_domain(p);
>  		read_unlock(&tasklist_lock);
> -		/***** CRITICAL SECTION END *****/
>  	} else if (!strncmp(data, "domain=", 7)) {
>  		if (tomoyo_is_domain_def(data + 7)) {
>  			down_read(&tomoyo_domain_list_lock);
> @@ -1594,13 +1588,11 @@ static int tomoyo_read_pid(struct tomoyo
>  		const int pid = head->read_step;
>  		struct task_struct *p;
>  		struct tomoyo_domain_info *domain = NULL;
> -		/***** CRITICAL SECTION START *****/
>  		read_lock(&tasklist_lock);
>  		p = find_task_by_vpid(pid);
>  		if (p)
>  			domain = tomoyo_real_domain(p);
>  		read_unlock(&tasklist_lock);
> -		/***** CRITICAL SECTION END *****/
>  		if (domain)
>  			tomoyo_io_printf(head, "%d %u %s", pid, domain->profile,
>  					 domain->domainname->name);
> --- security-testing-2.6.git.orig/security/tomoyo/domain.c
> +++ security-testing-2.6.git/security/tomoyo/domain.c
> @@ -67,14 +67,12 @@ void tomoyo_set_domain_flag(struct tomoy
>  {
>  	/* We need to serialize because this is bitfield operation. */
>  	static DEFINE_SPINLOCK(lock);
> -	/***** CRITICAL SECTION START *****/
>  	spin_lock(&lock);
>  	if (!is_delete)
>  		domain->flags |= flags;
>  	else
>  		domain->flags &= ~flags;
>  	spin_unlock(&lock);
> -	/***** CRITICAL SECTION END *****/
>  }
>  
>  /**
> @@ -135,7 +133,6 @@ static int tomoyo_update_domain_initiali
>  	saved_program = tomoyo_save_name(program);
>  	if (!saved_program)
>  		return -ENOMEM;
> -	/***** EXCLUSIVE SECTION START *****/
>  	down_write(&tomoyo_domain_initializer_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_domain_initializer_list, list) {
>  		if (ptr->is_not != is_not ||
> @@ -161,7 +158,6 @@ static int tomoyo_update_domain_initiali
>  	error = 0;
>   out:
>  	up_write(&tomoyo_domain_initializer_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return error;
>  }
>  
> @@ -315,7 +311,6 @@ static int tomoyo_update_domain_keeper_e
>  	saved_domainname = tomoyo_save_name(domainname);
>  	if (!saved_domainname)
>  		return -ENOMEM;
> -	/***** EXCLUSIVE SECTION START *****/
>  	down_write(&tomoyo_domain_keeper_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_domain_keeper_list, list) {
>  		if (ptr->is_not != is_not ||
> @@ -341,7 +336,6 @@ static int tomoyo_update_domain_keeper_e
>  	error = 0;
>   out:
>  	up_write(&tomoyo_domain_keeper_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return error;
>  }
>  
> @@ -476,7 +470,6 @@ static int tomoyo_update_alias_entry(con
>  	saved_aliased_name = tomoyo_save_name(aliased_name);
>  	if (!saved_original_name || !saved_aliased_name)
>  		return -ENOMEM;
> -	/***** EXCLUSIVE SECTION START *****/
>  	down_write(&tomoyo_alias_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_alias_list, list) {
>  		if (ptr->original_name != saved_original_name ||
> @@ -499,7 +492,6 @@ static int tomoyo_update_alias_entry(con
>  	error = 0;
>   out:
>  	up_write(&tomoyo_alias_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return error;
>  }
>  
> @@ -567,7 +559,6 @@ int tomoyo_delete_domain(char *domainnam
>  
>  	name.name = domainname;
>  	tomoyo_fill_path_info(&name);
> -	/***** EXCLUSIVE SECTION START *****/
>  	down_write(&tomoyo_domain_list_lock);
>  	/* Is there an active domain? */
>  	list_for_each_entry(domain, &tomoyo_domain_list, list) {
> @@ -581,7 +572,6 @@ int tomoyo_delete_domain(char *domainnam
>  		break;
>  	}
>  	up_write(&tomoyo_domain_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return 0;
>  }
>  
> @@ -600,7 +590,6 @@ struct tomoyo_domain_info *tomoyo_find_o
>  	struct tomoyo_domain_info *domain = NULL;
>  	const struct tomoyo_path_info *saved_domainname;
>  
> -	/***** EXCLUSIVE SECTION START *****/
>  	down_write(&tomoyo_domain_list_lock);
>  	domain = tomoyo_find_domain(domainname);
>  	if (domain)
> @@ -619,7 +608,6 @@ struct tomoyo_domain_info *tomoyo_find_o
>  		    domain->domainname != saved_domainname)
>  			continue;
>  		flag = false;
> -		/***** CRITICAL SECTION START *****/
>  		read_lock(&tasklist_lock);
>  		for_each_process(p) {
>  			if (tomoyo_real_domain(p) != domain)
> @@ -628,7 +616,6 @@ struct tomoyo_domain_info *tomoyo_find_o
>  			break;
>  		}
>  		read_unlock(&tasklist_lock);
> -		/***** CRITICAL SECTION END *****/
>  		if (flag)
>  			continue;
>  		list_for_each_entry(ptr, &domain->acl_info_list, list) {
> @@ -651,7 +638,6 @@ struct tomoyo_domain_info *tomoyo_find_o
>  	}
>   out:
>  	up_write(&tomoyo_domain_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return domain;
>  }
>  
> --- security-testing-2.6.git.orig/security/tomoyo/file.c
> +++ security-testing-2.6.git/security/tomoyo/file.c
> @@ -166,7 +166,6 @@ static int tomoyo_update_globally_readab
>  	saved_filename = tomoyo_save_name(filename);
>  	if (!saved_filename)
>  		return -ENOMEM;
> -	/***** EXCLUSIVE SECTION START *****/
>  	down_write(&tomoyo_globally_readable_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_globally_readable_list, list) {
>  		if (ptr->filename != saved_filename)
> @@ -187,7 +186,6 @@ static int tomoyo_update_globally_readab
>  	error = 0;
>   out:
>  	up_write(&tomoyo_globally_readable_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return error;
>  }
>  
> @@ -284,7 +282,6 @@ static int tomoyo_update_file_pattern_en
>  	saved_pattern = tomoyo_save_name(pattern);
>  	if (!saved_pattern)
>  		return -ENOMEM;
> -	/***** EXCLUSIVE SECTION START *****/
>  	down_write(&tomoyo_pattern_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_pattern_list, list) {
>  		if (saved_pattern != ptr->pattern)
> @@ -305,7 +302,6 @@ static int tomoyo_update_file_pattern_en
>  	error = 0;
>   out:
>  	up_write(&tomoyo_pattern_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return error;
>  }
>  
> @@ -407,7 +403,6 @@ static int tomoyo_update_no_rewrite_entr
>  	saved_pattern = tomoyo_save_name(pattern);
>  	if (!saved_pattern)
>  		return -ENOMEM;
> -	/***** EXCLUSIVE SECTION START *****/
>  	down_write(&tomoyo_no_rewrite_list_lock);
>  	list_for_each_entry(ptr, &tomoyo_no_rewrite_list, list) {
>  		if (ptr->pattern != saved_pattern)
> @@ -428,7 +423,6 @@ static int tomoyo_update_no_rewrite_entr
>  	error = 0;
>   out:
>  	up_write(&tomoyo_no_rewrite_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return error;
>  }
>  
> @@ -745,7 +739,6 @@ static int tomoyo_update_single_path_acl
>  	saved_filename = tomoyo_save_name(filename);
>  	if (!saved_filename)
>  		return -ENOMEM;
> -	/***** EXCLUSIVE SECTION START *****/
>  	down_write(&tomoyo_domain_acl_info_list_lock);
>  	if (is_delete)
>  		goto delete;
> @@ -800,7 +793,6 @@ static int tomoyo_update_single_path_acl
>  	}
>   out:
>  	up_write(&tomoyo_domain_acl_info_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return error;
>  }
>  
> @@ -836,7 +828,6 @@ static int tomoyo_update_double_path_acl
>  	saved_filename2 = tomoyo_save_name(filename2);
>  	if (!saved_filename1 || !saved_filename2)
>  		return -ENOMEM;
> -	/***** EXCLUSIVE SECTION START *****/
>  	down_write(&tomoyo_domain_acl_info_list_lock);
>  	if (is_delete)
>  		goto delete;
> @@ -884,7 +875,6 @@ static int tomoyo_update_double_path_acl
>  	}
>   out:
>  	up_write(&tomoyo_domain_acl_info_list_lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return error;
>  }
>  
> --- security-testing-2.6.git.orig/security/tomoyo/realpath.c
> +++ security-testing-2.6.git/security/tomoyo/realpath.c
> @@ -220,7 +220,6 @@ void *tomoyo_alloc_element(const unsigne
>  		= roundup(size, max(sizeof(void *), sizeof(long)));
>  	if (word_aligned_size > PATH_MAX)
>  		return NULL;
> -	/***** EXCLUSIVE SECTION START *****/
>  	mutex_lock(&lock);
>  	if (buf_used_len + word_aligned_size > PATH_MAX) {
>  		if (!tomoyo_quota_for_elements ||
> @@ -251,7 +250,6 @@ void *tomoyo_alloc_element(const unsigne
>  		}
>  	}
>  	mutex_unlock(&lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return ptr;
>  }
>  
> @@ -318,7 +316,6 @@ const struct tomoyo_path_info *tomoyo_sa
>  		return NULL;
>  	}
>  	hash = full_name_hash((const unsigned char *) name, len - 1);
> -	/***** EXCLUSIVE SECTION START *****/
>  	mutex_lock(&lock);
>  	list_for_each_entry(ptr, &tomoyo_name_list[hash % TOMOYO_MAX_HASH],
>  			     list) {
> @@ -366,7 +363,6 @@ const struct tomoyo_path_info *tomoyo_sa
>  	}
>   out:
>  	mutex_unlock(&lock);
> -	/***** EXCLUSIVE SECTION END *****/
>  	return ptr ? &ptr->entry : NULL;
>  }
>  
> 

-- 
James Morris
<jmorris@namei.org>