[PATCH][2.6] rtas error-inject support

[Paul Mackerras]

Jake Moilanen writes:

> Here's a patch w/ check for EFAULT.

In general, since this is a patch for 2.6 (according to the subject
line :), it would be better to do all this in userspace using the rtas
syscall that was added recently.  But here are comments on the patch
anyway:


> +config RTAS_ERRINJCT
> +	bool "RTAS Errinject"

How about bool "RTAS Error injection facility" ?

> +static unsigned int open_token = 0;

No need to initialize things to 0, C does that by default.

> @@ -207,7 +221,8 @@
>  void proc_rtas_init(void)
>  {
>  	struct proc_dir_entry *entry;
> -
> + 	int errinjct_token;
> +

I can't see any difference between the blank line that is deleted and
the one that is inserted (not even any whitespace).  I wonder why diff
put that in?  Or has your mailer deleted trailing whitespace?

> +static ssize_t ppc_rtas_errinjct_write(struct file * file, const char * buf,
> +				       size_t count, loff_t *ppos)
> +{
> +
> +	char * ei_token;
> +	char * workspace = NULL;
> +	size_t max_len;
> +	int token_len;
> +	int rc;
> +
> +	/* Verify the errinjct token length */
> +	if (count < ERRINJCT_TOKEN_LEN) {
> +		max_len = count;
> +	} else {
> +		max_len = ERRINJCT_TOKEN_LEN;
> +	}
> +
> +	token_len = strnlen(buf, max_len);

That's a user pointer that you're using strnlen on.  Ouch.  Use
strnlen_user or copy_from_user.  In fact, do we need to check the
string length at all?  Would it matter if there was a null in the
buffer, and the value taken as a string was shorter than we thought?

> +	token_len++; /* Add one for the null termination */
> +
> +	ei_token = (char *)kmalloc(token_len, GFP_KERNEL);
> +	if (!ei_token) {
> +		printk(KERN_WARNING "error: kmalloc failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	strncpy(ei_token, buf, token_len);

Another access to the user buffer without using *user functions.
Ouch.

> +int
> +rtas_errinjct(unsigned int open_token, char * ei_token, char * workspace, size_t workspace_size)
> +{
> +	struct errinjct_token * ei;
> +	int rtas_ei_token = -1;
> +	unsigned int time;
> +	int rc = 0;
> + 	int i;
> +
> + 	ei = ei_token_list;
> + 	for (i = 0; i < MAX_ERRINJCT_TOKENS && ei->name; i++) {
> + 		if (strcmp(ei_token, ei->name) == 0) {
> + 			rtas_ei_token = ei->value;
> + 			break;
> + 		}
> + 		ei++;
> + 	}
> + 	if (rtas_ei_token == -1) {
> + 		return -EINVAL;
> + 	}
> +
> + 	spin_lock(&rtas_data_buf_lock);
> +
> +	while (1) {
> +		if (rc != RTAS_BUSY && workspace) {
> +			memset(rtas_data_buf, 0, RTAS_DATA_BUF_SIZE);
> +			memcpy(rtas_data_buf, workspace, workspace_size);
> +		}

This worries me.  We copy the workspace (the contents of which are
undefined since we just kmalloc'd it) to rtas_data_buf, but we never
copy rtas_data_buf back to the workspace after the rtas call.  So how
can the contents of workspace ever be anything but undefined?

If we were doing this from userspace we could use the existing
facilities for getting memory below 4G and use that for the workspace
without any copying.

> +static int __init rtas_errinjct_init(void)
> +{
> +	char * token_array;
> + 	char * end_array;
> + 	int array_len = 0;
> + 	int len;
> + 	int i, j;
> +
> + 	token_array = (char *) get_property(rtas.dev, "ibm,errinjct-tokens",
> + 					    &array_len);
> + 	end_array = token_array + array_len;
> + 	for (i = 0, j = 0; i < MAX_ERRINJCT_TOKENS && token_array < end_array; i++) {
> +
> + 		len = strnlen(token_array, ERRINJCT_TOKEN_LEN) + 1;
> + 		ei_token_list[i].name = (char *) kmalloc(len, GFP_KERNEL);
> + 		if (!ei_token_list[i].name) {
> + 			printk(KERN_WARNING "error: kmalloc failed\n");

Why can't we just store a pointer to the token name within the OF
property value?  Why do we have to make a copy of it?

In fact, why do we need to parse the list here at all?  We use the
list for two things: matching the token name in the write function,
and listing the tokens in the read function.  In both cases we could
just run through the ibm,errinjct-tokens (what do they have against
vwls?) property value almost as easily as ei_token_list.

> +/* Error inject defines */
> +#define ERRINJCT_TOKEN_LEN 24  /* Max length of an error inject token */
> +#define MAX_ERRINJCT_TOKENS 15 /* Max # tokens. */
> +#define WORKSPACE_SIZE 1024

I worry about these too.  15 tokens sounds like future machines are
going to going to exceed this limit quite easily.  Also, is the
workspace size limit there something that applies to all RTAS
functions, or just to the error injection functions?  If the latter,
you should choose a name that indicates that.

Overall, this looks to me like something that could be done just as
well or better in userspace.  Doing it in userspace would make it
easy to avoid having an arbitrary limit on the number of tokens, for
instance.  Userspace could just read
/proc/device-tree/rtas/ibm,errinjct-tokens to get the list and match
against that directly.

Paul.

** Sent via the linuxppc64-dev mail list. See http://lists.linuxppc.org/