Messages in this thread | | | From | THOBY Simon <> | Subject | Re: [PATCH] ima: fix deadlock within RCU list of ima_rules | Date | Fri, 27 Aug 2021 07:30:55 +0000 |
| |
Hi liqiong,
a few nits but nothing significant. This is getting in good shape!
On 8/27/21 8:41 AM, liqiong wrote: > Hi Simon, > > Thanks for you help, i may got it, here is the new commit message: > > > The current IMA ruleset is identified by the variable "ima_rules" > that default to "&ima_default_rules". When loading a custom policy > for the first time, the variable is updated to "&ima_policy_rules" > instead. That update isn't RCU-safe, and deadlocks are possible. > Because some functions like ima_match_policy() may loop indefinitely
s/Because/Indeed,/ (in english, sentences with a subordinating conjunction like 'because' are usually written in two parts, and a dependent clause standing by itself is rarely used except for stylistic effect)
> over traversing the "ima_default_rules" as list_for_each_entry_rcu().
s/over traversing the "ima_default_rules" as list_for_each_entry_rcu()/when traversing "ima_default_rules" with list_for_each_entry_rcu()./
> > When iterating over the default ruleset back to head, value of > "&entry->list" is "&ima_default_rules", and "ima_rules" may have been
s/value of "&entry->list" is "&ima_default_rules"/if the list head is "ima_default_rules"/ s/may have been/have been/
> updated to "&ima_policy_rules", the loop condition (&entry->list != ima_rules) > stay alway true, traversing doesn't terminate, cause soft lockup and
Don't forget the 's' in "stays" (or "remains") Ditto for "always" s/traversing doesn't/traversing won't/ Also: s/cause/causing a/
> RCU stalls. > > Introduce a temporary value for "ima_rules" when iterating over > the ruleset to avoid the deadlocks. > > > Signed-off-by: liqiong <liqiong@nfschina.com> > --- > security/integrity/ima/ima_policy.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index fd5d46e511f1..e92b197bfd3c 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > > > Thanks > > liqiong >
Thanks, Simon | |