Re: [PATCH RESEND v4 1/4] kernfs: make ->attr.open RCU protected.

[Imran Khan]

Hello Tejun,

On 13/6/22 1:09 pm, Tejun Heo wrote:
> On Mon, Jun 13, 2022 at 12:55:14PM +1000, Imran Khan wrote:
>> I took below phrases as reference:
>>
>> If the access might be within an RCU read-side critical section on the one hand,
>> or protected by (say) my_lock on the other, use rcu_dereference_check(), for
>> example:
>>
>> p1 = rcu_dereference_check(p->rcu_protected_pointer,
>>                            lockdep_is_held(&my_lock));
>>
>>
>> and
>>
>>
>> If the access might be within an RCU read-side critical section on the one hand,
>> or protected by either my_lock or your_lock on the other, again use
>> rcu_dereference_check(), for example:
>>
>> p1 = rcu_dereference_check(p->rcu_protected_pointer,
>>                            lockdep_is_held(&my_lock) ||
>>                            lockdep_is_held(&your_lock));
> 
> So, both are saying that if a given reference can be under both read
> critical section or a lock which blocks updates, you can use deref_check to
> cover both cases - we're just using the stronger form of derefing even
> though that's not necessary while update side is locked out, which is fine.
> 
> The protected one is different in that it doesn't enforce the load ordering
> which is required for accesses with only RCU read lock. Given that all
> that's required is dependency ordering, I doubt it makes any actual
> difference and it likely is more useful in marking a specific dereference as
> always being with the update side locked.> tl;dr is that you're way over-thinking the rcu deref code. Just make one
> deref accessor which encompasses all three use cases.
> 
> 

Agree. I did over think this and went for the safest interface that I could
think of in each of the use cases. I will remove
kernfs_check_open_node_protected and use kernfs_deref_open_node_protected in its
place as well. This will cover all accesses under kernfs_open_file_mutex.

But we will still need kernfs_deref_open_node for cases where
!list_empty(&of->list) ensures safe access of ->attr.open and where we can't
ensure holding of kernfs_open_file_mutex. So we will need 2 deref accessors.
Right? Just asking this because you mentioned above to come up with one deref
accessor that can be used in all three use cases

Please let me if this sounds okay. I can send updated patch-set with these
changes in place.

Thanks,
-- Imran