[PATCH i-g-t v2 2/2] lib/igt_sysfs: make sure to write empty strings

[Lucas De Marchi]

On Mon, Feb 26, 2024 at 10:11:05PM +0100, Janusz Krzysztofik wrote:
>Hi Lucas,
>
>On Thursday, 22 February 2024 20:33:26 CET Lucas De Marchi wrote:
>> 	echo -n "" > /sys/module/<modulename>/parameters/<param>
>>
>> doesn't really work as it will just create a open() + close() expecting
>> the file to be truncated. The same issue happens with igt as it will
>> stop writing when there are 0 chars to write. Special case the empty
>> string so it always write a '\0' and make sure igt_sysfs_set() accounts
>> for the extra null char.
>>
>> Shell example:
>> 	# echo -n "/foo" > /sys/module/firmware_class/parameters/path
>> 	# cat /sys/module/firmware_class/parameters/path
>> 	/foo
>> 	# echo -n "" > /sys/module/firmware_class/parameters/path
>> 	/foo
>> 	# # make sure to actually write a \0:
>> 	echo -ne "\0" > /sys/module/firmware_class/parameters/path
>> 	# cat /sys/module/firmware_class/parameters/path
>>
>> Same thing happens when testing igt_sysfs_set():
>>        int dirfd = open("/sys/module/firmware_class/parameters", O_RDONLY);
>>        igt_sysfs_set(dirfd, "path", "");
>>
>> Previously it was not really setting the param.
>>
>> v2:
>>   - Fix return code from igt_sysfs_vprintf() to differentiate between
>>     writing 1 or 0 chars (Janusz)
>>   - Document the behavior of igt_sysfs_set() as being a higher-level
>>     than igt_sysfs_write().
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  lib/igt_sysfs.c | 28 +++++++++++++++++++++++-----
>>  1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
>> index 2b0225138..d5d5b249f 100644
>> --- a/lib/igt_sysfs.c
>> +++ b/lib/igt_sysfs.c
>> @@ -349,6 +349,9 @@ int igt_sysfs_get_num_gt(int device)
>>   * @len: the length to write
>>   *
>>   * This writes @len bytes from @data to the sysfs file.
>> + * Contrary to igt_sysfs_set(), this does not automatically add a
>> + * null char to terminate the data. It's caller responsibility to pass
>> + * the right len according to the data being written.
>>   *
>>   * Returns:
>>   * The number of bytes written, or -errno on error.
>> @@ -406,7 +409,12 @@ int igt_sysfs_read(int dir, const char *attr, void *data, int len)
>>   */
>>  bool igt_sysfs_set(int dir, const char *attr, const char *value)
>>  {
>> -	int len = strlen(value);
>> +	/*
>> +	 * Always write the null char at the end, to guarantee the write goes
>> +	 * through, even for an empty string.
>> +	 */
>> +	int len = strlen(value) + 1;
>
>Since pre-merge results indicate some sysfs entries don't like that extra null
>char, I'm assuming you are going to submit a fixed version.

yes, basically I have to return back to my previous version, changing
the behavior only when it's an empty string. But this time I tweaked the
return code so we don't return 1 when writting an empty string.

That's because some places in the kernel try to play nice with a
possible \n in the end of the string and do a strchr(s, '\n'). If it
doesn't find, then it considers the passed size to be the len and then
compare with possible values. However now the lengths don't match and it
fails. Example: pm_test. See kernel/power/main.c:pm_test_store on the
kernel side.

I thought about a quick fix on the kernel side that would basically be:

-       len = p ? p - buf : n;               
+       len = p ? p - buf : strnlen(buf, n); 

but it would be hard to know how many other places use a similar pattern
and would start failing.

Lucas De Marchi

>
>> +
>>  	return igt_sysfs_write(dir, attr, value, len) == len;
>>  }
>>
>> @@ -506,7 +514,7 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>>  {
>>  	char stack[128], *buf = stack;
>>  	va_list tmp;
>> -	int ret, fd;
>> +	int ret, fd, len;
>>
>>  	fd = openat(dir, attr, O_WRONLY);
>>  	if (igt_debug_on(fd < 0))
>> @@ -515,11 +523,12 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>>  	va_copy(tmp, ap);
>>  	ret = vsnprintf(stack, sizeof(stack), fmt, tmp);
>>  	va_end(tmp);
>> +
>
>NIT: Not related.
>
>>  	if (igt_debug_on(ret < 0))
>>  		return -EINVAL;
>>
>>  	if (ret > sizeof(stack)) {
>> -		unsigned int len = ret + 1;
>> +		len = ret + 1;
>>
>>  		buf = malloc(len);
>>  		if (igt_debug_on(!buf))
>> @@ -532,13 +541,22 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>>  		}
>>  	}
>>
>> -	ret = igt_writen(fd, buf, ret);
>> +	/*
>> +	 * Make sure to always issue a write() syscall, even if writing an
>> +	 * empty string, otherwise values in sysfs like module parameters don't
>> +	 * really get overwritten.  vsnprintf() guarantees to return a \0
>> +	 * terminated string, so just add that char. The return code is still
>> +	 * the same as before, to abstract that from caller.
>> +	 */
>> +	len = ret + 1;
>
>Potentially the same issue with some sysfs entries, I believe.
>
>Thanks,
>Janusz
>
>
>> +
>> +	ret = igt_writen(fd, buf, len);
>>
>>  	close(fd);
>>  	if (buf != stack)
>>  		free(buf);
>>
>> -	return ret;
>> +	return ret == len ? len - 1 : ret;
>>  }
>>
>>  /**
>>
>
>
>
>