Messages in this thread |  | | Subject | Re: Suspicious RCU usage in tomoyo code | From | John Garry <> | Date | Mon, 16 Dec 2019 13:13:52 +0000 |
| |
+
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@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@huawei.com>
Yeah, this looks to have fixed it:
Tested-by: John Garry <john.garry@huawei.com>
Thanks
> Signed-off-by: Tetsuo Handa <penguin-kernel@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; > >
|  |