Re: [PATCH] iomap: Convert from atomic_t to refcount_t on iomap_dio->ref

Re: [PATCH] iomap: Convert from atomic_t to refcount_t on iomap_dio->ref

[Darrick J. Wong]

On Mon, Jul 19, 2021 at 04:32:24PM +0800, Xiyu Yang wrote:
> refcount_t type and corresponding API can protect refcounters from
> accidental underflow and overflow and further use-after-free situations.
> 
> Signed-off-by: Xiyu Yang <xiyuyang19@fudan.edu.cn>
> Signed-off-by: Xin Tan <tanxin.ctf@gmail.com>
> ---
>  fs/iomap/direct-io.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 9398b8c31323..d1393579a954 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -3,6 +3,7 @@
>   * Copyright (C) 2010 Red Hat, Inc.
>   * Copyright (c) 2016-2018 Christoph Hellwig.
>   */
> +#include <linux/refcount.h>
>  #include <linux/module.h>
>  #include <linux/compiler.h>
>  #include <linux/fs.h>
> @@ -28,7 +29,7 @@ struct iomap_dio {
>  	const struct iomap_dio_ops *dops;
>  	loff_t			i_size;
>  	loff_t			size;
> -	atomic_t		ref;
> +	refcount_t		ref;
>  	unsigned		flags;
>  	int			error;
>  	bool			wait_for_completion;
> @@ -62,7 +63,7 @@ EXPORT_SYMBOL_GPL(iomap_dio_iopoll);
>  static void iomap_dio_submit_bio(struct iomap_dio *dio, struct iomap *iomap,
>  		struct bio *bio, loff_t pos)
>  {
> -	atomic_inc(&dio->ref);
> +	refcount_inc(&dio->ref);

Seems all right to me, though I wonder if any of the more
performance-minded people have comments about the overhead of refcount_t
vs. atomic_t?

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  
>  	if (dio->iocb->ki_flags & IOCB_HIPRI)
>  		bio_set_polled(bio, dio->iocb);
> @@ -158,7 +159,7 @@ static void iomap_dio_bio_end_io(struct bio *bio)
>  	if (bio->bi_status)
>  		iomap_dio_set_error(dio, blk_status_to_errno(bio->bi_status));
>  
> -	if (atomic_dec_and_test(&dio->ref)) {
> +	if (refcount_dec_and_test(&dio->ref)) {
>  		if (dio->wait_for_completion) {
>  			struct task_struct *waiter = dio->submit.waiter;
>  			WRITE_ONCE(dio->submit.waiter, NULL);
> @@ -471,7 +472,7 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  		return ERR_PTR(-ENOMEM);
>  
>  	dio->iocb = iocb;
> -	atomic_set(&dio->ref, 1);
> +	refcount_set(&dio->ref, 1);
>  	dio->size = 0;
>  	dio->i_size = i_size_read(inode);
>  	dio->dops = dops;
> @@ -611,7 +612,7 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  	 *	after we got woken by the I/O completion handler.
>  	 */
>  	dio->wait_for_completion = wait_for_completion;
> -	if (!atomic_dec_and_test(&dio->ref)) {
> +	if (!refcount_dec_and_test(&dio->ref)) {
>  		if (!wait_for_completion)
>  			return ERR_PTR(-EIOCBQUEUED);
>  
> -- 
> 2.7.4
>