Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <b4fe7970-06ce-43eb-a972-eb7cad26ff34@intel.com>
Date:   Fri, 29 Sep 2023 10:04:21 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.15.1
Subject: Re: [PATCH v5 1/2] selftests/resctrl: Fix schemata write error check
Content-Language: en-US
To:     Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Shuah Khan <shuah@kernel.org>
CC:     <ilpo.jarvinen@linux.intel.com>, <linux-kernel@vger.kernel.org>,
        <linux-kselftest@vger.kernel.org>
References: <cover.1695975327.git.maciej.wieczor-retman@intel.com>
 <f1f8f4132d6e6cdb1438186cb40c5d78b94acd5f.1695975327.git.maciej.wieczor-retman@intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <f1f8f4132d6e6cdb1438186cb40c5d78b94acd5f.1695975327.git.maciej.wieczor-retman@intel.com>
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?cVVjQWEvRzB3QUp0ZUpkY3ZzcVBtbnZnSm9kbkpoUHJMZk0zOWtyNGxibXZs?=
 =?utf-8?B?MHVENHpZbktUeGthcVk0RlVZaTFpYzlnMTg4ZnpJbk5aQzdRQUJpMzZ2TVR0?=
 =?utf-8?B?YnlTdUJhWGZXQmhoajV1Tm9aWWp6aEN5T09oZTNLM21adGE5SWsxV1AwTE9K?=
 =?utf-8?B?ZTdtVEUySjNZTHJCSFpmbzVKdkFuOHQyZXhjMGhmdlliaGZKQW9wSTY2NEpv?=
 =?utf-8?B?Q29qY3RhMGxZN3IxRm9BU2pnbUEvUmZObzVlQWN1WUhRWFROc09RVXQ2aGRn?=
 =?utf-8?B?Q0ZVUzhMZnV2SXJydklGdU9yaW5CcDB3RXZucmEyNHVySEJ0V2NuOGI3YUxE?=
 =?utf-8?B?VFMwVEJoL2NKTWN1WjBjeWRDZmFYL0VlTFlEZzl6bEV3NUhIb1FCZ0FGbnpC?=
 =?utf-8?B?OW1vUEx2elBJeFJhd01kM3QxYXdiTkcxNUptdFZVb29pd1ZzYWtaMzlKUHMz?=
 =?utf-8?B?aHdHVjFWNGpUUExubnowais1NWpWUUtGZVdKYitIc3lUZ1k2RFYrUFRBc3Zw?=
 =?utf-8?B?Y3BqWkpKc0NLTWE5b1M3UnNHbHhUeHYyNlhnYVp1SmdpZ1FQdWhKRlhPUWlD?=
 =?utf-8?B?K05FTnhud2JnNGVKWVlUWDdFRTduZkRyM2FSM0RmRGpzS2oyNXNvMnBUKzEw?=
 =?utf-8?B?UEE1d09IeGFKSURjUVF2NUg0Q3BiZjNkWDl4YjJtdzFNSmpaYkZtUGNkWVNu?=
 =?utf-8?B?aWkwWTZyK2VsY3o2M3VvUzBCVXFvcWcrM200VUxVUmNEWldPZEpCQXNzNjd6?=
 =?utf-8?B?TGdHdXZaL3VYd29rdDhib1V4cVFTZDBEbkhZWG1qOFlJckhjWVY3MVUxcDB0?=
 =?utf-8?B?UnJsSExJaUs2Yk40YktDUWd4cFV2QnBPd1poVC9NQ09iT3RYcjRENDEwY1pW?=
 =?utf-8?B?UVpZOS8vWUE1VTNybWlVUVBtTFRyckhYNWNqY3dFZ0VTY3dUMXZnRXdhZ2RK?=
 =?utf-8?B?VVBKRkNPVUljODF0dllBN1RBN0x4NlIvdkZWdzJIenZiUVNSZ0RLVDZ3RFk4?=
 =?utf-8?B?Q0ZRMnRoRG1QTWN1ZnU0SXBEZUkxWjFGUzFCV1Z0ODc0azhxazRqQ3J5WFM1?=
 =?utf-8?B?ZFdWcVV4R25jWDg1T0ZGdEpZZFd0Rlo1RVlOQ0drdlp5eEF1UUlnd3FnZW1O?=
 =?utf-8?B?Qi8vN2Y0YVhSSG9KQ2tZRnFTTXQ5dVBFbGFMZ3VKd3hVemFLRWpObkgzZU8y?=
 =?utf-8?B?cDQ0OXVqVFVsV29iUkZBUzgycnZRa20xUlpRMGdqeEoweG03RURWQjNkY0pL?=
 =?utf-8?B?ZS9rZzlqVmdYTmtTSzVXeG5vbVAwdFR1TEsrVk1jZW1TVTNRUm1SamR2Zndt?=
 =?utf-8?B?U09HWUx5VjdTLzFFS1hDWFFOY3YvZEVaZXZCNkJSa1U2TDJuU1dMdEVwMlVR?=
 =?utf-8?B?d2FNR0xIczBXd1dOQzhrejY3bzNkMnpiWEtBWVRHNytqRmRBNy9oNkhhaFox?=
 =?utf-8?B?SllxZHhHb1dmSE5maHl5MGxVM0dBYXVlTHM1Z2tScEdQeDlZYzd2Y1FMVkhQ?=
 =?utf-8?B?RmRqd1dRYkkza292L3dUUjhwbmh0UTYxS09pM0hGZU9DbmtLY096VVBOM1hq?=
 =?utf-8?B?NURILzRQMHNpNkcvTjYxWm4xVm5JZ1VCVm9md0xQbXlBbXdycFpwSnBLT1RF?=
 =?utf-8?B?ZXk1TmFWUXFYWUJyQzhsaHBzU1JrUmU0ODFDOEhGeUtoL1FKWGNxSk55VnJi?=
 =?utf-8?B?Z2N6S0dXN09nMkRHRlkvOUtrYnphMnhsTGNJc1A4c3ZYWVYxNk44OWs4QVdZ?=
 =?utf-8?B?akYwQUNSVCtRWmg1KzNkTXFJUlZwYjdCeDk1VjhOUnI1ZG5ma0ZGZ2VQU0dt?=
 =?utf-8?B?WDBtTXJzWUpQZzFBT2NQWWtmRUdDUERlWTVNeVVaOW1qZ05IS1Z0T1QwdFF0?=
 =?utf-8?B?ciszZjlHNDE4MDVwWHU5MXdtY1VEMnpmOVFjVzZlYnVMU1JxK0dnNVMrQ2Jq?=
 =?utf-8?B?TkZYdEc0aHBYYVE0ZzNMbXc4RnNlMWxiRFBxNDRacFhuNzJmSzNLZDNYWWJN?=
 =?utf-8?B?OEo1b1JBTnI1djVsRzhvQXArSWN3MUtvQ2lyUVl5ZkpFSWl4cTIxY3NzUzk4?=
 =?utf-8?B?WmdvcDB6eWhxSFZrU0ZhOWpVSzZaeUNvTWhmZmt6eEtpM1BqaVlRb3BwTVJ0?=
 =?utf-8?B?QXZ4alBuNERjQnJCanFlNi9EYzFLTnoybE52UitPMk9pOFdUTUo5bjRpWWNv?=
 =?utf-8?B?aVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 71f997fe-b686-44ab-0ef4-08dbc10e21c0
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Sep 2023 17:04:24.4199
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 9mSuibBNo07w49UqeYWtaTkjNYahAE5g8/CPPZrtdEG8W4A+DeNtKQ83IKPcth76TtlM1bD5CAVnLwbmRh/N6F4BTtQ0YPQQfNKBp5XMeUM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB8044
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.1 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Fri, 29 Sep 2023 10:04:43 -0700 (PDT)

