linux-kernel - Re: Re: [PATCH] kernfs: Convert from atomic_t to refcount_t on kernfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <57e48621.5aed.17accfa0d36.Coremail.19210240158@fudan.edu.cn>
Date:   Thu, 22 Jul 2021 14:49:37 +0800 (GMT+08:00)
From:   "Xiyu Yang" <19210240158@...an.edu.cn>
To:     "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>
Cc:     "Tejun Heo" <tj@...nel.org>, linux-kernel@...r.kernel.org,
        yuanxzhang@...an.edu.cn, "Xin Tan" <tanxin.ctf@...il.com>
Subject: Re: Re: [PATCH] kernfs: Convert from atomic_t to refcount_t on
 kernfs_node->count

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@...uxfoundation.org>
> Sent Time: 2021-07-21 23:36:20 (Wednesday)
> To: "Xiyu Yang" <xiyuyang19@...an.edu.cn>
> Cc: "Tejun Heo" <tj@...nel.org>, linux-kernel@...r.kernel.org, yuanxzhang@...an.edu.cn, "Xin Tan" <tanxin.ctf@...il.com>
> 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@...an.edu.cn>
> > Signed-off-by: Xin Tan <tanxin.ctf@...il.com>
> > ---
> >  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