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

[Michael Neuling <mikey@neuling.org>]

> > 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().

That was my point.  Let's do it upfront so it's clear they've been
checked.

> >> +	/*
> >> +	 * 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.

Sounds delicate to me.

> 
> 
> >> +	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).

Humm.

> >> +#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.

Please document this acronym before using it.

Mikey