All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] scsi: target: fixup incorrect use of 'cpumask_t'
@ 2022-05-13  6:27 mingzhe.zou
  2022-05-14 16:30 ` Mike Christie
  0 siblings, 1 reply; 2+ messages in thread
From: mingzhe.zou @ 2022-05-13  6:27 UTC (permalink / raw)
  To: torvalds, zgrieee, martin.petersen, michael.christie
  Cc: linux-kernel, linux-scsi, target-devel, dongsheng.yang, zoumingzhe

From: mingzhe <mingzhe.zou@easystack.cn>

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 <zgrieee@gmail.com>
Signed-off-by: mingzhe <mingzhe.zou@easystack.cn>
---
 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


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] scsi: target: fixup incorrect use of 'cpumask_t'
  2022-05-13  6:27 [PATCH] scsi: target: fixup incorrect use of 'cpumask_t' mingzhe.zou
@ 2022-05-14 16:30 ` Mike Christie
  0 siblings, 0 replies; 2+ messages in thread
From: Mike Christie @ 2022-05-14 16:30 UTC (permalink / raw)
  To: mingzhe.zou, torvalds, zgrieee, martin.petersen
  Cc: linux-kernel, linux-scsi, target-devel, dongsheng.yang, zoumingzhe

On 5/13/22 1:27 AM, mingzhe.zou@easystack.cn wrote:
> From: mingzhe <mingzhe.zou@easystack.cn>
> 
> 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 <zgrieee@gmail.com>
> Signed-off-by: mingzhe <mingzhe.zou@easystack.cn>
> ---
>  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;
>  }
>  


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2022-05-14 16:30 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-13  6:27 [PATCH] scsi: target: fixup incorrect use of 'cpumask_t' mingzhe.zou
2022-05-14 16:30 ` Mike Christie

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.