Re: [PATCH v2] scsi_debug: fix sleep in invalid context

[Douglas Gilbert <dgilbert@interlog.com>]

On 2016-06-08 04:57 PM, Douglas Gilbert wrote:
> On 2016-06-07 09:55 PM, Christoph Hellwig wrote:
>>> +static int p_fill_from_dev_buffer(struct scsi_cmnd *scp, const void *arr,
>>> +                  int arr_len, unsigned int off_dst)
>>> +{
>>> +    int act_len, n;
>>> +    struct scsi_data_buffer *sdb = scsi_in(scp);
>>> +    off_t skip = off_dst;
>>
>> Why off_t which is a signed value instead of the unsigned in passed in?
>
> Because off_t is the type of the last argument to sg_pcopy_from_buffer()
> which is where the off_dst value goes. So there is potentially a size
> of integer change plus signedness, IMO best expressed by defining a
> new auto variable. I notice some projects have a uoff_t typedef for
> offsets that make no sense when negative (like this case), but not the
> Linux kernel.
>
>>> +#define RL_BUCKET_ELEMS 8
>>> +
>>>   /* Even though each pseudo target has a REPORT LUNS "well known logical unit"
>>>    * (W-LUN), the normal Linux scanning logic does not associate it with a
>>>    * device (e.g. /dev/sg7). The following magic will make that association:
>>> @@ -3285,12 +3315,14 @@ static int resp_report_luns(struct scsi_cmnd *scp,
>>>       unsigned char select_report;
>>>       u64 lun;
>>>       struct scsi_lun *lun_p;
>>> -    u8 *arr;
>>> +    u8 arr[RL_BUCKET_ELEMS * sizeof(struct scsi_lun)];
>>
>> just use an on-stack array of type struct scsi_lun here, e.g.:
>>
>>     struct scsi_lun arr[RL_BUCKET_ELEMS];
>>
>> Which you can then use directly instead of lun_p later, but which can
>> also be passed p_fill_from_dev_buffer as that takes a void pointer.
>
> Can do.

When I looked at doing this, it's not that simple. The first 8 byte
"slot" is not a LUN, it's the response header, which just happens
to be 8 bytes long. That is the reason for the comment above that loop:
   /* loops rely on sizeof response header same as sizeof lun (both 8) */

So IMO: 'struct scsi_lun arr[RL_BUCKET_ELEMS];' will work but
is deceptive. IMO the v2 patch should stand, unless someone has an
alternate patch. The original patch is also fine. This is a bug
fix, not a new feature.

Doug Gilbert