Message-ID: <4532123f-4c03-0303-1480-a51faabca806@oracle.com>
Date:   Tue, 17 May 2022 12:30:26 +1000
From:   Imran Khan <imran.f.khan@...cle.com>
To:     Tejun Heo <tj@...nel.org>
Cc:     viro@...iv.linux.org.uk, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5] kernfs: make ->attr.open RCU protected.

Hello Tejun,
Thanks for your reply.

On 17/5/22 5:35 am, Tejun Heo wrote:
> Hello,
[...]
> Why does it need to return NULL on empty on->files? We wanna trigger lockdep
> warning cuz that's a bug but it's not like the caller can recover from it
> (short of causing unexpected user visible error), so I don't see what the
> point is.
> 

Yeah. Returning NULL for this is wrong. I overdid the checking part because
current users are using on->event or on->poll. Also I have modified the
interface to accommodate for different condition checks (Please see below) and
this would mean that users of the interface can/should specify the condition
under which dereferenced pointer will be treated as valid.

>> If this looks okay then I can replace usage of kernfs_deref_on_raw with
>> kernfs_deref_on_check.
> 
> So, this is the main deref function without holding the mutex, right? Then
> name it kernfs_deref_open_node() (or on but it seem a bit confusing to me).
> 

kernfs_deref_open_node() looks better to me. But I am thinking that for
dereferencing ->attr.open outside explicit read-side critical section we need
only 2 interfaces. One for the updaters and one for the readers. These updaters
and readers should be able to deref ->attr.open under specific conditions which
guarantee the validity of dereferenced pointer. For example can we have just
have these 2 interfaces:

+/**
+ *     kernfs_deref_open_node - Get kernfs_open_node corresponding to @kn.
+ *
+ *     @kn: target kernfs_node.
+ *     @val: RCU dereference will take place only if val is true.
+ *
+ *     Fetch and return ->attr.open of @kn when current updater of ->attr.open
+ *     ensures that as long as @val is true, other updater(s) of ->attr.open
+ *     can not change it. The caller needs to pass value of the condition
+ *     (@val) that prevents value of ->attr.open from changing.
+ *
+ *     This should ONLY be used by updaters of ->attr.open.
+ */
+static struct kernfs_open_node *
+kernfs_deref_open_node(struct kernfs_node *kn, bool val)
+{
+       return rcu_dereference_protected(kn->attr.open, val);
+}
+
+/**
+ *     kernfs_check_open_node - Get kernfs_open_node corresponding to @kn.
+ *
+ *     @kn: target kernfs_node.
+ *     @val: RCU dereference will take place only if val is true.
+ *
+ *     rcu_dereference and return ->attr.open of @kn. This is used in cases
+ *     where we can access ->attr.open outside RCU read-side critical section
+ *     as long as specified conditions are correct i.e @val is true.
+ *
+ *     This should ONLY be used by readers of ->attr.open.
+ */
+static struct kernfs_open_node *
+kernfs_check_open_node(struct kernfs_node *kn, bool val)
+{
+       return rcu_dereference_check(kn->attr.open, val);
+}
+

This will avoid need of different interfaces for different conditions
(holding kernfs_open_file_mutex, @of->list non-empty etc.) Current updaters like
kernfs_get_open_node, kernfs_unlink_open_file can specify
lockdep_is_held(&kernfs_open_file_mutex) as valid condition. Amongst current
readers kernfs_drain_files can specify the same condition as valid and
kernfs_seq_show, kernfs_file_read_iter etc. can specify !list_empty(&of->list)
as valid condition. This will also help us if dereferencing is needed under
certain other conditions in future.
For just checking pointer value we are using rcu_access_pointer anyways and that
should not need any wrapper.

Could you please let me know if this sounds reasonable? It is not changing
anything that we have discussed so far in this regard, it is just reducing the
number of interfaces .

Thanks
-- Imran

Powered by blists - more mailing lists