Suspicious RCU usage in tomoyo code
John Garry
john.garry at huawei.com
Mon Dec 16 13:13:52 UTC 2019
+
On 16/12/2019 10:30, Tetsuo Handa wrote:
> On 2019/12/16 18:20, John Garry wrote:
>> On 16/12/2019 01:04, Paul E. McKenney wrote:
>>>> Thank you for reporting. I was surprised that this warning did not show up.
>>>> Something has changed or only some config combination could trigger it ...
>>> Any particular reason we are having this discussion privately?;-)
>>>
>>
>> I did mention this initially - I didn't know if reporting issues in "security" domain is generally always open. Probably being very paranoid or silly of me...
>
> Since this is not a security problem, you can post to public lists.
> Anyway, here is a patch. Will you try?
>
> From 8356e05a5822ffad5d374c992bc6af26ea655d6d Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> Date: Mon, 16 Dec 2019 19:16:48 +0900
> Subject: [PATCH] tomoyo: Add tomoyo: Suppress RCU warning at list_for_each_entry_rcu().
>
> John Garry has reported that allmodconfig kernel on arm64 causes flood of
> "RCU-list traversed in non-reader section!!" warning. I don't know what
> change caused this warning, but this warning is safe because TOMOYO uses
> SRCU lock instead. Let's suppress this warning by explicitly telling that
> the caller is holding SRCU lock.
>
> Reported-by: John Garry <john.garry at huawei.com>
Yeah, this looks to have fixed it:
Tested-by: John Garry <john.garry at huawei.com>
Thanks
> Signed-off-by: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> ---
> security/tomoyo/common.c | 9 ++++++---
> security/tomoyo/domain.c | 15 ++++++++++-----
> security/tomoyo/group.c | 9 ++++++---
> security/tomoyo/util.c | 6 ++++--
> 4 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
> index dd3d5942e669..c36bafbcd77e 100644
> --- a/security/tomoyo/common.c
> +++ b/security/tomoyo/common.c
> @@ -951,7 +951,8 @@ static bool tomoyo_manager(void)
> exe = tomoyo_get_exe();
> if (!exe)
> return false;
> - list_for_each_entry_rcu(ptr, &tomoyo_kernel_namespace.policy_list[TOMOYO_ID_MANAGER], head.list) {
> + list_for_each_entry_rcu(ptr, &tomoyo_kernel_namespace.policy_list[TOMOYO_ID_MANAGER], head.list,
> + srcu_read_lock_held(&tomoyo_ss)) {
> if (!ptr->head.is_deleted &&
> (!tomoyo_pathcmp(domainname, ptr->manager) ||
> !strcmp(exe, ptr->manager->name))) {
> @@ -1095,7 +1096,8 @@ static int tomoyo_delete_domain(char *domainname)
> if (mutex_lock_interruptible(&tomoyo_policy_lock))
> return -EINTR;
> /* Is there an active domain? */
> - list_for_each_entry_rcu(domain, &tomoyo_domain_list, list) {
> + list_for_each_entry_rcu(domain, &tomoyo_domain_list, list,
> + srcu_read_lock_held(&tomoyo_ss)) {
> /* Never delete tomoyo_kernel_domain */
> if (domain == &tomoyo_kernel_domain)
> continue;
> @@ -2778,7 +2780,8 @@ void tomoyo_check_profile(void)
>
> tomoyo_policy_loaded = true;
> pr_info("TOMOYO: 2.6.0\n");
> - list_for_each_entry_rcu(domain, &tomoyo_domain_list, list) {
> + list_for_each_entry_rcu(domain, &tomoyo_domain_list, list,
> + srcu_read_lock_held(&tomoyo_ss)) {
> const u8 profile = domain->profile;
> struct tomoyo_policy_namespace *ns = domain->ns;
>
> diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
> index 8526a0a74023..7869d6a9980b 100644
> --- a/security/tomoyo/domain.c
> +++ b/security/tomoyo/domain.c
> @@ -41,7 +41,8 @@ int tomoyo_update_policy(struct tomoyo_acl_head *new_entry, const int size,
>
> if (mutex_lock_interruptible(&tomoyo_policy_lock))
> return -ENOMEM;
> - list_for_each_entry_rcu(entry, list, list) {
> + list_for_each_entry_rcu(entry, list, list,
> + srcu_read_lock_held(&tomoyo_ss)) {
> if (entry->is_deleted == TOMOYO_GC_IN_PROGRESS)
> continue;
> if (!check_duplicate(entry, new_entry))
> @@ -119,7 +120,8 @@ int tomoyo_update_domain(struct tomoyo_acl_info *new_entry, const int size,
> }
> if (mutex_lock_interruptible(&tomoyo_policy_lock))
> goto out;
> - list_for_each_entry_rcu(entry, list, list) {
> + list_for_each_entry_rcu(entry, list, list,
> + srcu_read_lock_held(&tomoyo_ss)) {
> if (entry->is_deleted == TOMOYO_GC_IN_PROGRESS)
> continue;
> if (!tomoyo_same_acl_head(entry, new_entry) ||
> @@ -166,7 +168,8 @@ void tomoyo_check_acl(struct tomoyo_request_info *r,
> u16 i = 0;
>
> retry:
> - list_for_each_entry_rcu(ptr, list, list) {
> + list_for_each_entry_rcu(ptr, list, list,
> + srcu_read_lock_held(&tomoyo_ss)) {
> if (ptr->is_deleted || ptr->type != r->param_type)
> continue;
> if (!check_entry(r, ptr))
> @@ -298,7 +301,8 @@ static inline bool tomoyo_scan_transition
> {
> const struct tomoyo_transition_control *ptr;
>
> - list_for_each_entry_rcu(ptr, list, head.list) {
> + list_for_each_entry_rcu(ptr, list, head.list,
> + srcu_read_lock_held(&tomoyo_ss)) {
> if (ptr->head.is_deleted || ptr->type != type)
> continue;
> if (ptr->domainname) {
> @@ -735,7 +739,8 @@ int tomoyo_find_next_domain(struct linux_binprm *bprm)
>
> /* Check 'aggregator' directive. */
> candidate = &exename;
> - list_for_each_entry_rcu(ptr, list, head.list) {
> + list_for_each_entry_rcu(ptr, list, head.list,
> + srcu_read_lock_held(&tomoyo_ss)) {
> if (ptr->head.is_deleted ||
> !tomoyo_path_matches_pattern(&exename,
> ptr->original_name))
> diff --git a/security/tomoyo/group.c b/security/tomoyo/group.c
> index a37c7dc66e44..1cecdd797597 100644
> --- a/security/tomoyo/group.c
> +++ b/security/tomoyo/group.c
> @@ -133,7 +133,8 @@ tomoyo_path_matches_group(const struct tomoyo_path_info *pathname,
> {
> struct tomoyo_path_group *member;
>
> - list_for_each_entry_rcu(member, &group->member_list, head.list) {
> + list_for_each_entry_rcu(member, &group->member_list, head.list,
> + srcu_read_lock_held(&tomoyo_ss)) {
> if (member->head.is_deleted)
> continue;
> if (!tomoyo_path_matches_pattern(pathname, member->member_name))
> @@ -161,7 +162,8 @@ bool tomoyo_number_matches_group(const unsigned long min,
> struct tomoyo_number_group *member;
> bool matched = false;
>
> - list_for_each_entry_rcu(member, &group->member_list, head.list) {
> + list_for_each_entry_rcu(member, &group->member_list, head.list,
> + srcu_read_lock_held(&tomoyo_ss)) {
> if (member->head.is_deleted)
> continue;
> if (min > member->number.values[1] ||
> @@ -191,7 +193,8 @@ bool tomoyo_address_matches_group(const bool is_ipv6, const __be32 *address,
> bool matched = false;
> const u8 size = is_ipv6 ? 16 : 4;
>
> - list_for_each_entry_rcu(member, &group->member_list, head.list) {
> + list_for_each_entry_rcu(member, &group->member_list, head.list,
> + srcu_read_lock_held(&tomoyo_ss)) {
> if (member->head.is_deleted)
> continue;
> if (member->address.is_ipv6 != is_ipv6)
> diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
> index 52752e1a84ed..eba0b3395851 100644
> --- a/security/tomoyo/util.c
> +++ b/security/tomoyo/util.c
> @@ -594,7 +594,8 @@ struct tomoyo_domain_info *tomoyo_find_domain(const char *domainname)
>
> name.name = domainname;
> tomoyo_fill_path_info(&name);
> - list_for_each_entry_rcu(domain, &tomoyo_domain_list, list) {
> + list_for_each_entry_rcu(domain, &tomoyo_domain_list, list,
> + srcu_read_lock_held(&tomoyo_ss)) {
> if (!domain->is_deleted &&
> !tomoyo_pathcmp(&name, domain->domainname))
> return domain;
> @@ -1028,7 +1029,8 @@ bool tomoyo_domain_quota_is_ok(struct tomoyo_request_info *r)
> return false;
> if (!domain)
> return true;
> - list_for_each_entry_rcu(ptr, &domain->acl_info_list, list) {
> + list_for_each_entry_rcu(ptr, &domain->acl_info_list, list,
> + srcu_read_lock_held(&tomoyo_ss)) {
> u16 perm;
> u8 i;
>
>
More information about the Linux-security-module-archive
mailing list