Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Tue, 3 Oct 2023 12:47:50 +0200
From:   Maciej =?utf-8?Q?Wiecz=C3=B3r-Retman?= 
        <maciej.wieczor-retman@intel.com>
To:     Reinette Chatre <reinette.chatre@intel.com>
CC:     Fenghua Yu <fenghua.yu@intel.com>, Shuah Khan <shuah@kernel.org>,
        <ilpo.jarvinen@linux.intel.com>, <linux-kernel@vger.kernel.org>,
        <linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH v5 1/2] selftests/resctrl: Fix schemata write error check
Message-ID: <da3f4meb46bxnv4rkntirtiskn23gsegj4n2nnxp355kgiaen2@j62s7xfa4iql>
References: <cover.1695975327.git.maciej.wieczor-retman@intel.com>
 <f1f8f4132d6e6cdb1438186cb40c5d78b94acd5f.1695975327.git.maciej.wieczor-retman@intel.com>
 <b4fe7970-06ce-43eb-a972-eb7cad26ff34@intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <b4fe7970-06ce-43eb-a972-eb7cad26ff34@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?bFp2ncacaXeyZwdR7ICRPOUKr7zS7Abf4ENeXWm9k1oJaRnPNm6m7Qv5EY?=
 =?iso-8859-1?Q?qRVZL8l2jeIpGbatft7d/d2dV1MJuSSPQ/c0H29RXUd64i9XCibrywI3u1?=
 =?iso-8859-1?Q?Kh5/NhGLnShZNeZhQ5jaQqoorGJFDS572EaMnDd6QVMVKeL89xFcAO4Jw/?=
 =?iso-8859-1?Q?Z+1OefzwHqs4bN7pOJeeNwfxggiox3q7QKdFh2JCXEYKZ0FxODhc8EARVb?=
 =?iso-8859-1?Q?fWAw919kFcieD9G9ex4ZgTMfSe7h4XkLLeeyIqYoKt1PmzY6o9K7SaazR0?=
 =?iso-8859-1?Q?C6MaCti2fPA17CV7YlBj1gzzexiJHQT06RcOm6JaYv6IJoMrOA1Tp+ST9F?=
 =?iso-8859-1?Q?YCbEyta2YFNwVHzxJRTE+HLJvHaSaqf1VdchRhR/Oc4kvxyP+BiG1iL7oo?=
 =?iso-8859-1?Q?A6zJpsznNHs3hMETteb1Vhv57lJR8M9jlcq9YRohQb/xG9tXD4IzBG6MvR?=
 =?iso-8859-1?Q?YHDn3bw1MDDU6B8+1R6Wadqnzg+jorg3Ad6qNmBLJ7Kr5JV/6FWWMaHyDe?=
 =?iso-8859-1?Q?FRfki3oqRdgCT14G2IdggPFSlfzotTWW8qpmk/ZLBS1yQOBQ12dHZ575gd?=
 =?iso-8859-1?Q?oP1KDWVFFW49VI92nwiXeSwnl0MDKEcOV3fLxpAO5DAFBnQJOz3k2sGb+u?=
 =?iso-8859-1?Q?7WJpd+0F5ekj/B8qnK7VPw0JcwW5f+k8sZWePzqrUaIJnTWcEh4abRYRpc?=
 =?iso-8859-1?Q?tWtwoMZ54g3ukJiJIdHVH623XfPaQiV7nSCJjnCg1IUvBtvYtXIpIot7ws?=
 =?iso-8859-1?Q?jxUwVWj1E6Sh/Za3d4lO8mJ7z02+w4JtCdxW2kwsL6Z6wpBN4TcQor3A6c?=
 =?iso-8859-1?Q?aro8TrB2z+ADXSGzh2u0FWKEvnAK2iOcyJk8rnQwrNBydr3X1ycEh+JE/d?=
 =?iso-8859-1?Q?yPI8cqeLTcFbt5Y+IJlfY0+bxv75fQyDsJLH3WFu/NkDp7jLNTml55+UKk?=
 =?iso-8859-1?Q?WS5IZb9ChnZwsxD5ngcBMN0eH0QXHzbYrVaSFeZN8k1d4GsMZHLB8EzU74?=
 =?iso-8859-1?Q?TIGbqVe6B70JYdwiB08n+SrsI/l/TZl2Dassv7HsylSiw27j99exgjTiNH?=
 =?iso-8859-1?Q?JGPogcesxiPbnULpEcQVNYIeWhtHxPw5P3AMGtk8iJ+bDCEo1nnTkbNvBz?=
 =?iso-8859-1?Q?oOqpBvQWrOasZRs9q6nDVrF015HqSaCMEeGKNb9qM8nMDGsAetAot9IGy/?=
 =?iso-8859-1?Q?Jj7DEgFrSGtpMTtrQ2PMRNc0PKw1i36OjfrgdeJD6PtBNJBJ1Pb+htKDuX?=
 =?iso-8859-1?Q?0Mru3ncMz/7Ju7OcV6m4h9WLSLaAnoUTb3NpV/fKIhi0pdzdnvQpZ1872m?=
 =?iso-8859-1?Q?ysmVAH3fZMJU37Kx5O7zlnIvF5MeQ/liqaaOnl1UU8G58iV6NkYbwh7pMG?=
 =?iso-8859-1?Q?j9fIDr/AieTGjxc0tCsGITov/lsjvenV09wGbF6qCR9M6gb0HSS04vlTY4?=
 =?iso-8859-1?Q?Z/JmwyhwyZElfzlN8vVW8sHgvuyB7wbBb1ABTAguCFRL9QIQ40W8rc2MA3?=
 =?iso-8859-1?Q?Mp1x+GHZ0ysFR2yd+Yq3/X0ZEXr3n7sECncllFpQnVhVu9bpJFpAxdM0Lo?=
 =?iso-8859-1?Q?jiiBGOcots229qZ9XBt0pVwdoSHoFLQvmxuGrp8nG/zl53IzcZqqLk8A09?=
 =?iso-8859-1?Q?InQ4+Zcao3pMyFIqvHpqZPLLg5vZ1FuDaSwK7cpllABKxKC1mdfuwtl3t3?=
 =?iso-8859-1?Q?w7WT6Iq4NgVoyy8JqDE=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c1d1d273-98b1-4e0e-502f-08dbc3fe3524
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6231.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Oct 2023 10:47:58.5556
 (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: 1daghwqTw8BpLN6uLWXoISUXq+NAEiNYgxrgs09PER9xKbuxrdyWPu4Wf71RgO7Reog8sueRj11UhxYz6d0hyr2+KvTWpbQEnLxQFlyHOkE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7694
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,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 pete.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 (pete.vger.email [0.0.0.0]); Tue, 03 Oct 2023 03:48:24 -0700 (PDT)

Hello!

On 2023-09-29 at 10:04:21 -0700, Reinette Chatre wrote:
>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 */

Okay, I'll change it to the more typical error checking then. I suppose
it's not possible to write zero characters to "schema" so my " < 1"
won't be useful.

>> +		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

Thanks for pointing it out, I'll fix it for the next version.

-- 
Kind regards
Maciej Wiecz?r-Retman