From: mingzhe <[email protected]>
In commit d72d827f2f26, I used 'cpumask_t' incorrectly.
```
void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
{
int ord, cpu;
cpumask_t conn_allowed_cpumask;
......
}
static ssize_t lio_target_wwn_cpus_allowed_list_store(
struct config_item *item, const char *page, size_t count)
{
int ret;
char *orig;
cpumask_t new_allowed_cpumask;
......
}
```
So, that the correct pattern should be as follows:
```
cpumask_var_t mask;
if (!alloc_cpumask_var(&mask, GFP_KERNEL))
return -ENOMEM;
... use 'mask' here as a ...
free_cpumask_var(mask);
```
Reported-by: Test Bot <[email protected]>
Signed-off-by: mingzhe <[email protected]>
---
drivers/target/iscsi/iscsi_target.c | 32 ++++++++++++++------
drivers/target/iscsi/iscsi_target_configfs.c | 20 ++++++++----
2 files changed, 36 insertions(+), 16 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 6fe6a6bab3f4..3f9f5b8879fe 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -3596,10 +3596,7 @@ static int iscsit_send_reject(
void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
{
int ord, cpu;
- cpumask_t conn_allowed_cpumask;
-
- cpumask_and(&conn_allowed_cpumask, iscsit_global->allowed_cpumask,
- cpu_online_mask);
+ cpumask_var_t conn_allowed_cpumask;
/*
* bitmap_id is assigned from iscsit_global->ts_bitmap from
@@ -3609,13 +3606,28 @@ void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
* iSCSI connection's RX/TX threads will be scheduled to
* execute upon.
*/
- cpumask_clear(conn->conn_cpumask);
- ord = conn->bitmap_id % cpumask_weight(&conn_allowed_cpumask);
- for_each_cpu(cpu, &conn_allowed_cpumask) {
- if (ord-- == 0) {
- cpumask_set_cpu(cpu, conn->conn_cpumask);
- return;
+ if (!alloc_cpumask_var(&conn_allowed_cpumask, GFP_KERNEL)) {
+ ord = conn->bitmap_id % cpumask_weight(cpu_online_mask);
+ for_each_online_cpu(cpu) {
+ if (ord-- == 0) {
+ cpumask_set_cpu(cpu, conn->conn_cpumask);
+ return;
+ }
+ }
+ } else {
+ cpumask_and(conn_allowed_cpumask, iscsit_global->allowed_cpumask,
+ cpu_online_mask);
+
+ cpumask_clear(conn->conn_cpumask);
+ ord = conn->bitmap_id % cpumask_weight(conn_allowed_cpumask);
+ for_each_cpu(cpu, conn_allowed_cpumask) {
+ if (ord-- == 0) {
+ cpumask_set_cpu(cpu, conn->conn_cpumask);
+ free_cpumask_var(conn_allowed_cpumask);
+ return;
+ }
}
+ free_cpumask_var(conn_allowed_cpumask);
}
/*
* This should never be reached..
diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
index 0cedcfe207b5..ae9319934cd0 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1139,20 +1139,28 @@ static ssize_t lio_target_wwn_cpus_allowed_list_store(
{
int ret;
char *orig;
- cpumask_t new_allowed_cpumask;
+ cpumask_var_t new_allowed_cpumask;
+
+ if (!alloc_cpumask_var(&new_allowed_cpumask, GFP_KERNEL))
+ return -ENOMEM;
orig = kstrdup(page, GFP_KERNEL);
- if (!orig)
+ if (!orig) {
+ free_cpumask_var(new_allowed_cpumask);
return -ENOMEM;
+ }
- cpumask_clear(&new_allowed_cpumask);
- ret = cpulist_parse(orig, &new_allowed_cpumask);
+ cpumask_clear(new_allowed_cpumask);
+ ret = cpulist_parse(orig, new_allowed_cpumask);
kfree(orig);
- if (ret != 0)
+ if (ret != 0) {
+ free_cpumask_var(new_allowed_cpumask);
return ret;
+ }
- cpumask_copy(iscsit_global->allowed_cpumask, &new_allowed_cpumask);
+ cpumask_copy(iscsit_global->allowed_cpumask, new_allowed_cpumask);
+ free_cpumask_var(new_allowed_cpumask);
return count;
}
--
2.17.1
On 5/13/22 1:27 AM, [email protected] wrote:
> From: mingzhe <[email protected]>
>
> In commit d72d827f2f26, I used 'cpumask_t' incorrectly.
> ```
> void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
> {
> int ord, cpu;
> cpumask_t conn_allowed_cpumask;
> ......
> }
>
> static ssize_t lio_target_wwn_cpus_allowed_list_store(
> struct config_item *item, const char *page, size_t count)
> {
> int ret;
> char *orig;
> cpumask_t new_allowed_cpumask;
> ......
> }
> ```
>
> So, that the correct pattern should be as follows:
> ```
> cpumask_var_t mask;
>
> if (!alloc_cpumask_var(&mask, GFP_KERNEL))
> return -ENOMEM;
> ... use 'mask' here as a ...
> free_cpumask_var(mask);
> ```
>
Add a:
Fixes: d72d827f2f26 ("scsi: target: Add iscsi/cpus_allowed_list in configfs")
to make it easier for people to pick up.
> Reported-by: Test Bot <[email protected]>
> Signed-off-by: mingzhe <[email protected]>
> ---
> drivers/target/iscsi/iscsi_target.c | 32 ++++++++++++++------
> drivers/target/iscsi/iscsi_target_configfs.c | 20 ++++++++----
> 2 files changed, 36 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 6fe6a6bab3f4..3f9f5b8879fe 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -3596,10 +3596,7 @@ static int iscsit_send_reject(
> void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
> {
> int ord, cpu;
> - cpumask_t conn_allowed_cpumask;
> -
> - cpumask_and(&conn_allowed_cpumask, iscsit_global->allowed_cpumask,
> - cpu_online_mask);
> + cpumask_var_t conn_allowed_cpumask;
>
> /*
> * bitmap_id is assigned from iscsit_global->ts_bitmap from
> @@ -3609,13 +3606,28 @@ void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
> * iSCSI connection's RX/TX threads will be scheduled to
> * execute upon.
> */
> - cpumask_clear(conn->conn_cpumask);
> - ord = conn->bitmap_id % cpumask_weight(&conn_allowed_cpumask);
> - for_each_cpu(cpu, &conn_allowed_cpumask) {
> - if (ord-- == 0) {
> - cpumask_set_cpu(cpu, conn->conn_cpumask);
> - return;
> + if (!alloc_cpumask_var(&conn_allowed_cpumask, GFP_KERNEL)) {
Do a zalloc_cpumask_var to make sure it's all initialized.
> + ord = conn->bitmap_id % cpumask_weight(cpu_online_mask);
> + for_each_online_cpu(cpu) {
> + if (ord-- == 0) {
> + cpumask_set_cpu(cpu, conn->conn_cpumask);
> + return;
> + }
> + }
> + } else {
> + cpumask_and(conn_allowed_cpumask, iscsit_global->allowed_cpumask,
> + cpu_online_mask);
> +
> + cpumask_clear(conn->conn_cpumask);
> + ord = conn->bitmap_id % cpumask_weight(conn_allowed_cpumask);
> + for_each_cpu(cpu, conn_allowed_cpumask) {
> + if (ord-- == 0) {
> + cpumask_set_cpu(cpu, conn->conn_cpumask);
> + free_cpumask_var(conn_allowed_cpumask);
> + return;
> + }
> }
> + free_cpumask_var(conn_allowed_cpumask);
> }
> /*
> * This should never be reached..
> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> index 0cedcfe207b5..ae9319934cd0 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -1139,20 +1139,28 @@ static ssize_t lio_target_wwn_cpus_allowed_list_store(
> {
> int ret;
> char *orig;
> - cpumask_t new_allowed_cpumask;
> + cpumask_var_t new_allowed_cpumask;
> +
> + if (!alloc_cpumask_var(&new_allowed_cpumask, GFP_KERNEL))
> + return -ENOMEM;
You can do zalloc_cpumask_var and then drop the clear call below.
>
> orig = kstrdup(page, GFP_KERNEL);
> - if (!orig)
> + if (!orig) {
> + free_cpumask_var(new_allowed_cpumask);
> return -ENOMEM;
Just add a goto at the bottom before free_cpumask_var then have this
and the other failure path use it. Make sure to also fix up the
return code (set count to -ENOMEM/ret or use ret at the bottom and
set count to ret).
> + }
>
> - cpumask_clear(&new_allowed_cpumask);
> - ret = cpulist_parse(orig, &new_allowed_cpumask);
> + cpumask_clear(new_allowed_cpumask);
> + ret = cpulist_parse(orig, new_allowed_cpumask);
>
> kfree(orig);
> - if (ret != 0)
> + if (ret != 0) {
> + free_cpumask_var(new_allowed_cpumask);
> return ret;
> + }
>
> - cpumask_copy(iscsit_global->allowed_cpumask, &new_allowed_cpumask);
> + cpumask_copy(iscsit_global->allowed_cpumask, new_allowed_cpumask);
> + free_cpumask_var(new_allowed_cpumask);
> return count;
> }
>