From: Zhong Jinghua <[email protected]>
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.
Zhong Jinghua (3):
nbd: fold nbd config initialization into nbd_alloc_config()
nbd: factor out a helper to get nbd_config without holding
'config_lock'
nbd: fix null-ptr-dereference while accessing 'nbd->config'
drivers/block/nbd.c | 82 +++++++++++++++++++++++++++++----------------
1 file changed, 53 insertions(+), 29 deletions(-)
--
2.31.1
From: Zhong Jinghua <[email protected]>
There are no functional changes, just to make code cleaner and prepare
to fix null-ptr-dereference while accessing 'nbd->config'.
Signed-off-by: Zhong Jinghua <[email protected]>
---
drivers/block/nbd.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index cd6d78914954..7186a9a49445 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -393,6 +393,14 @@ 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))
+ return nbd->config;
+
+ return NULL;
+}
+
static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req)
{
struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
@@ -407,13 +415,13 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req)
return BLK_EH_DONE;
}
- if (!refcount_inc_not_zero(&nbd->config_refs)) {
+ config = nbd_get_config_unlocked(nbd);
+ if (!config) {
cmd->status = BLK_STS_TIMEOUT;
__clear_bit(NBD_CMD_INFLIGHT, &cmd->flags);
mutex_unlock(&cmd->lock);
goto done;
}
- config = nbd->config;
if (config->num_connections > 1 ||
(config->num_connections == 1 && nbd->tag_set.timeout)) {
@@ -975,12 +983,12 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
struct nbd_sock *nsock;
int ret;
- if (!refcount_inc_not_zero(&nbd->config_refs)) {
+ config = nbd_get_config_unlocked(nbd);
+ if (!config) {
dev_err_ratelimited(disk_to_dev(nbd->disk),
"Socks array is empty\n");
return -EINVAL;
}
- config = nbd->config;
if (index >= config->num_connections) {
dev_err_ratelimited(disk_to_dev(nbd->disk),
@@ -1556,6 +1564,7 @@ static int nbd_alloc_and_init_config(struct nbd_device *nbd)
static int nbd_open(struct block_device *bdev, fmode_t mode)
{
struct nbd_device *nbd;
+ struct nbd_config *config;
int ret = 0;
mutex_lock(&nbd_index_mutex);
@@ -1568,7 +1577,9 @@ static int nbd_open(struct block_device *bdev, fmode_t mode)
ret = -ENXIO;
goto out;
}
- if (!refcount_inc_not_zero(&nbd->config_refs)) {
+
+ config = nbd_get_config_unlocked(nbd);
+ if (!config) {
mutex_lock(&nbd->config_lock);
if (refcount_inc_not_zero(&nbd->config_refs)) {
mutex_unlock(&nbd->config_lock);
@@ -1584,7 +1595,7 @@ static int nbd_open(struct block_device *bdev, fmode_t mode)
mutex_unlock(&nbd->config_lock);
if (max_part)
set_bit(GD_NEED_PART_SCAN, &bdev->bd_disk->state);
- } else if (nbd_disconnected(nbd->config)) {
+ } else if (nbd_disconnected(config)) {
if (max_part)
set_bit(GD_NEED_PART_SCAN, &bdev->bd_disk->state);
}
@@ -2194,7 +2205,8 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
}
mutex_unlock(&nbd_index_mutex);
- if (!refcount_inc_not_zero(&nbd->config_refs)) {
+ config = nbd_get_config_unlocked(nbd);
+ if (!config) {
dev_err(nbd_to_dev(nbd),
"not configured, cannot reconfigure\n");
nbd_put(nbd);
@@ -2202,7 +2214,6 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
}
mutex_lock(&nbd->config_lock);
- config = nbd->config;
if (!test_bit(NBD_RT_BOUND, &config->runtime_flags) ||
!nbd->pid) {
dev_err(nbd_to_dev(nbd),
--
2.31.1
From: Zhong Jinghua <[email protected]>
There are no functional changes, make the code cleaner and prepare to
fix null-ptr-dereference while accessing 'nbd->config'.
Signed-off-by: Zhong Jinghua <[email protected]>
---
drivers/block/nbd.c | 41 +++++++++++++++++++----------------------
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index cb38477f359f..cd6d78914954 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1526,17 +1526,20 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
return error;
}
-static struct nbd_config *nbd_alloc_config(void)
+static int nbd_alloc_and_init_config(struct nbd_device *nbd)
{
struct nbd_config *config;
+ if (WARN_ON(nbd->config))
+ return -EINVAL;
+
if (!try_module_get(THIS_MODULE))
- return ERR_PTR(-ENODEV);
+ return -ENODEV;
config = kzalloc(sizeof(struct nbd_config), GFP_NOFS);
if (!config) {
module_put(THIS_MODULE);
- return ERR_PTR(-ENOMEM);
+ return -ENOMEM;
}
atomic_set(&config->recv_threads, 0);
@@ -1544,7 +1547,10 @@ static struct nbd_config *nbd_alloc_config(void)
init_waitqueue_head(&config->conn_wait);
config->blksize_bits = NBD_DEF_BLKSIZE_BITS;
atomic_set(&config->live_connections, 0);
- return config;
+ nbd->config = config;
+ refcount_set(&nbd->config_refs, 1);
+
+ return 0;
}
static int nbd_open(struct block_device *bdev, fmode_t mode)
@@ -1563,21 +1569,17 @@ static int nbd_open(struct block_device *bdev, fmode_t mode)
goto out;
}
if (!refcount_inc_not_zero(&nbd->config_refs)) {
- struct nbd_config *config;
-
mutex_lock(&nbd->config_lock);
if (refcount_inc_not_zero(&nbd->config_refs)) {
mutex_unlock(&nbd->config_lock);
goto out;
}
- config = nbd_alloc_config();
- if (IS_ERR(config)) {
- ret = PTR_ERR(config);
+ ret = nbd_alloc_and_init_config(nbd);
+ if (ret) {
mutex_unlock(&nbd->config_lock);
goto out;
}
- nbd->config = config;
- refcount_set(&nbd->config_refs, 1);
+
refcount_inc(&nbd->refs);
mutex_unlock(&nbd->config_lock);
if (max_part)
@@ -1979,22 +1981,17 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
pr_err("nbd%d already in use\n", index);
return -EBUSY;
}
- if (WARN_ON(nbd->config)) {
- mutex_unlock(&nbd->config_lock);
- nbd_put(nbd);
- return -EINVAL;
- }
- config = nbd_alloc_config();
- if (IS_ERR(config)) {
+
+ ret = nbd_alloc_and_init_config(nbd);
+ if (ret) {
mutex_unlock(&nbd->config_lock);
nbd_put(nbd);
pr_err("couldn't allocate config\n");
- return PTR_ERR(config);
+ return ret;
}
- nbd->config = config;
- refcount_set(&nbd->config_refs, 1);
- set_bit(NBD_RT_BOUND, &config->runtime_flags);
+ config = nbd->config;
+ set_bit(NBD_RT_BOUND, &config->runtime_flags);
ret = nbd_genl_size_set(info, nbd);
if (ret)
goto out;
--
2.31.1
From: Zhong Jinghua <[email protected]>
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.
Signed-off-by: Zhong Jinghua <[email protected]>
---
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;
--
2.31.1
?? 2023/07/07 11:15, Zhong Jinghua д??:
> From: Zhong Jinghua <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Zhong Jinghua <[email protected]>
> ---
> 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;
>
?? 2023/07/07 11:15, Zhong Jinghua д??:
> From: Zhong Jinghua <[email protected]>
>
> There are no functional changes, just to make code cleaner and prepare
> to fix null-ptr-dereference while accessing 'nbd->config'.
>
LGTM
Reviewed-by: Yu Kuai <[email protected]>
> Signed-off-by: Zhong Jinghua <[email protected]>
> ---
> drivers/block/nbd.c | 27 +++++++++++++++++++--------
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index cd6d78914954..7186a9a49445 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -393,6 +393,14 @@ 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))
> + return nbd->config;
> +
> + return NULL;
> +}
> +
> static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req)
> {
> struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req);
> @@ -407,13 +415,13 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req)
> return BLK_EH_DONE;
> }
>
> - if (!refcount_inc_not_zero(&nbd->config_refs)) {
> + config = nbd_get_config_unlocked(nbd);
> + if (!config) {
> cmd->status = BLK_STS_TIMEOUT;
> __clear_bit(NBD_CMD_INFLIGHT, &cmd->flags);
> mutex_unlock(&cmd->lock);
> goto done;
> }
> - config = nbd->config;
>
> if (config->num_connections > 1 ||
> (config->num_connections == 1 && nbd->tag_set.timeout)) {
> @@ -975,12 +983,12 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
> struct nbd_sock *nsock;
> int ret;
>
> - if (!refcount_inc_not_zero(&nbd->config_refs)) {
> + config = nbd_get_config_unlocked(nbd);
> + if (!config) {
> dev_err_ratelimited(disk_to_dev(nbd->disk),
> "Socks array is empty\n");
> return -EINVAL;
> }
> - config = nbd->config;
>
> if (index >= config->num_connections) {
> dev_err_ratelimited(disk_to_dev(nbd->disk),
> @@ -1556,6 +1564,7 @@ static int nbd_alloc_and_init_config(struct nbd_device *nbd)
> static int nbd_open(struct block_device *bdev, fmode_t mode)
> {
> struct nbd_device *nbd;
> + struct nbd_config *config;
> int ret = 0;
>
> mutex_lock(&nbd_index_mutex);
> @@ -1568,7 +1577,9 @@ static int nbd_open(struct block_device *bdev, fmode_t mode)
> ret = -ENXIO;
> goto out;
> }
> - if (!refcount_inc_not_zero(&nbd->config_refs)) {
> +
> + config = nbd_get_config_unlocked(nbd);
> + if (!config) {
> mutex_lock(&nbd->config_lock);
> if (refcount_inc_not_zero(&nbd->config_refs)) {
> mutex_unlock(&nbd->config_lock);
> @@ -1584,7 +1595,7 @@ static int nbd_open(struct block_device *bdev, fmode_t mode)
> mutex_unlock(&nbd->config_lock);
> if (max_part)
> set_bit(GD_NEED_PART_SCAN, &bdev->bd_disk->state);
> - } else if (nbd_disconnected(nbd->config)) {
> + } else if (nbd_disconnected(config)) {
> if (max_part)
> set_bit(GD_NEED_PART_SCAN, &bdev->bd_disk->state);
> }
> @@ -2194,7 +2205,8 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
> }
> mutex_unlock(&nbd_index_mutex);
>
> - if (!refcount_inc_not_zero(&nbd->config_refs)) {
> + config = nbd_get_config_unlocked(nbd);
> + if (!config) {
> dev_err(nbd_to_dev(nbd),
> "not configured, cannot reconfigure\n");
> nbd_put(nbd);
> @@ -2202,7 +2214,6 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
> }
>
> mutex_lock(&nbd->config_lock);
> - config = nbd->config;
> if (!test_bit(NBD_RT_BOUND, &config->runtime_flags) ||
> !nbd->pid) {
> dev_err(nbd_to_dev(nbd),
>
?? 2023/07/07 11:15, Zhong Jinghua д??:
> From: Zhong Jinghua <[email protected]>
>
> There are no functional changes, make the code cleaner and prepare to
> fix null-ptr-dereference while accessing 'nbd->config'.
>
LGTM
Reviewed-by: Yu Kuai <[email protected]>
> Signed-off-by: Zhong Jinghua <[email protected]>
> ---
> drivers/block/nbd.c | 41 +++++++++++++++++++----------------------
> 1 file changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index cb38477f359f..cd6d78914954 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -1526,17 +1526,20 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
> return error;
> }
>
> -static struct nbd_config *nbd_alloc_config(void)
> +static int nbd_alloc_and_init_config(struct nbd_device *nbd)
> {
> struct nbd_config *config;
>
> + if (WARN_ON(nbd->config))
> + return -EINVAL;
> +
> if (!try_module_get(THIS_MODULE))
> - return ERR_PTR(-ENODEV);
> + return -ENODEV;
>
> config = kzalloc(sizeof(struct nbd_config), GFP_NOFS);
> if (!config) {
> module_put(THIS_MODULE);
> - return ERR_PTR(-ENOMEM);
> + return -ENOMEM;
> }
>
> atomic_set(&config->recv_threads, 0);
> @@ -1544,7 +1547,10 @@ static struct nbd_config *nbd_alloc_config(void)
> init_waitqueue_head(&config->conn_wait);
> config->blksize_bits = NBD_DEF_BLKSIZE_BITS;
> atomic_set(&config->live_connections, 0);
> - return config;
> + nbd->config = config;
> + refcount_set(&nbd->config_refs, 1);
> +
> + return 0;
> }
>
> static int nbd_open(struct block_device *bdev, fmode_t mode)
> @@ -1563,21 +1569,17 @@ static int nbd_open(struct block_device *bdev, fmode_t mode)
> goto out;
> }
> if (!refcount_inc_not_zero(&nbd->config_refs)) {
> - struct nbd_config *config;
> -
> mutex_lock(&nbd->config_lock);
> if (refcount_inc_not_zero(&nbd->config_refs)) {
> mutex_unlock(&nbd->config_lock);
> goto out;
> }
> - config = nbd_alloc_config();
> - if (IS_ERR(config)) {
> - ret = PTR_ERR(config);
> + ret = nbd_alloc_and_init_config(nbd);
> + if (ret) {
> mutex_unlock(&nbd->config_lock);
> goto out;
> }
> - nbd->config = config;
> - refcount_set(&nbd->config_refs, 1);
> +
> refcount_inc(&nbd->refs);
> mutex_unlock(&nbd->config_lock);
> if (max_part)
> @@ -1979,22 +1981,17 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
> pr_err("nbd%d already in use\n", index);
> return -EBUSY;
> }
> - if (WARN_ON(nbd->config)) {
> - mutex_unlock(&nbd->config_lock);
> - nbd_put(nbd);
> - return -EINVAL;
> - }
> - config = nbd_alloc_config();
> - if (IS_ERR(config)) {
> +
> + ret = nbd_alloc_and_init_config(nbd);
> + if (ret) {
> mutex_unlock(&nbd->config_lock);
> nbd_put(nbd);
> pr_err("couldn't allocate config\n");
> - return PTR_ERR(config);
> + return ret;
> }
> - nbd->config = config;
> - refcount_set(&nbd->config_refs, 1);
> - set_bit(NBD_RT_BOUND, &config->runtime_flags);
>
> + config = nbd->config;
> + set_bit(NBD_RT_BOUND, &config->runtime_flags);
> ret = nbd_genl_size_set(info, nbd);
> if (ret)
> goto out;
>