Re: [PATCH v4 1/2] selftests/resctrl: Fix schemata write error check

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Maciej,

On 9/27/2023 11:46 PM, Maciej Wieczór-Retman wrote:
> On 2023-09-27 at 15:15:06 -0700, Reinette Chatre wrote:
>> On 9/22/2023 1:10 AM, Maciej Wieczor-Retman wrote:

>>> 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.
> 
> My idea was that if the initial value for schema_len was 0, then if
> resctrl_val wouldn't equal any of MBA_STR, MBM_STR, CAT_STR, CMT_STR

Ensuring that resctrl_val is equal to one of these seems to be the
first thing write_schemata() does.

> values schema_len would stay zero and write nothing.

Your alternative writes "-1". write() is declared as:
	ssize_t write(int fd, const void *buf, size_t count);

note that "count" is size_t, which is an unsigned value. Providing
it -1 is thus a very large number and likely to cause overflow. In fact
if I even try to compile a program where the compiler can figure out
count will be -1 it fails the compile (stringop-overflow).
 
> I think it would be difficult to debug such an error because even later
> in ksft_print_msg the requested schema would get printed as if there was
> no error. In the case I mentioned above the function will just error out
> which I assume could be helpful.

You seem to rely on write() to cleanly catch giving it bad data.

> Other solutions that can accomplish the same goal would be checking
> write() not only for negative values but also for zero (since in
> here this is pretty much an error). Or checking schema_len for only
> positive values after the block of code where it gets assigned a
> value from sprintf.
> 
> Are any of the above safer or more logical in your opinion?

There is no error checking on schema_len. After it has been initialized it
can be checked for errors and write_schemata() can be exited immediately if
an error was encountered without attempting the write().

Reinette