Date:   Fri, 6 May 2022 09:12:23 +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,

Thank you for reviewing this.

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.

>> +/*
>> + * 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.

>> @@ -156,8 +188,9 @@ static void kernfs_seq_stop(struct seq_file *sf, void *v)
>>  static int kernfs_seq_show(struct seq_file *sf, void *v)
>>  {
>>  	struct kernfs_open_file *of = sf->private;
>> +	struct kernfs_open_node *on = kernfs_deref_on_raw(of->kn);
> 
> I suppose this is protected by the fact that @of is on @on? 

Yes.

> If so, just add
> the condition to the checked version. The condition doesn't have to be
> perfect - e.g. you can just say that neither @on's and @of's list_head isn't
> empty. While not comprehensive, it'd still provide meaningful protection
> against mistakes and be easier to understand if the deref accessor clearly
> explains the expectations.
> 

Sure. Could you please let me know if below modification looks good ?

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 946a4a8d7e32..a12d5067f8d5 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -56,12 +56,23 @@ static inline struct mutex
*kernfs_open_file_mutex_lock(struct kernfs_node *kn)

 /*
  * Raw deref RCU protected kn->attr.open.
- * The caller guarantees that @on will not vanish in the middle of this
- * function and hence we can deref ->attr.open raw.
+ * 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 raw derefeencing is safe here.
  */
-static struct kernfs_open_node *kernfs_deref_on_raw(struct kernfs_node *kn)
+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;
 }
 /*
@@ -191,7 +202,10 @@ static void kernfs_seq_stop(struct seq_file *sf,
void *v)
 static int kernfs_seq_show(struct seq_file *sf, void *v)
 {
        struct kernfs_open_file *of = sf->private;
-       struct kernfs_open_node *on = kernfs_deref_on_raw(of->kn);
+       struct kernfs_open_node *on = kernfs_deref_on_raw(of, of->kn);
+
+       if(!on)
+               return -EINVAL;

        of->event = atomic_read(&unrcu_pointer(on)->event);

@@ -238,7 +252,10 @@ static ssize_t kernfs_file_read_iter(struct kiocb
*iocb, struct iov_iter *iter)
                goto out_free;
        }

-       on = kernfs_deref_on_raw(of->kn);
+       on = kernfs_deref_on_raw(of, of->kn);
+       if(!on)
+               return -EINVAL;
+
        of->event = atomic_read(&unrcu_pointer(on)->event);
        ops = kernfs_ops(of->kn);
        if (ops->read)
@@ -850,7 +867,10 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
 __poll_t kernfs_generic_poll(struct kernfs_open_file *of, poll_table *wait)
 {
        struct kernfs_node *kn =
kernfs_dentry_node(of->file->f_path.dentry);
-       struct kernfs_open_node *on = kernfs_deref_on_raw(kn);
+       struct kernfs_open_node *on = kernfs_deref_on_raw(of, kn);
+
+       if (!on)
+               return EPOLLERR;

        poll_wait(of->file, &on->poll, wait);

>> @@ -201,7 +235,8 @@ static ssize_t kernfs_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>>  		goto out_free;
[...]
>> @@ -922,13 +950,13 @@ void kernfs_notify(struct kernfs_node *kn)
>>  		return;
>>  
>>  	/* kick poll immediately */
>> -	spin_lock_irqsave(&kernfs_open_node_lock, flags);
>> -	on = kn->attr.open;
>> +	rcu_read_lock();
>> +	on = rcu_dereference(kn->attr.open);
> 
> Shouldn't this be kernfs_deref_on() too?
> 

kernfs_notify can be invoked w/o holding kernfs_open_file_mutex (e.g.
cgroup_file_notify). So we have explicit RCU read-side critical section
here and hence just rcu_dereference.

Thanks
 -- Imran
> Thanks.
> 

Powered by blists - more mailing lists