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

[Scott Bauer <sbauer@plzdonthack.me>]

On Thu, Oct 31, 2019 at 10:13:21AM -0600, Revanth Rajashekar wrote:
> This feature gives the user RW access to any opal table with admin1
> authority. The flags described in the new structure determines if the user
> wants to read/write the data. Flags are checked for valid values in
> order to allow future features to be added to the ioctl.
> 
> The user can provide the desired table's UID. Also, the ioctl provides a
> size and offset field and internally will loop data accesses to return
> the full data block. Read overrun is prevented by the initiator's
> sec_send_recv() backend. The ioctl provides a private field with the
> intention to accommodate any future expansions to the ioctl.
> 
> Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
> Signed-off-by: Revanth Rajashekar <revanth.rajashekar@intel.com>
Looks fine
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>



> +static int read_table_data(struct opal_dev *dev, void *data)
> +{
> +	struct opal_read_write_table *read_tbl = data;
> +	int err;
> +	size_t off = 0, max_read_size = OPAL_MAX_READ_TABLE;
> +	u64 table_len, len;
> +	u64 offset = read_tbl->offset, read_size = read_tbl->size - 1;
> +	u8 __user *dst;

> +
> +		/* len+1: This includes the NULL terminator at the end*/
> +		if (dev->prev_d_len > len + 1) {
> +			err = -EOVERFLOW;
> +			break;
> +		}
> +
> +		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;
> +		}
> +		dev->prev_data = NULL;

If you end up needing to spin a v4 can you please add a comment here reminding me that prev_data in this scenario is not kmalloc'd memory but an offset into the response buffer. I had to go track down why you were not kfree()ing this. I know in a year I'll have forgotten it and will review it again.



> +static int opal_generic_read_write_table(struct opal_dev *dev,
> +					 struct opal_read_write_table *rw_tbl)
> +{
> +	int ret, bit_set;
> +
> +	mutex_lock(&dev->dev_lock);
> +	setup_opal_dev(dev);
> +
> +	bit_set = fls64(rw_tbl->flags) - 1;
Maybe I am missing something obvious but why don't we just use flags as a number? Like instead of bit packing flags we just use it as a number like number 0 is read_table, 1 is write_table, 2 is delete_table, 3 is clear_table etc etc. I don't understand the neccessity for fls-ing some bit field when a number would suffice.

I don't want another revision --it's fine-- I'm just wondering why this method was chosen.