Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <64f16117-beb1-def1-5017-c5006c362070@oracle.com>
Date:   Sat, 14 May 2022 11:30:06 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH] scsi: target: fixup incorrect use of 'cpumask_t'
Content-Language: en-US
To:     mingzhe.zou@easystack.cn, torvalds@linux-foundation.org,
        zgrieee@gmail.com, martin.petersen@oracle.com
Cc:     linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org,
        target-devel@vger.kernel.org, dongsheng.yang@easystack.cn,
        zoumingzhe@qq.com
References: <20220513062745.3245-1-mingzhe.zou@easystack.cn>
From:   Mike Christie <michael.christie@oracle.com>
In-Reply-To: <20220513062745.3245-1-mingzhe.zou@easystack.cn>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WDFNelloK0VaM3piTEVFb1FiOSs1UWFUM2F4Q2ZSRjJRMURiL2JPZDBTRElx?=
 =?utf-8?B?QUdMcTV0RXMvNnBUZ0YrdEs2bi9MdFV6NEJzREIwK0IwZ2ZFOVNlTm1RSHp1?=
 =?utf-8?B?Wk9sNWNsOVhLc1pBTFpmVWVkTk14a3JuQW9VQ0JHb1NiemFEU0VQdWc5bTdS?=
 =?utf-8?B?elkyb0NoQ2N0a2tldnVEcXB0YlFlQXMzMVM5cWVpMHpqUVJaeFJuY3R6WWQ0?=
 =?utf-8?B?aUhETzFWd3Y3NlF4L0ZLMEdjajlzcDJQbVJxQ2tDQXBCNVc3ZTdzWlBwQUtT?=
 =?utf-8?B?bWM0bGVjZXVPa24yZlhpRFh0WnFrVGE4L3BaLytQOEw5RzRFbGtEYk9NU0hD?=
 =?utf-8?B?N283UjFIYTRXVFEvVFN5OGxxMVVWYjlOOFRDSlE1V1pra0VkeUN2cFExby9t?=
 =?utf-8?B?TVRldXhvZ3BuYkg3OWQ3Q0w5dFpxVm9KYnpWUzVTdHdIUlZXajk0Sys3WGhR?=
 =?utf-8?B?MVlsVjFaOEpEQlhZbEFMTS9pWUNvcGFjaVdLT0Q1Q3N3Z2Y4M2MrSnJpenNa?=
 =?utf-8?B?RWdFdHJEbnJ1VkdhRG05VUlnWXhGL0Q3YlRpV1M3M0ZlNjY5WHpNRnFUZ1ZO?=
 =?utf-8?B?RmNyMEQxOHA1RGdQZ1F1aHErTW9NNjBPT0VXYnl6QzRQTW9zL1NkYzE0aFl1?=
 =?utf-8?B?NGJXZ0NNMXU2bWwwemNvODdpMWx1UU5tYStLMG0xejh3ZDFIODlLRXV2VmVk?=
 =?utf-8?B?TUFVMU0zRTBlaVNleExhbzNSZGNndDg1TEZLc05ta0JRWTVjazI0RkRiSXJE?=
 =?utf-8?B?YlhWM0pkckdDdm1BTUNBQkdOUE4yNUZDNUF1VGlCVytWWjhidDRQbFVVUTdo?=
 =?utf-8?B?SW16TXF2TW5US0JSRnorZElXcWs0UnByWFgweHNzb2JFa3lUV2JRcDRhTE1H?=
 =?utf-8?B?UzRyNGpsMzFvdDNZMGtiWmpIeU1TRmMvZFdJeEN3RG50eDdJRG9Ocmk0MlBR?=
 =?utf-8?B?ZEhTZTdUS0lzSjhPSG5PUm13L1duV1VkRzhMQ1I1VnFFRUYzZW5ESGNHTmNs?=
 =?utf-8?B?b01LVHU3VnhVdUV4ZzlnN2YydkJqaS90VzZRTVd0V3pybDAzQms2YVlDeVhW?=
 =?utf-8?B?c1Z4c240bGJ3YXh1bkVsNG53dUo3UTJJZVBSc3Q3ZDRWT3g0TDVhZk9ZY1hN?=
 =?utf-8?B?a0Nvcm5xSkV5cThXajdRa3dkVk81aG5nQ3pKeHltV2laK2F5NUJQeFV3R1Fn?=
 =?utf-8?B?NXJYVDlKbGZHQUhDMHBPUXFnc2FCY0IxUWlxY0RzL1VlUkpubGNRZUhiYnRV?=
 =?utf-8?B?MmJUeXlPYmFPTElPSG5ZOUF3V05FRG85YkF4TDdtaDlKK1BjTjFHRjlSUTlM?=
 =?utf-8?B?TWtBWGU5Z0tWK0dDbXRNT0NEVnpreVVFMHQyc2JaYjh0d1dCaE81RVNtNjdP?=
 =?utf-8?B?QmpuRWhDWjQ4eXVnNVRjeFhlK1pSNVY2VU0zdVZCNUttL1EwN3VUSk5yZWE3?=
 =?utf-8?B?SFF4bjR1NzhmM3ZMOHpDSHFlRWxHbER6Vy9BdjBQY2tic1JSb2pmdmlFRHdp?=
 =?utf-8?B?QndBTFlDSnBlTTFia2QyUDBpS1pTUUZja0hkbUpNa3EvcW55K0xmaGJKSzVq?=
 =?utf-8?B?YWRveDFmcjBmTWljOTFTR0VwamtRMi9za2FsU0cvK0NZb1ZFcmpodktMQmpW?=
 =?utf-8?B?TDc3WEFwcFAvbHU1R2NLTGI0cWdybEdEZDhtWFRCSVkzOGFvUDR2YVdINUJz?=
 =?utf-8?B?U084NHhtNnN3alM1czVIa1VwbUgwMjc4NXhQZ2dmSDFCZ0c4ZWNHN0NOY0Ey?=
 =?utf-8?B?NTRDenJMcXNyNy9ma2tyUnNvb3RHMnkvT1Y0OGhYUE5ncUpxditJa0lob293?=
 =?utf-8?B?V2kvWkszdlFYODJudW5yQ0htWTVkWjJNMjNtZmZhUEVoZjZpdE9IVXA3ZWFD?=
 =?utf-8?B?a3RpSWp3VngvclN0MFU3bm9uTDh1ZmUwYzB3TngzaU5UZUlIVTVYU2VzSnVx?=
 =?utf-8?B?UnJEUCs1SGkwL085YzFQQUk4M3lMczlkTU54eDBqWTlITjRGS2lkay9qR3Zn?=
 =?utf-8?B?UmRNcjNFaU95aWQvemFEQUZiZk9vOHRReWNKQy94T05EcWc5Y2RkaXoxZFFu?=
 =?utf-8?B?QUdVeHIyWm9LNklsWnNTWUJ2UFh6Nnh1TGVPRUtybWRSdExjL2UxaHZRTzlk?=
 =?utf-8?B?b2pMNzd6Wnk5YzVhSzI4WSs2VHYvdjdXa1FXcCtGUlJkQ0YzcFVrV0ZteEhx?=
 =?utf-8?B?T0trZHhLZXpybkIvbHNPZHdhcFlhY1pYN3N2SllzR0pVNHlnM3c2aWE5SVIz?=
 =?utf-8?B?V0VTa3NjVndYNEpZblZSejlHemhIZlRvcUF2TTl5RWhsMXcyM0twSlZsb3Vi?=
 =?utf-8?B?SkFVVjJDVjROdFlXQUtReXBxVXAvdk5Halc1VFc3R1dhdHBqVlMvUVYvTTQ4?=
 =?utf-8?Q?9yYkVsXSGRQsdPGE=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8277f5da-ed9c-46b8-4cad-08da35c70203
X-MS-Exchange-CrossTenant-AuthSource: DM5PR10MB1466.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 May 2022 16:30:07.8005
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: en6cXhNJxqRMoSpoYXBOAFF/2pBxkx83RAkbMnJfd9FWLQFEv6ZrAnN3OKGYKNFwLRnltxOQWzUQnR3Hg3O+vjY6qiHsRTqOAPgV9lnQZPY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB3178
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-05-14_02:2022-05-13,2022-05-14 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0
 mlxlogscore=999 spamscore=0 suspectscore=0 malwarescore=0 mlxscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2205140087
X-Proofpoint-GUID: X9h4nG8q8LyzgYJzdpK40L0Db0Jd-Wt5
X-Proofpoint-ORIG-GUID: X9h4nG8q8LyzgYJzdpK40L0Db0Jd-Wt5
X-Spam-Status: No, score=-3.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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;
>  }
>