Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Tue, 10 Oct 2023 08:28:25 +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 v6 1/2] selftests/resctrl: Fix schemata write error check
Message-ID: <oc3jfclnvdlg27mcmnplguho2hifz2xvl6bcowgx2oy5xgevju@g2rp3r6hrfdx>
References: <cover.1696848653.git.maciej.wieczor-retman@intel.com>
 <ef13c2395db0a688ed84f4b535b308bb6f7b71c7.1696848653.git.maciej.wieczor-retman@intel.com>
 <822a4716-4079-44d5-81d9-83c7e9bef610@intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <822a4716-4079-44d5-81d9-83c7e9bef610@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?6fT3YpxlsPl03v88lZ2pveHLGCihXF19L+x0Gmw7qboZaUchkhx3P7X1H6?=
 =?iso-8859-1?Q?MAaVs2MQ0HVnmO4Re6g9JPUmA1GrlDk6KtYDU4StNK0/v3VZH/LdSVwU3y?=
 =?iso-8859-1?Q?4wEhvEyrOIN/wF1VRgh4NtJug8FbhO9jXavq0MJoUKEAQ6xkG+Kgy+hgSe?=
 =?iso-8859-1?Q?l1mGHc9Z0LtQUPJY1wetGFK8G9g03+JON7mMy4sS/29IievprXVa4Ev1yQ?=
 =?iso-8859-1?Q?8ZOeSpgh8m9kIZNe0ivWpLY97OyuIc5oAFg6/1xx5LVLFQlaR2mpQrM+aj?=
 =?iso-8859-1?Q?b3FjRUDVexRHLyEQUiSupEJHsZdVgHLNSxNPq3oe22xhhPEjjoiPtl7ugr?=
 =?iso-8859-1?Q?2ycF6rXfG1g4oy4qlAQZtj54tYqJTvAt+u32XKK1Ma+g6JTGhDDprEDVVp?=
 =?iso-8859-1?Q?g2ix62RDrFe3JRvC+ITwV0eNvV/resYy++fqJsO8XxBk8jPdAgFe3eEvGg?=
 =?iso-8859-1?Q?Re0Eo20O1K3xz+lfvrFtPtljG+Oy0Ch6mU+WCkK83nSv9RsmWIDro/kxk2?=
 =?iso-8859-1?Q?NpN3IvPoHE6YiKXghbC/shZ71+DcGw06WwamRTssdr9em2S1kfavAGXZPN?=
 =?iso-8859-1?Q?qEQ8hpJivs0pIXdROwpUlF1cNFSgcCPwSUIcQ+8hEwUykZeyYJSEbSIFwu?=
 =?iso-8859-1?Q?4Kv15uiY8+dDLlSeRJv6g+ykAOKC0ta015mGfFw73LXmofu1N+hhbA5wqp?=
 =?iso-8859-1?Q?NKkUGGC7njZU2d+bpWab9mGW7yLxo9tgoubPSEZnn1D1Joz9dHB/WrwjoP?=
 =?iso-8859-1?Q?90jfd6LBWAAMubJVXbLK7RZnUPUuURhyHpzsMwCcCh8JZt4FMnlzqM8yeC?=
 =?iso-8859-1?Q?ekLceQYuQbC+MEulQ5pJxyn5v+t2DckzZYLQ7bH3Yf/jfkIZBeQ4dcxElw?=
 =?iso-8859-1?Q?O4RQX7qZ18e0I044Vv/1IpM3BT35nzMoO/FY+RNKidwi0GPG+Tlk8vRE0E?=
 =?iso-8859-1?Q?+7ojPV6DE1xDu4b/E1gS2K59zrugGJwK1EsbRoThzXjh369+42wtSJ0ei5?=
 =?iso-8859-1?Q?Pn3cB4tvH30sPgkdS3WjXGDoY9RkarwFaADJIw7eGer2aA5YqZSpl9UehF?=
 =?iso-8859-1?Q?k+xpycKV3XVvxO6k31PFQ8gOV1L27egIRL/Q9kN+ejyJkTzqEzvoEdi1nQ?=
 =?iso-8859-1?Q?Vq/ygZYGzB8MUdMO/Gifd3hzxgoVsKYsRU43saBxpK7XPjHc7EBW+KzJit?=
 =?iso-8859-1?Q?tqicZ1iSIaqYD7JB+AXnEA6lQ/WcR9rdOfZUTuyOh57vRpVYmGh55kkMMm?=
 =?iso-8859-1?Q?0ksA40853VixvEK3ucFzT0z3Ge6iqirLbb25nIQD9/+UJMFaHolL17a8AA?=
 =?iso-8859-1?Q?KPlBuA0Ic5bjOQzrvITbuMA6Wcmxd/D5CXxjwrC6EA2ktGo74HoJcIMTFO?=
 =?iso-8859-1?Q?A08nbC0UWIUft0aOTkNbrmXXQFtk8zy1m8ggqZLwcMD2+uSaQAsaIjEdxz?=
 =?iso-8859-1?Q?badT8GhRMyk9YUI8IBw/ZNsmV5kMov8DnMpLCTyxw918I678mVdobLJCfO?=
 =?iso-8859-1?Q?/6H4vHEbH1lFPstQims/UxF8zuEaZYOd/xFzrhm44tjSqEQkNjOrAz6EIL?=
 =?iso-8859-1?Q?leLfSVtihvaGHwW8nsHbsA4dCU8WqZSh/UTYR6DLS4mUDs4aBTbaK1u1xi?=
 =?iso-8859-1?Q?mjdQV2zaOBfTdRNTGSk3LIdN4JBLeFPSYRGR9OxQRpKlItUZ/P1521T5YJ?=
 =?iso-8859-1?Q?un0IVkvN4QjhN4jJXHs=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 323fef1c-aadc-4a88-c506-08dbc95a1e6d
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6231.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Oct 2023 06:28:29.9273
 (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: FaWn4NXzVCSnEbag3fISWJNHWVKXNbDwlMi5DBvZsnR9SmBlz6wKDAGqFvlRiHhlhFKbliz3Kk7WAmzc19LlnIwOX3y23sKXbe8cARba+i0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4564
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=2.7 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS,
	URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.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 (agentk.vger.email [0.0.0.0]); Mon, 09 Oct 2023 23:29:22 -0700 (PDT)
X-Spam-Level: **

On 2023-10-09 at 11:55:16 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 10/9/2023 3:57 AM, Maciej Wieczor-Retman wrote:
>> Writing bitmasks to the schemata can fail when the bitmask doesn't
>> adhere to constraints defined by what a particular CPU supports.
>> Some example of constraints are max length or having contiguous bits.
>> The driver should properly return errors when any rule concerning
>> bitmask format is broken.
>> 
>> Resctrl FS returns error codes from fprintf() only when fclose() is
>> called. Current error checking scheme allows invalid bitmasks to be
>> written into schemata file and the selftest doesn't notice because the
>> fclose() error code isn't checked.
>> 
>> Substitute fopen(), flose() and fprintf() with open(), close() and
>> write() to avoid error code buffering between fprintf() and fclose().
>> 
>> Remove newline character from the schema string after writing it to
>> the schemata file so it prints correctly before function return.
>> 
>> Pass the string generated with strerror() to the "reason" buffer so
>> the error message is more verbose. Extend "reason" buffer so it can hold
>> longer messages.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> Reviewed-by: Ilpo J?rvinen <ilpo.jarvinen@linux.intel.com>
>> ---
>> Changelog v6:
>> - Align schema_len error checking with typical snprintf format.
>>   (Reinette)
>> - Initialize schema string for early return eventuality. (Reinette)
>> 
>> Changelog v5:
>> - Add Ilpo's reviewed-by tag.
>> - Fix wrong open() error checking. (Reinette)
>> - Add error checking to schema_len variable.
>> 
>> Changelog v4:
>> - Unify error checking between open() and write(). (Reinette)
>> - Add fcntl.h for glibc backward compatiblitiy. (Reinette)
>> 
>> Changelog v3:
>> - Rename fp to fd. (Ilpo)
>> - Remove strlen, strcspn and just use the snprintf value instead. (Ilpo)
>> 
>> Changelog v2:
>> - Rewrite patch message.
>> - Double "reason" buffer size to fit longer error explanation.
>> - Redo file interactions with syscalls instead of stdio functions.
>> 
>>  tools/testing/selftests/resctrl/resctrlfs.c | 36 +++++++++++++--------
>>  1 file changed, 23 insertions(+), 13 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>> index 3a8111362d26..648f9ec8b355 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], reason[128], schema[1024] = {};
>> +	int resource_id, fd, schema_len = -1, 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 < 0 || schema_len >= sizeof(schema)) {
>> +		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",
>
>
>As changelog states, the newline is removed from schema to
>ensure it is printed correctly. Note that this is not done when an
>error is encountered during open() or write() so when an error is
>encountered in these places then the print does not look as intended.
>
>I think a new goto label inserted just before the newline removal
>should be sufficient, with the open() and write() error paths jumping
>to it.
>
>With that addressed:
>
>Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
>
>Reinette

Thanks, I'll make the correction and resend.

-- 
Kind regards
Maciej Wiecz?r-Retman