Date:   Mon, 9 Oct 2023 11:55:16 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Shuah Khan <shuah@...nel.org>
CC:     <ilpo.jarvinen@...ux.intel.com>, <linux-kernel@...r.kernel.org>,
        <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH v6 1/2] selftests/resctrl: Fix schemata write error check

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@...el.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.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@...el.com>,
>   *    Fenghua Yu <fenghua.yu@...el.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@...el.com>

Reinette

Powered by blists - more mailing lists