Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Subject: Re: [PATCH -next 3/3] nbd: fix null-ptr-dereference while accessing
 'nbd->config'
To:     Zhong Jinghua <zhongjinghua@huaweicloud.com>, josef@toxicpanda.com,
        axboe@kernel.dk
Cc:     linux-block@vger.kernel.org, nbd@other.debian.org,
        linux-kernel@vger.kernel.org, yi.zhang@huawei.com,
        "yukuai (C)" <yukuai3@huawei.com>
References: <20230707031536.666482-1-zhongjinghua@huaweicloud.com>
 <20230707031536.666482-4-zhongjinghua@huaweicloud.com>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <1a59b683-38e0-82ee-6122-4bd1613a2269@huaweicloud.com>
Date:   Thu, 28 Sep 2023 14:07:23 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20230707031536.666482-4-zhongjinghua@huaweicloud.com>
Content-Type: text/plain; charset=gbk; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

?? 2023/07/07 11:15, Zhong Jinghua д??:
> From: Zhong Jinghua <zhongjinghua@huawei.com>
> 
> nbd->config = config and refcount_set(&nbd->config_refs, 1) in
> nbd_genl_connect may be out of order, causing config_refs to be set to 1
> first, and then nbd_open accessing nbd->config reports a null pointer
> reference.
>     T1                      T2
> vfs_open
>    do_dentry_open
>      blkdev_open
>        blkdev_get
>          __blkdev_get
>            nbd_open
>             nbd_get_config_unlocked
>                          genl_rcv_msg
>                            genl_family_rcv_msg
>                              genl_family_rcv_msg_doit
>                                nbd_genl_connect
>                                  nbd_alloc_and_init_config
>                                    // out of order execution
>                                    refcount_set(&nbd->config_refs, 1); // 2
>               nbd->config
>               // null point
>                                    nbd->config = config; // 1
> 
> Fix it by adding a cpu memory barrier to guarantee sequential execution.
> 
LGTM

Reviewed-by: Yu Kuai <yukuai3@huawei.com>

> Signed-off-by: Zhong Jinghua <zhongjinghua@huawei.com>
> ---
>   drivers/block/nbd.c | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 7186a9a49445..c410cf29fb0c 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -395,8 +395,16 @@ static u32 req_to_nbd_cmd_type(struct request *req)
>   
>   static struct nbd_config *nbd_get_config_unlocked(struct nbd_device *nbd)
>   {
> -	if (refcount_inc_not_zero(&nbd->config_refs))
> +	if (refcount_inc_not_zero(&nbd->config_refs)) {
> +		/*
> +		 * Add smp_mb__after_atomic to ensure that reading nbd->config_refs
> +		 * and reading nbd->config is ordered. The pair is the barrier in
> +		 * nbd_alloc_and_init_config(), avoid nbd->config_refs is set
> +		 * before nbd->config.
> +		 */
> +		smp_mb__after_atomic();
>   		return nbd->config;
> +	}
>   
>   	return NULL;
>   }
> @@ -1555,7 +1563,15 @@ static int nbd_alloc_and_init_config(struct nbd_device *nbd)
>   	init_waitqueue_head(&config->conn_wait);
>   	config->blksize_bits = NBD_DEF_BLKSIZE_BITS;
>   	atomic_set(&config->live_connections, 0);
> +
>   	nbd->config = config;
> +	/*
> +	 * Order refcount_set(&nbd->config_refs, 1) and nbd->config assignment,
> +	 * its pair is the barrier in nbd_get_config_unlocked().
> +	 * So nbd_get_config_unlocked() won't see nbd->config as null after
> +	 * refcount_inc_not_zero() succeed.
> +	 */
> +	smp_mb__before_atomic();
>   	refcount_set(&nbd->config_refs, 1);
>   
>   	return 0;
>