Re: [PATCH 2/5] kernfs: make ->attr.open RCU protected.

[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