Re: Re: [PATCH] kernfs: Convert from atomic_t to refcount_t on kernfs_node->count

[Xiyu Yang]

Hi Greg,

I consider it as a reference count due to its related operations and the developer's comments, such as "put a reference count on a kernfs_node" around the kernfs_put(). If anything wrong, please let me know.

Thanks.

> -----Original Messages-----
> From: "Greg Kroah-Hartman" <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent Time: 2021-07-21 23:36:20 (Wednesday)
> To: "Xiyu Yang" <xiyuyang19@xxxxxxxxxxxx>
> Cc: "Tejun Heo" <tj@xxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, yuanxzhang@xxxxxxxxxxxx, "Xin Tan" <tanxin.ctf@xxxxxxxxx>
> Subject: Re: [PATCH] kernfs: Convert from atomic_t to refcount_t on kernfs_node->count
> 
> On Mon, Jul 19, 2021 at 04:33:21PM +0800, Xiyu Yang wrote:
> > refcount_t type and corresponding API can protect refcounters from
> > accidental underflow and overflow and further use-after-free situations.
> 
> But this really is a count, not a refcount.
> 
> So are you _sure_ this should be changed?
> 
> What did you to do to test this?
> 
> thanks,
> 
> greg k-h
> > Signed-off-by: Xiyu Yang <xiyuyang19@xxxxxxxxxxxx>
> > Signed-off-by: Xin Tan <tanxin.ctf@xxxxxxxxx>
> > ---
> >  fs/kernfs/dir.c        | 12 ++++++------
> >  include/linux/kernfs.h |  3 ++-
> >  2 files changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> > index 33166ec90a11..201091e2f2dc 100644
> > --- a/fs/kernfs/dir.c
> > +++ b/fs/kernfs/dir.c
> > @@ -491,8 +491,8 @@ static void kernfs_drain(struct kernfs_node *kn)
> >  void kernfs_get(struct kernfs_node *kn)
> >  {
> >  	if (kn) {
> > -		WARN_ON(!atomic_read(&kn->count));
> > -		atomic_inc(&kn->count);
> > +		WARN_ON(!refcount_read(&kn->count));
> > +		refcount_inc(&kn->count);
> >  	}
> >  }
> >  EXPORT_SYMBOL_GPL(kernfs_get);
> > @@ -508,7 +508,7 @@ void kernfs_put(struct kernfs_node *kn)
> >  	struct kernfs_node *parent;
> >  	struct kernfs_root *root;
> >  
> > -	if (!kn || !atomic_dec_and_test(&kn->count))
> > +	if (!kn || !refcount_dec_and_test(&kn->count))
> >  		return;
> >  	root = kernfs_root(kn);
> >   repeat:
> > @@ -538,7 +538,7 @@ void kernfs_put(struct kernfs_node *kn)
> >  
> >  	kn = parent;
> >  	if (kn) {
> > -		if (atomic_dec_and_test(&kn->count))
> > +		if (refcount_dec_and_test(&kn->count))
> >  			goto repeat;
> >  	} else {
> >  		/* just released the root kn, free @root too */
> > @@ -598,7 +598,7 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
> >  
> >  	kn->id = (u64)id_highbits << 32 | ret;
> >  
> > -	atomic_set(&kn->count, 1);
> > +	refcount_set(&kn->count, 1);
> >  	atomic_set(&kn->active, KN_DEACTIVATED_BIAS);
> >  	RB_CLEAR_NODE(&kn->rb);
> >  
> > @@ -691,7 +691,7 @@ struct kernfs_node *kernfs_find_and_get_node_by_id(struct kernfs_root *root,
> >  	 * grab kernfs_mutex.
> >  	 */
> >  	if (unlikely(!(kn->flags & KERNFS_ACTIVATED) ||
> > -		     !atomic_inc_not_zero(&kn->count)))
> > +		     !refcount_inc_not_zero(&kn->count)))
> >  		goto err_unlock;
> >  
> >  	spin_unlock(&kernfs_idr_lock);
> > diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
> > index 9e8ca8743c26..4378a5befcf7 100644
> > --- a/include/linux/kernfs.h
> > +++ b/include/linux/kernfs.h
> > @@ -6,6 +6,7 @@
> >  #ifndef __LINUX_KERNFS_H
> >  #define __LINUX_KERNFS_H
> >  
> > +#include <linux/refcount.h>
> >  #include <linux/kernel.h>
> >  #include <linux/err.h>
> >  #include <linux/list.h>
> > @@ -121,7 +122,7 @@ struct kernfs_elem_attr {
> >   * active reference.
> >   */
> >  struct kernfs_node {
> > -	atomic_t		count;
> > +	refcount_t		count;
> >  	atomic_t		active;
> >  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >  	struct lockdep_map	dep_map;
> > -- 
> > 2.7.4