Hi Maciej,

On 9/29/2023 1:21 AM, Maciej Wieczor-Retman wrote:
...

> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 3a8111362d26..342a3dbcdbb6 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -8,6 +8,7 @@
>   *    Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>,
>   *    Fenghua Yu <fenghua.yu@intel.com>
>   */
> +#include <fcntl.h>
>  #include <limits.h>
>  
>  #include "resctrl.h"
> @@ -490,9 +491,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
>   */
>  int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
>  {
> -	char controlgroup[1024], schema[1024], reason[64];
> -	int resource_id, ret = 0;
> -	FILE *fp;
> +	char controlgroup[1024], schema[1024], reason[128];
> +	int resource_id, fd, schema_len = 0, ret = 0;
>  
>  	if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) &&
>  	    strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) &&
> @@ -520,27 +520,37 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
>  
>  	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) ||
>  	    !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
> -		sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata);
> +		schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n",
> +				      "L3:", resource_id, '=', schemata);
>  	if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) ||
>  	    !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)))
> -		sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata);
> +		schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n",
> +				      "MB:", resource_id, '=', schemata);
> +	if (schema_len < 1) {

I find that this complicates the code since this is not the typical snprintf()
error checking (0 is a valid snprintf() return). I think it will make the code
easier to understand if it sticks to snprintf() error checking and initialize
schema_len to match. What I mean with this is something like this:

	int schema_len = -1;

	if (...)
		schema_len = snprintf(...);

	if (schema_len < 0 || schema_len >= sizeof(schema))
		/* error handling */

> +		snprintf(reason, sizeof(reason),
> +			 "snprintf() failed with return value : %d", schema_len);
> +		ret = -1;
> +		goto out;
> +	}
>  
> -	fp = fopen(controlgroup, "w");
> -	if (!fp) {
> -		sprintf(reason, "Failed to open control group");
> +	fd = open(controlgroup, O_WRONLY);
> +	if (fd < 0) {
> +		snprintf(reason, sizeof(reason),
> +			 "open() failed : %s", strerror(errno));
>  		ret = -1;
>  
>  		goto out;
>  	}
> -
> -	if (fprintf(fp, "%s\n", schema) < 0) {
> -		sprintf(reason, "Failed to write schemata in control group");
> -		fclose(fp);
> +	if (write(fd, schema, schema_len) < 0) {
> +		snprintf(reason, sizeof(reason),
> +			 "write() failed : %s", strerror(errno));
> +		close(fd);
>  		ret = -1;
>  
>  		goto out;
>  	}
> -	fclose(fp);
> +	close(fd);
> +	schema[schema_len - 1] = 0;
>  
>  out:
>  	ksft_print_msg("Write schema \"%s\" to resctrl FS%s%s\n",

Also please note that _if_ there is an early exit then uninitialized schema
will be printed. Maybe this also needs a schema[1024] = {} ?

Reinette