| Message ID | 1545162003-11577-3-git-send-email-frowand.list@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | of: phandle_cache, fix refcounts, remove stale entry | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/build-ppc64le | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64be | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64e | warning | build succeeded but added 1 new sparse warning(s) |
| snowpatch_ozlabs/build-pmac32 | warning | build succeeded but added 1 new sparse warning(s) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 66 lines checked |
On 12/18/18 11:40 AM, frowand.list@gmail.com wrote: > From: Frank Rowand <frank.rowand@sony.com> > > Non-overlay dynamic devicetree node removal may leave the node in > the phandle cache. Subsequent calls to of_find_node_by_phandle() > will incorrectly find the stale entry. Remove the node from the > cache. > > Add paranoia checks in of_find_node_by_phandle() as a second level > of defense (do not return cached node if detached, do not add node > to cache if detached). > > Fixes: 0b3ce78e90fc ("of: cache phandle nodes to reduce cost of of_find_node_by_phandle()") > Reported-by: Michael Bringmann <mwb@linux.vnet.ibm.com> > Signed-off-by: Frank Rowand <frank.rowand@sony.com> > --- > > do not "cc: stable", unless the following commits are also in stable: > > commit e54192b48da7 ("of: fix phandle cache creation for DTs with no phandles") > commit b9952b5218ad ("of: overlay: update phandle cache on overlay apply and remove") > commit 0b3ce78e90fc ("of: cache phandle nodes to reduce cost of of_find_node_by_phandle()") > > > Changes since v2: > - add temporary variable np in __of_free_phandle_cache_entry() to improve > readability > - explain reason for WARN_ON() in comment > - add Fixes tag in patch comment I should have carried this forward: changes since v1: - add WARN_ON(1) for unexpected condition in of_find_node_by_phandle() -Frank > > drivers/of/base.c | 31 ++++++++++++++++++++++++++++++- > drivers/of/dynamic.c | 3 +++ > drivers/of/of_private.h | 4 ++++ > 3 files changed, 37 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/base.c b/drivers/of/base.c > index 6c33d63361b8..6d20b6dcf034 100644 > --- a/drivers/of/base.c > +++ b/drivers/of/base.c > @@ -162,6 +162,28 @@ int of_free_phandle_cache(void) > late_initcall_sync(of_free_phandle_cache); > #endif > > +/* > + * Caller must hold devtree_lock. > + */ > +void __of_free_phandle_cache_entry(phandle handle) > +{ > + phandle masked_handle; > + struct device_node *np; > + > + if (!handle) > + return; > + > + masked_handle = handle & phandle_cache_mask; > + > + if (phandle_cache) { > + np = phandle_cache[masked_handle]; > + if (np && handle == np->phandle) { > + of_node_put(np); > + phandle_cache[masked_handle] = NULL; > + } > + } > +} > + > void of_populate_phandle_cache(void) > { > unsigned long flags; > @@ -1209,11 +1231,18 @@ struct device_node *of_find_node_by_phandle(phandle handle) > if (phandle_cache[masked_handle] && > handle == phandle_cache[masked_handle]->phandle) > np = phandle_cache[masked_handle]; > + if (np && of_node_check_flag(np, OF_DETACHED)) { > + WARN_ON(1); /* did not uncache np on node removal */ > + of_node_put(np); > + phandle_cache[masked_handle] = NULL; > + np = NULL; > + } > } > > if (!np) { > for_each_of_allnodes(np) > - if (np->phandle == handle) { > + if (np->phandle == handle && > + !of_node_check_flag(np, OF_DETACHED)) { > if (phandle_cache) { > /* will put when removed from cache */ > of_node_get(np); > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c > index f4f8ed9b5454..ecea92f68c87 100644 > --- a/drivers/of/dynamic.c > +++ b/drivers/of/dynamic.c > @@ -268,6 +268,9 @@ void __of_detach_node(struct device_node *np) > } > > of_node_set_flag(np, OF_DETACHED); > + > + /* race with of_find_node_by_phandle() prevented by devtree_lock */ > + __of_free_phandle_cache_entry(np->phandle); > } > > /** > diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h > index 5d1567025358..24786818e32e 100644 > --- a/drivers/of/of_private.h > +++ b/drivers/of/of_private.h > @@ -84,6 +84,10 @@ static inline void __of_detach_node_sysfs(struct device_node *np) {} > int of_resolve_phandles(struct device_node *tree); > #endif > > +#if defined(CONFIG_OF_DYNAMIC) > +void __of_free_phandle_cache_entry(phandle handle); > +#endif > + > #if defined(CONFIG_OF_OVERLAY) > void of_overlay_mutex_lock(void); > void of_overlay_mutex_unlock(void); >
diff --git a/drivers/of/base.c b/drivers/of/base.c index 6c33d63361b8..6d20b6dcf034 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -162,6 +162,28 @@ int of_free_phandle_cache(void) late_initcall_sync(of_free_phandle_cache); #endif +/* + * Caller must hold devtree_lock. + */ +void __of_free_phandle_cache_entry(phandle handle) +{ + phandle masked_handle; + struct device_node *np; + + if (!handle) + return; + + masked_handle = handle & phandle_cache_mask; + + if (phandle_cache) { + np = phandle_cache[masked_handle]; + if (np && handle == np->phandle) { + of_node_put(np); + phandle_cache[masked_handle] = NULL; + } + } +} + void of_populate_phandle_cache(void) { unsigned long flags; @@ -1209,11 +1231,18 @@ struct device_node *of_find_node_by_phandle(phandle handle) if (phandle_cache[masked_handle] && handle == phandle_cache[masked_handle]->phandle) np = phandle_cache[masked_handle]; + if (np && of_node_check_flag(np, OF_DETACHED)) { + WARN_ON(1); /* did not uncache np on node removal */ + of_node_put(np); + phandle_cache[masked_handle] = NULL; + np = NULL; + } } if (!np) { for_each_of_allnodes(np) - if (np->phandle == handle) { + if (np->phandle == handle && + !of_node_check_flag(np, OF_DETACHED)) { if (phandle_cache) { /* will put when removed from cache */ of_node_get(np); diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c index f4f8ed9b5454..ecea92f68c87 100644 --- a/drivers/of/dynamic.c +++ b/drivers/of/dynamic.c @@ -268,6 +268,9 @@ void __of_detach_node(struct device_node *np) } of_node_set_flag(np, OF_DETACHED); + + /* race with of_find_node_by_phandle() prevented by devtree_lock */ + __of_free_phandle_cache_entry(np->phandle); } /** diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h index 5d1567025358..24786818e32e 100644 --- a/drivers/of/of_private.h +++ b/drivers/of/of_private.h @@ -84,6 +84,10 @@ static inline void __of_detach_node_sysfs(struct device_node *np) {} int of_resolve_phandles(struct device_node *tree); #endif +#if defined(CONFIG_OF_DYNAMIC) +void __of_free_phandle_cache_entry(phandle handle); +#endif + #if defined(CONFIG_OF_OVERLAY) void of_overlay_mutex_lock(void); void of_overlay_mutex_unlock(void);