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

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

[Greg Kroah-Hartman]

On Thu, Jul 22, 2021 at 12:50:34AM +0800, xiyuyang19@fudan.edu.cn wrote:

You sent an empty reply???

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

[Xiyu Yang]

Hi Greg,

I'm not sure why failed... I send it again now.

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@linuxfoundation.org>
> Sent Time: 2021-07-22 13:49:53 (Thursday)
> To: "xiyuyang19@fudan.edu.cn" <xiyuyang19@fudan.edu.cn>
> Cc: "Tejun Heo" <tj@kernel.org>, linux-kernel@vger.kernel.org, yuanxzhang@fudan.edu.cn, "Xin Tan" <tanxin.ctf@gmail.com>
> Subject: Re: [PATCH] kernfs: Convert from atomic_t to refcount_t on kernfs_node->count
> 
> On Thu, Jul 22, 2021 at 12:50:34AM +0800, xiyuyang19@fudan.edu.cn wrote:
> 
> You sent an empty reply???

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

[Greg Kroah-Hartman]

On Thu, Jul 22, 2021 at 02:25:18PM +0800, Xiyu Yang wrote:
> Hi Greg,
> 
> I'm not sure why failed... I send it again now.
> 
> 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(). 

I'm sorry, but I have no context for this statement, what are you
referring to?

Always properly quote the email you are responding to, so we know what
is happening.  Remember, we deal with thousands of emails a week...

thanks,

greg k-h

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

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

[Greg Kroah-Hartman]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Thu, Jul 22, 2021 at 02:49:37PM +0800, Xiyu Yang wrote:
> 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.

Did you test this?  Is this really a reference count when looking at the
code?  Or is it just a counter that we use for dealing with vfs issues?

Usually filesystems and the vfs can not use the refcount_t type for
various reasons, please do some research on that before making these
changes.

And of course, please explain how you tested this patch if you resubmit
it with the needed information.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

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