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

[Chris Leech <cleech@redhat.com>]

On Mon, Jul 28, 2025 at 04:55:12PM -0600, Gustavo A. R. Silva wrote:
> 
> 
> 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?

This might be confusing to see as a diff; qla24xx_alloc_purex_item is
either allocating a new heap item with a flexible array size, or if the
size is small enough and there is only one in use at a time it's
returning default_item.

If the allocation fails, then the driver is already not using
default_item for this call and returns NULL.

  static struct purex_item *
  qla24xx_alloc_purex_item(scsi_qla_host_t *vha, uint16_t size)
  {
      struct purex_item *item = NULL;

      if (size > QLA_DEFAULT_PAYLOAD_SIZE) {
          item = kzalloc(struct_size(item, iocb, size), GFP_ATOMIC);
      } else {
          if (atomic_inc_return(&vha->default_item.in_use) == 1) {
              item = (struct purex_item *)&vha->default_item;
              goto initialize_purex_header;
          } else {
              item = kzalloc(
                  struct_size(item, iocb, QLA_DEFAULT_PAYLOAD_SIZE),
                  GFP_ATOMIC);
          }
      }
      if (!item) {
          ql_log(ql_log_warn, vha, 0x5092,
                 ">> Failed allocate purex list item.\n");

          return NULL;
      }

  initialize_purex_header:
      item->vha = vha;
      item->size = size;
      return item;
  }


default item was replaced with the header and a static sized array,
cast to a purex_item * when returned through the alloc function

  -       struct purex_item default_item;
  +       struct purex_item_hdr default_item;
  +       uint8_t __default_item_iocb[QLA_DEFAULT_PAYLOAD_SIZE];

- Chris