Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Sender: Tejun Heo <htejun@gmail.com>
Date:   Thu, 5 May 2022 10:01:51 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Imran Khan <imran.f.khan@oracle.com>
Cc:     viro@zeniv.linux.org.uk, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/5] kernfs: make ->attr.open RCU protected.
Message-ID: <YnQtL7+GYHwpo4n2@slm.duckdns.org>
References: <20220428055431.3826852-1-imran.f.khan@oracle.com>
 <20220428055431.3826852-3-imran.f.khan@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220428055431.3826852-3-imran.f.khan@oracle.com>
Precedence: bulk

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.

> +/*
> + * 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()?

> @@ -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? 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.

> @@ -201,7 +235,8 @@ static ssize_t kernfs_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  		goto out_free;
>  	}
>  
> -	of->event = atomic_read(&of->kn->attr.open->event);
> +	on = kernfs_deref_on_raw(of->kn);
> +	of->event = atomic_read(&unrcu_pointer(on)->event);

Ditto here.

> @@ -815,7 +843,7 @@ 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 = kn->attr.open;
> +	struct kernfs_open_node *on = kernfs_deref_on_raw(kn);

and here.

> @@ -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?

Thanks.

-- 
tejun