From: Li Nan <[email protected]>
Li Nan (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.39.2
From: Li Nan <[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: Li Nan <[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 df1cd0f718b8..904d01796d37 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1530,17 +1530,20 @@ static int nbd_ioctl(struct block_device *bdev, blk_mode_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);
@@ -1548,7 +1551,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 gendisk *disk, blk_mode_t mode)
@@ -1567,21 +1573,17 @@ static int nbd_open(struct gendisk *disk, blk_mode_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)
@@ -1982,22 +1984,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.39.2
From: Li Nan <[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: Li Nan <[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 904d01796d37..1b9ee96d3b8a 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -396,6 +396,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);
@@ -410,13 +418,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)) {
@@ -978,12 +986,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),
@@ -1560,6 +1568,7 @@ static int nbd_alloc_and_init_config(struct nbd_device *nbd)
static int nbd_open(struct gendisk *disk, blk_mode_t mode)
{
struct nbd_device *nbd;
+ struct nbd_config *config;
int ret = 0;
mutex_lock(&nbd_index_mutex);
@@ -1572,7 +1581,9 @@ static int nbd_open(struct gendisk *disk, blk_mode_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);
@@ -1588,7 +1599,7 @@ static int nbd_open(struct gendisk *disk, blk_mode_t mode)
mutex_unlock(&nbd->config_lock);
if (max_part)
set_bit(GD_NEED_PART_SCAN, &disk->state);
- } else if (nbd_disconnected(nbd->config)) {
+ } else if (nbd_disconnected(config)) {
if (max_part)
set_bit(GD_NEED_PART_SCAN, &disk->state);
}
@@ -2197,7 +2208,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);
@@ -2205,7 +2217,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.39.2
From: Li Nan <[email protected]>
Memory reordering may occur in nbd_genl_connect(), causing config_refs
to be set to 1 while nbd->config is still empty. Opening nbd at this
time will cause null-ptr-dereference.
T1 T2
nbd_open
nbd_get_config_unlocked
nbd_genl_connect
nbd_alloc_and_init_config
//memory reordered
refcount_set(&nbd->config_refs, 1) // 2
nbd->config
->null point
nbd->config = config // 1
Fix it by adding smp barrier to guarantee the execution sequence.
Signed-off-by: Li Nan <[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 1b9ee96d3b8a..03f387f1abb5 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -398,8 +398,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;
}
@@ -1559,7 +1567,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.39.2
On Fri, Nov 17, 2023 at 12:23:13AM +0800, [email protected] wrote:
> From: Li Nan <[email protected]>
>
> Li Nan (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.39.2
>
Reviewed-by: Josef Bacik <[email protected]>
Thanks,
Josef
On Fri, 17 Nov 2023 00:23:13 +0800, [email protected] wrote:
> Li Nan (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(-)
>
> [...]
Applied, thanks!
[1/3] nbd: fold nbd config initialization into nbd_alloc_config()
commit: 1b59860540a4018e8071dc18d4893ec389506b7d
[2/3] nbd: factor out a helper to get nbd_config without holding 'config_lock'
commit: 3123ac77923341774ca3ad1196ad20bb0732bf70
[3/3] nbd: fix null-ptr-dereference while accessing 'nbd->config'
commit: c2da049f419417808466c529999170f5c3ef7d3d
Best regards,
--
Jens Axboe