Re: [PATCH v4 3/3] cxlflash: Virtual LUN support

[Manoj Kumar <manoj@linux.vnet.ibm.com>]

Mikey: Thanks for your review. Comments inline below.

On 8/11/2015 5:54 AM, Michael Neuling wrote:
>
> I'm not keen on the numerous pr_err() in here.  I think it'll make the driver
> chatty especially with a badly behaving userspace.


Will look at all the pr_err() and limit them to errors that are 
indicative of a mis-behaving device, as opposed to a mis-behaving 
application.

>> -	.ioctl = cxlflash_ioctl,
 >
> Where are you hooking this in now?  This seems broken.

This was an error in splitting up the patch. Will correct in v5.

>>   	size = (MAX_RHT_PER_CONTEXT * sizeof(*ctxi->rht_lun));
>> +	size += (MAX_RHT_PER_CONTEXT * sizeof(*ctxi->rht_needs_ws));
>
> This needs to be cleaned up as per my comment on patch 2.  Make this a separate
> allocation.

Okay. Will split this into multiple allocations.


>> +	{sizeof(struct dk_cxlflash_clone), (sioctl)cxlflash_disk_clone},
>>   	{sizeof(struct dk_cxlflash_recover_afu), (sioctl)cxlflash_afu_recover},
>>   	{sizeof(struct dk_cxlflash_manage_lun), (sioctl)cxlflash_manage_lun},
>
> Does this actually work if we don't have this patch?  If the IOCTLS are sparse
> (like with only patch 2/3) won't this table be broken?

Agreed. Look for these ioctls being re-ordered in v5 to avoid sparseness 
in the individual patches.

>>   		switch (cmd) {
>> +		case DK_CXLFLASH_USER_VIRTUAL:
>> +		case DK_CXLFLASH_VLUN_RESIZE:
>>   		case DK_CXLFLASH_RELEASE:
>> +		case DK_CXLFLASH_CLONE:
>>   			pr_err("%s: %s not supported for lun_mode=%d\n",
>>   			       __func__, decode_ioctl(cmd), afu->internal_lun);
>>   			rc = -EINVAL;
>
> If someone creates an internal lun and then does this, then we are going to get
> a bunch of errors on the console.  I don't think that should happen.  Just
> return it to the caller.

Will remove the pr_err().


> Be good to do some clear sanity check the "struct dk_cxlflash_resize *resize"
> here.  It's passed from userspace but then gets propogated to a bunch of other
> things here like nsectors, get_context etc who will all now be responsible for
> handling any dodgy data passed in.
>
> Same with all the other ioctl calls.  Sanity check the parameters ASAP.  Don't
> propagate potentially dodgy values all over the code base.
>

The ioctls have a standard header structure, with version etc. that are 
sanity checked before we get here. The other fields are sanity checked 
where they are used, i.e. in get_context().

>> +	/*
>> +	 * The requested size (req_size) is always assumed to be in 4k blocks,
>> +	 * so we have to convert it here from 4k to chunk size.
>> +	 */
>> +	nsectors = (resize->req_size * CXLFLASH_BLOCK_SIZE) / gli->blk_len;
>> +	new_size = DIV_ROUND_UP(nsectors, MC_CHUNK_SIZE);
>
> Like here. resize->req_size => new_size => grow_lxt() now grow_lxt() need to
> sanity check new_size.


This is a best effort allocator. If an allocation request y, cannot be 
satisfied because of insufficient space in the LUN (i.e. only x amount 
of space is available), then the allocator returns the remaining space 
(x). This best effort allocation mechanism avoids having to sanity check 
the size parameter.


>> +	struct dk_cxlflash_uvirtual *virt = (struct dk_cxlflash_uvirtual *)arg;
>
> Again, this should be sanity checked.

Same as comment above.

>> +	/* Resize even if requested size is 0 */
>> +	marshal_virt_to_resize(virt, &resize);
>
> Virt has not been sanity checked.  So now resize can contain bad data.
>
>
>> +	resize.rsrc_handle = rsrc_handle;

Same as above. As mentioned earlier, the size is immaterial. The rest of 
the parameters are set here (rsrc_handle).


>> +	dma_wmb(); /* Make RHT entry's LXT table size update visible */
>
> Nice documentation here for the memory barriers!

Thank you!

>> +#define LXT_LUNIDX_SHIFT  8	/* LXT entry, shift for LUN index */
>> +#define LXT_PERM_SHIFT    4	/* LXT entry, shift for permission bits */
>
> What is LXT?

LXT = lun translation table. There is one LXT entry per set of 
contiguous blocks for a virtual LUN (known both to the host and to the 
AFU). Will clarify this with inline comments.

>> +
>> +#define BITS_PER_LONG     64
>
> Please use #include <asm/bitsperlong.h>

Will re-use this, instead of creating our own definition.

>
>> +#define BPL               BITS_PER_LONG
>
> Don't redefine this.  Make it harder for others to read.  No one wants to learn
> your TLAs.

Same as above.

>> +	void *ba_lun_handle;
>
> ba_lun_handle seems to be commonly cast to a struct ba_lun_info *.  Can it just
> be a pointer to that?

Good catch. Will change in v5.