Re: [PATCH v2 1/1] scsi: qla2xxx: replace non-standard flexible array purex_item.iocb

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 28/07/25 15:15, Chris Leech wrote:
> On Mon, Jul 28, 2025 at 01:43:10PM -0600, Gustavo A. R. Silva wrote:
>> On 28/07/25 12:57, Chris Leech wrote:
>>> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
>>> index fe98c76e9be32..a00c06a9898ec 100644
>>> --- a/drivers/scsi/qla2xxx/qla_isr.c
>>> +++ b/drivers/scsi/qla2xxx/qla_isr.c
>>> @@ -1077,17 +1077,17 @@ static struct purex_item *
>>>    qla24xx_alloc_purex_item(scsi_qla_host_t *vha, uint16_t size)
>>>    {
>>>    	struct purex_item *item = NULL;
>>> -	uint8_t item_hdr_size = sizeof(*item);
>>>    	if (size > QLA_DEFAULT_PAYLOAD_SIZE) {
>>> -		item = kzalloc(item_hdr_size +
>>> -		    (size - QLA_DEFAULT_PAYLOAD_SIZE), GFP_ATOMIC);
>>> +		item = kzalloc(struct_size(item, iocb, size), GFP_ATOMIC);
>>
>> With the inclusion of `counted_by`, I think `item->size` should be updated
>> here:
>> 		item->size = size;
>>
>>>    	} else {
>>>    		if (atomic_inc_return(&vha->default_item.in_use) == 1) {
>>> -			item = &vha->default_item;
>>> +			item = (struct purex_item *)&vha->default_item;
>>>    			goto initialize_purex_header;
>>>    		} else {
>>> -			item = kzalloc(item_hdr_size, GFP_ATOMIC);
>>> +			item = kzalloc(
>>> +				struct_size(item, iocb, QLA_DEFAULT_PAYLOAD_SIZE),
>>> +				GFP_ATOMIC);
>>
>> ...and here:
>> 			item->size = QLA_DEFAULT_PAYLOAD_SIZE;
>>
>> Then remove `item->size = size;` just before `return item;`
> 
> Hmm, I don't think I agree with that. The single assignment before
> returning just keeps the allocation failure check in one place.
> The conditional nesting in this function is a little odd, but I don't
> want to start reworking it completly for this.
> 
> Is there a problem with the size referenced by counted_by potentially
> being smaller than the allocation?  That looks possible in the case of
> using the single pre-allocated default_item, but not needing to use the
> entire 64-bytes (I don't know if that happens).

But if both allocations for `struct purex_item *item` fail, then
you'd end up with a flexible-array member of size 0, and `item->size`
potentially being greater than zero, since `default_item` doesn't
contain `uint8_t iocb[64];` anymore (it was turned into flex-array
member `uint8_t iocb[] __counted_by(size);`)... unless I'm missing
something?

Thanks
-Gustavo