Re: [PATCH v2 1/3] scsi: Fix a scsi_show_rq() NULL pointer dereference

[Ming Lei <ming.lei@redhat.com>]

Hi Martin,

On Thu, Dec 07, 2017 at 09:46:21PM -0500, Martin K. Petersen wrote:
> 
> Ming,
> 
> > As I explained in [1], the use-after-free is inevitable no matter if
> > clearing 'SCpnt->cmnd' before mempool_free() in sd_uninit_command() or
> > not, so we need to comment the fact that cdb may point to garbage
> > data, and this function(especially __scsi_format_command() has to
> > survive that, so that people won't be surprised when kasan complains
> > use-after-free, and guys will be careful when they try to change the
> > code in future.
> 
> Longer term we really need to get rid of the separate CDB allocation. It
> was a necessary evil when I did it. And not much of a concern since I
> did not expect anybody sane to use Type 2 (it's designed for use inside
> disk arrays).
> 
> However, I keep hearing about people using Type 2 drives. Some vendors
> source drives formatted that way and use the same SKU for arrays and
> standalone servers.
> 
> So we should really look into making it possible for a queue to have a
> bigger than 16-byte built-in CDB. For Type 2 devices, 32-byte reads and
> writes are a prerequisite. So it would be nice to be able to switch a
> queue to a larger allocation post creation (we won't know the type until
> after READ CAPACITY(16) has been sent).

I am wondering why you don't make __cmd[] in scsi_request defined as 32bytes?
Even for some hosts with thousands of tag, the memory waste is still not
too much.

Or if you prefer to do post creation, we have sbitmap_queue now, which can
help to build a pre-allocated memory pool easily, and its allocation/free is
pretty efficient.

Thanks,
Ming