Re: [PATCH 3/3] block: sed-opal: Add support to read/write opal tables generically

[Scott Bauer <sbauer@plzdonthack.me>]

On Wed, Aug 21, 2019 at 01:10:51PM -0600, Revanth Rajashekar wrote:

[snip]

> The ioctl provides a private field with the intentiont to accommodate
> any future expansions to the ioctl.

spelling (intentiont) 

[snip]

> + * IO_BUFFER_LENGTH = 2048
> + * sizeof(header) = 56
> + * No. of Token Bytes in the Response = 11
> + * MAX size of data that can be carried in response buffer
> + * at a time is : 2048 - (56 + 11) = 1981 = 0x7BD.
> + */
> +#define OPAL_MAX_READ_TABLE (0x7BD)
> +
> +static int read_table_data(struct opal_dev *dev, void *data)
> +{
> +		dst = (u8 __user *)(uintptr_t)read_tbl->data;
> +		if (copy_to_user(dst + off, dev->prev_data, dev->prev_d_len)) {
> +			pr_debug("Error copying data to userspace\n");
> +			err = -EFAULT;
> +			break;
> +		}

I'm with Jon on this one. Even though the spec says we have a max size, lets not put our trust in firmware engineers.
A simple if check is easy to place before the CTU and will solve any future wtf debugging on a userland program.





> +static int opal_generic_read_write_table(struct opal_dev *dev,
> +                                         struct opal_read_write_table *rw_tbl)
> +{
> +	const struct opal_step write_table_steps[] = {
> +		{ start_admin1LSP_opal_session, &rw_tbl->key },
> +		{ write_table_data, rw_tbl },
> +		{ end_opal_session, }
> +	};
> +
> +	const struct opal_step read_table_steps[] = {
> +		{ start_admin1LSP_opal_session, &rw_tbl->key },
> +		{ read_table_data, rw_tbl },
> +		{ end_opal_session, }
> +	};
> +	int ret = 0;
> +
> +	mutex_lock(&dev->dev_lock);
> +	setup_opal_dev(dev);
> +	if (rw_tbl->flags & OPAL_TABLE_READ) {
> +		if (rw_tbl->size > 0)
> +			ret = execute_steps(dev, read_table_steps,
> +					    ARRAY_SIZE(read_table_steps));
> +	} else if (rw_tbl->flags & OPAL_TABLE_WRITE) {
> +		if (rw_tbl->size > 0)
> +			ret = execute_steps(dev, write_table_steps,
> +					    ARRAY_SIZE(write_table_steps));
> +	} else {
> +		pr_debug("Invalid bit set in the flag.\n");
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&dev->dev_lock);
> +
> +	return ret;
> +}

Do we expect to add more flags in the future? I ask because this function can quickly get out
of hand with regard to the else if chain and the function table list above. If we think we're going
to add more flags in the future lets slap a switch statement in here to call opal_table_write() and
opal_table_read(). We can deal with that in the future I guess, I just don't want a 3000 line function.




> diff --git a/include/uapi/linux/sed-opal.h b/include/uapi/linux/sed-opal.h
> index 59eed0bdffd3..a803ed0534da 100644
> --- a/include/uapi/linux/sed-opal.h
> +++ b/include/uapi/linux/sed-opal.h
> +struct opal_read_write_table {
> +	struct opal_key key;
> +	const __u64 data;
> +	const __u8 table_uid[OPAL_UID_LENGTH];
> +	__u64 offset;
> +	__u64 size;
> +	#define OPAL_TABLE_READ (1 << 0)
> +	#define OPAL_TABLE_WRITE (1 << 1)
> +	__u64 flags;
> +	__u64 priv;
> +};

Two things, can you double check the pahole on this struct (Google it or ask Jon he knows).
Second, can you lift those defines into Enumerations or out of the struct? Is there a reason
they're in there?