Messages in this thread | | | Date | Thu, 12 May 2022 07:09:26 -1000 | From | Tejun Heo <> | Subject | Re: [PATCH 2/5] kernfs: make ->attr.open RCU protected. |
| |
Hello,
On Fri, May 06, 2022 at 09:12:23AM +1000, Imran Khan wrote: > On 6/5/22 6:01 am, Tejun Heo wrote: > > On Thu, Apr 28, 2022 at 03:54:28PM +1000, Imran Khan wrote: > >> +static struct kernfs_open_node *kernfs_deref_on_raw(struct kernfs_node *kn) > >> +{ > >> + return rcu_dereference_raw(kn->attr.open); > >> +} > > > > Wrapping the above probably isn't helping anything. > > > > This change is using raw deref at a few places, so I thought of putting > raw deref under a wrapper and explain in the wrapper function comment > under what conditions raw dereferencing was safe.
I don't think they need raw deref in the first place and if you *really* need raw deref, the wrapper explaining why they're needed and how they're safe is the whole point of it and I don't think the wrapper achieves that.
> >> +/* > >> + * Check ->attr.open corresponding to @kn while holding kernfs_open_file_mutex. > >> + * ->attr.open is modified under kernfs_open_file_mutex. So it can be safely > >> + * accessed outside RCU read-side critical section, while holding the mutex. > >> + */ > >> +static struct kernfs_open_node *kernfs_check_on_protected(struct kernfs_node *kn) > >> +{ > >> + return rcu_dereference_check(kn->attr.open, > >> + lockdep_is_held(&kernfs_open_file_mutex)); > >> +} > > > > Maybe name this just kernfs_deref_on()? > > > > Sure. I can mention in the function comment that this should be used > only under kernfs_open_file_mutex.
and in the check condition, add the conditions that you need to make this not trigger warning when used in all the places that you wanna use it.
> +static struct kernfs_open_node *kernfs_deref_on_raw(struct > kernfs_open_file *of, struct kernfs_node *kn) > { > - return rcu_dereference_raw(kn->attr.open); > + struct kernfs_open_node *on; > + > + if (list_empty(&of->list)) > + return NULL; > + > + on = rcu_dereference_raw(kn->attr.open); > + > + if (list_empty(&on->files)) > + return NULL; > + else > + return on;
Put the above conditions in the rcu_dereference_check(). That's what it is for - describing the additional conditions that make the dereference safe.
Thanks.
-- tejun
| |