Re: [PATCH v11 23/23] x86/resctrl: Introduce interface to modify assignment states of the groups

[Dave Martin <Dave.Martin@arm.com>]

Hi,

On Thu, Feb 20, 2025 at 02:57:31PM -0600, Moger, Babu wrote:
> Hi Dave,

[...]

> Created the problem using this code using a "test" group.
> 
> include <stdio.h>
> #include <errno.h>
> #include <string.h>
> 
> int main()
> {
>         FILE *file;
>         int n;
> 
>         file = fopen("/sys/fs/resctrl/info/L3_MON/mbm_assign_control", "w");
> 
>         if (file == NULL) {
>                 printf("Error opening file!\n");
>                 return 1;
>         }
> 
>         printf("File opened successfully.\n");
> 
>         for (n = 0; n < 100; n++)
>                 if
> (fputs("test//0=tl;1=tl;2=tl;3=tl;4=tl;5=tl;9=tl;10=tl;11=tl\n", file) == EOF)
>                         fprintf(stderr, "Failed on interation %d error
> %s\n ", n, strerror(errno));
> 
>         if (fclose(file) == 0) {
>                 printf("File closed successfully.\n");
>         } else {
>                 printf("Error closing file!\n");
>         }
> }

Right.

> When the buffer overflow happens the newline will not be there. I have
> added this error via rdt_last_cmd_puts. At least user knows there is an error.
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 484d6009869f..70a96976e3ab 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1250,8 +1252,10 @@ static ssize_t
> resctrl_mbm_assign_control_write(struct kernfs_open_file *of,
>         int ret;
> 
>         /* Valid input requires a trailing newline */
> -       if (nbytes == 0 || buf[nbytes - 1] != '\n')
> +       if (nbytes == 0 || buf[nbytes - 1] != '\n') {
> +               rdt_last_cmd_puts("mbm_cntr_assign: buffer invalid\n");
>                 return -EINVAL;
> +       }
> 
>         buf[nbytes - 1] = '\0';
> 
> 
> 
> I am open to other ideas to handle this case.

Reinette, what do you think about this as a stopgap approach?

The worst that happens is that userspace gets an unexpected failure in
scenarios that seem unlikely in the near future (i.e., where there are
a lot of RMIDs available, and at the same time groups have been given
stupidly long names).

Since this is an implementation issue rather than an interface issue,
we could fix it later on.


Longer term, we may want to define some stuff along the lines of

	struct rdtgroup_file {
		/* persistent data for an rdtgroup open file instance */
	};

	static int rdtgroup_file_open(struct kernfs_open_file *of)
	{
		struct rdtgroup_file *rf;

		rf = kzalloc(sizeof(*rf), GFP_KERNEL);
		if (!rf)
			return -ENOMEM;

		of->priv;
	}

	static void rdtgroup_file_release(struct kernfs_open_file *of)
	{
		/*
		 * Deal with dangling data and do cleanup appropriate
		 * for whatever kind of file this is, then:
		 */
		kfree(of->priv);
	}


Then we'd have somewhere to stash data that needs to be carried over
from one read/write call to the next.

I tried to port my schemata buffering hack over, but the requirements
are not exactly the same as for mbm_assign_control, so it wasn't
trivial.  It feels do-able, but it might be better to stabilise this
series before going down that road.

(I'm happy to spend some time trying to wire this up if it would be
useful, though.)

Cheers
---Dave