Messages in this thread | | | Date | Mon, 16 May 2022 02:00:50 +1000 | Subject | Re: [PATCH 2/5] kernfs: make ->attr.open RCU protected. | From | Imran Khan <> |
| |
Hello Tejun,
Thanks for your reply.
On 13/5/22 3:09 am, Tejun Heo wrote: > 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. >
Okay. I checked with rcu_access_pointer and CONFIG_PROVE_RCU enabled and did not observe any warning(s) so rcu_access_pointer should have sufficed here. But I am using rcu_dereference_check to accommodate checking of @of->list as well. The checking of @of->list also covers one of your later suggestions. I have updated the description of function as well. Could you please let me know if below looks okay:
+/* + * Deref RCU protected kn->attr.open. + * If both @of->list and @kn->attr.open->files are non empty, we can safely + * assume that @of is on @kn->attr.open and hence @kn->attr.open will not + * vanish and derefeencing is safe here. + */ +static struct kernfs_open_node * +kernfs_deref_on_check(struct kernfs_open_file *of, struct kernfs_node *kn) +{ + struct kernfs_open_node *on; + + on = rcu_dereference_check(kn->attr.open, !list_empty(&of->list)); + + if (on && list_empty(&on->files)) + return NULL; + else + return on; +} +
If this looks okay then I can replace usage of kernfs_deref_on_raw with kernfs_deref_on_check.
>>>> +/* >>>> + * 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. >
Since ->attr.open is always accessed/modified under kernfs_open_file_mutex, I have included this check in helper which should be used only while holding this mutex. Do you mean that I should include some additional checks as well in the below helper:
+/* + * Deref ->attr.open corresponding to @kn while holding open_file_mutex. + * ->attr.open is modified under kernfs_open_file_mutex. So it can be safely + * safely accessed outside RCU read-side critical section, while holding + * the mutex. + */ +static struct kernfs_open_node *kernfs_deref_on(struct kernfs_node *kn) +{ + return rcu_dereference_protected(kn->attr.open, + lockdep_is_held(&kernfs_open_file_mutex)); +} +
>> +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. >
As mentioned earlier, I have included checking of @of->list in rcu_dereference_check. I am not sure if we can include checking of on->files as well because "on" itself is the dereferenced pointer value here. So I have kept checking of on->files separate as shown in the earlier snippet of kernfs_deref_on_check above.
Thanks -- Imran
| |