Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <322fb9ff-f3f4-45df-9b3f-524125ca6101@intel.com>
Date:   Wed, 27 Sep 2023 15:15:06 -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 v4 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.1695369120.git.maciej.wieczor-retman@intel.com>
 <5e17ed39ffb8d6bd530c057aa04e3ffb997573a9.1695369120.git.maciej.wieczor-retman@intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <5e17ed39ffb8d6bd530c057aa04e3ffb997573a9.1695369120.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?VTVXenFVd09IMDZ5Y0JqdU5kdWlvTWZNOE1RTjFRSTdoS0xBRW8wZTRoMHo0?=
 =?utf-8?B?aWxkNXNGK01UNVNhSFVJU05aeHNocHlXQUYzUDFsc1liSm4rOE5PY0hQSjJu?=
 =?utf-8?B?RnNlekdUWk5TWkprRVdiYTBFRzE4eGU5VzJUVTNlakNRWHVFRXQwVEl3bmdv?=
 =?utf-8?B?ZTFQd3dMOGMwY0dEbTlzYkd5bmtVTUJBcU55TUVGN1MwdW1LQ0dQdEJzYVBs?=
 =?utf-8?B?b2U1NkdHeDNVaXB3OUVYZmRwK0E1QVJjelZEakVVN1lXZEZKTzFjUlVYaTJT?=
 =?utf-8?B?dEdrMU9DQlN3d1paOWFmU1QzOWVBOStRdTBTbmx4U0luSW02andSMi9DZ0hV?=
 =?utf-8?B?aDFzZWRscW9IZWN3amQrOEJSMm5sUzA0TWtKVVJlYStyWUZ4WDkwMEcyMkov?=
 =?utf-8?B?ank1SEl1N1ljZ3VqZkdlQTVHRTEyTmU4YXZzQXZoaFlJRzd6SVVlSGw0VmNu?=
 =?utf-8?B?dStHdlZFOGljbDd5M2U3alRidzlWZ3FnRkhJTThVSVFyYzk1cUFjaFNPd1pv?=
 =?utf-8?B?VW1MdmdnZnhRemppVXBWQTJTWDIvcWl4VGxkelJIeDJHdk9CNXdjNVlQZFVN?=
 =?utf-8?B?QUYwcjNnRW5pYVpjc21SbkJ5eTljUXRUYjFpSUVtOEkvNk5SbVkzdExjcVlM?=
 =?utf-8?B?UmhIOGhZT2cxRTV2MWhRUk1VTlN3Y3lIa3Y4cEZZK210TjNueVNvSWdtRjhX?=
 =?utf-8?B?YzBBWUN6TUhOZEJMdVBxK1VqSlBXMGpMZEgydzl1dm5scnNFa2lWUmRDaEVR?=
 =?utf-8?B?aitVSklrN0RsRDkvanZxeVI1bWoxUUVkbUxnVFZRRkc0d2lrY3BqWjJPMmpE?=
 =?utf-8?B?cXdDeDBLb3JNUEV1bE9VSWpYbTVwb09rZU1UWW13Y2NST0xxV0VCSmE4R1hO?=
 =?utf-8?B?NzQ1bDNtVkFMY2ZKT2xwTUZBZy92NFNZVmNibURENUxsUEFMNFhkczBpaUk0?=
 =?utf-8?B?UHNLUFhPZHZMMlNsYlkwNVp4Y2Q1ZjBkZ2RscTBPbndnNmkrRWxYWEJHTEtG?=
 =?utf-8?B?Wk1ydFB1QXNFeHVsZVFMSUhSNSsxSlZDMmV3ZktTYlg0SERNSXZMRU5NcVlo?=
 =?utf-8?B?S3lmTEZXc0hwMDNSemNXRTFFU0ZHMld5WnJkVW5ibGVTRDJyby9KT2Y1L1pK?=
 =?utf-8?B?Y2tuRVkwVDYxQzBaenFZdVk0Y0xxcFBVU1FwSnpJSGE1dnVHd1ZRRW1kNDd3?=
 =?utf-8?B?enBYVStVbldYUkRqVGRMU21xOU9acG12VzRaTktuc1pWR2Z2RW95ZDRtQ2Z6?=
 =?utf-8?B?UFdTbnpJcUoycWZxeDAxYWcxZ1JVRWdyV2xJNkR2SWFpbkFjZ05kUngrS0U2?=
 =?utf-8?B?L09aSG9rYmlRZmtkamhqTTFRQ3d5ZE9ReTVBa3JFcWNjamc5aUEwYi8rYTVP?=
 =?utf-8?B?dG5ZYWR3RzBTS3VtNlI2YkNBcE5pWmNkSUEyKzlRVXdEK2tZVUx5NWtKRGtH?=
 =?utf-8?B?YkRJSzJ1WFZLZDZnME5KNGdiL01scEZIUUpZaTNnbVRmdDZXR3Fkd0NYUTZz?=
 =?utf-8?B?Q2Z3Tm44RE5sZHhyVTlSL1RHRDNDUmdRWFJCWk9GbjltQUErcDlhdVBYWG5S?=
 =?utf-8?B?VnkvUVNYU0J3RmRBeFZpTUN5Y2p2aWxrbXNUWnFSZU9sc09kSklDTHA3THlP?=
 =?utf-8?B?d2trS01DU0hvaFE0TWYvaVRDWFpFTkptWVFrN09hdWEzUnp5VnczSXpYTUhW?=
 =?utf-8?B?RWR1Ry9rZGRqNmRRa3VDZUVtQWNhOFIxWTFJOXhuNVRrSFc0dENhazRHekhD?=
 =?utf-8?B?TmlvdVN0UmJpNUYzdUhpZXorWGVNd0svbUhJazdCRlptRGR4YStlUHBpNlEx?=
 =?utf-8?B?dVRGeWNrc2ZENGxCK0gyTzRoSHVCdjMzQWJGM05vYStmRmlRd08yY2VDYXkr?=
 =?utf-8?B?bHJ0L3hnWE5XdTBIZ1Z6dlBVY1MrLzhYOCtseisrcVdINGU1Umd2ZU5EY213?=
 =?utf-8?B?MWYvb1VSMERrVkVWcStNc0pmSVNQWjhqcjVHWEN4Wm5MMkRQV3VXVXhZdXRq?=
 =?utf-8?B?NmtEV0pjWXAyMG5ZL3dmb081WnlZWGFNa2JFWHFtRjJydjlzZVd5UEY3TGgz?=
 =?utf-8?B?eTNwcGJvcGlhN1FUeExlRHVveEJwMDkyaUJNVVI5eTVLK0RrVkxaNFFBSWEw?=
 =?utf-8?B?dG5SdnN4MW5zdGJqaUpwNElqN2tWVU1YZE9yL1c2K2xGU1VFYWxuVHlEZWhj?=
 =?utf-8?B?Tmc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: facd962b-4373-41d2-1f5c-08dbbfa735bf
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Sep 2023 22:15:08.6395
 (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: XiB6B3sVLDm1fWHtXW6gq1GpOTbLAK8iDmNmgux6dfg7k5y0GwEUQdRmk4QkIwceG5K2m+l5ZdvohwAG0xO3i9NWvyYDZzMEeLTTkOwfdDQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV8PR11MB8771
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.3 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 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]); Wed, 27 Sep 2023 15:15:36 -0700 (PDT)

Hi Maciej,

On 9/22/2023 1:10 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>
> ---
> 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 | 30 ++++++++++++---------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 3a8111362d26..edc8fc6e44b0 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 = -1, ret = 0;

I am trying to understand the schema_len initialization. Could
you please elaborate why you chose -1? I'm a bit concerned with
the robustness here with it being used as an unsigned integer
in write() and also the negative array index later.

>  
>  	if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) &&
>  	    strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) &&
> @@ -520,27 +520,31 @@ 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);
>  
> -	fp = fopen(controlgroup, "w");
> -	if (!fp) {
> -		sprintf(reason, "Failed to open control group");
> +	fd = open(controlgroup, O_WRONLY);
> +	if (!fd) {

Be careful ... the error checking appropriate to the original
pointer needs a double check with this new usage.
According to "man 2 open" - open() returns -1 on error so I expect
that this should rather be:
	if (fd < 0) {
or
	if (fd == -1) {

The rest looks good to me.

Reinette