Re: [PATCH] media: em28xx: kzalloc + kcalloc to kzalloc_flex

[Kees Cook <kees@kernel.org>]

On Thu, Mar 19, 2026 at 06:02:12PM -0700, Rosen Penev wrote:
> There's no need to allocate these separately.
> 
> Add __counted_by for extra runtime analysis. Moved counting variable
> allocation to right after allocation as required by __counted_by.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

This looks reasonable to me.

One thought I've had while reviewing your flex-array patches is that I
want to make sure you're doing your test builds with
KCFLAGS=-Wflexible-array-member-not-at-end
so that you can validate there's no new uses of the target structures
being composed within other structures while making these changes.

That looks clear here, but I think going forward, it would be worth
mentioning it as part of the commit log. Something like:

  This structure is not composed within other structures, confirmed with
  builds using -Wflexible-array-member-not-at-end.

or similar.

Reviewed-by: Kees Cook <kees@kernel.org>

-Kees

> ---
>  drivers/media/usb/em28xx/em28xx-cards.c | 18 ++----------------
>  drivers/media/usb/em28xx/em28xx.h       |  3 ++-
>  2 files changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index d7075ebabceb..c278e48b3428 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -3567,9 +3567,6 @@ void em28xx_free_device(struct kref *ref)
>  	if (!dev->disconnected)
>  		em28xx_release_resources(dev);
>  
> -	if (dev->ts == PRIMARY_TS)
> -		kfree(dev->alt_max_pkt_size_isoc);
> -
>  	kfree(dev);
>  }
>  EXPORT_SYMBOL_GPL(em28xx_free_device);
> @@ -3912,21 +3909,13 @@ static int em28xx_usb_probe(struct usb_interface *intf,
>  	}
>  
>  	/* allocate memory for our device state and initialize it */
> -	dev = kzalloc_obj(*dev);
> +	dev = kzalloc_flex(*dev, alt_max_pkt_size_isoc, intf->num_altsetting);
>  	if (!dev) {
>  		retval = -ENOMEM;
>  		goto err;
>  	}
>  
> -	/* compute alternate max packet sizes */
> -	dev->alt_max_pkt_size_isoc = kcalloc(intf->num_altsetting,
> -					     sizeof(dev->alt_max_pkt_size_isoc[0]),
> -					     GFP_KERNEL);
> -	if (!dev->alt_max_pkt_size_isoc) {
> -		kfree(dev);
> -		retval = -ENOMEM;
> -		goto err;
> -	}
> +	dev->num_alt = intf->num_altsetting;
>  
>  	/* Get endpoints */
>  	for (i = 0; i < intf->num_altsetting; i++) {
> @@ -4028,8 +4017,6 @@ static int em28xx_usb_probe(struct usb_interface *intf,
>  			dev->dvb_ep_bulk ? " bulk" : "",
>  			dev->dvb_ep_isoc ? " isoc" : "");
>  
> -	dev->num_alt = intf->num_altsetting;
> -
>  	if ((unsigned int)card[nr] < em28xx_bcount)
>  		dev->model = card[nr];
>  
> @@ -4163,7 +4150,6 @@ static int em28xx_usb_probe(struct usb_interface *intf,
>  	return 0;
>  
>  err_free:
> -	kfree(dev->alt_max_pkt_size_isoc);
>  	kfree(dev);
>  
>  err:
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index f3449c240d21..1c2f92927889 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -730,7 +730,6 @@ struct em28xx {
>  	int packet_multiplier;	// multiplier for wMaxPacketSize, used for
>  				// URB buffer size definition
>  	int num_alt;		// number of alternative settings
> -	unsigned int *alt_max_pkt_size_isoc; // array of isoc wMaxPacketSize
>  	unsigned int analog_xfer_bulk:1;	// use bulk instead of isoc
>  						// transfers for analog
>  	int dvb_alt_isoc;	// alternate setting for DVB isoc transfers
> @@ -772,6 +771,8 @@ struct em28xx {
>  
>  	struct em28xx	*dev_next;
>  	int ts;
> +
> +	unsigned int alt_max_pkt_size_isoc[] __counted_by(num_alt); // array of isoc wMaxPacketSize
>  };
>  
>  #define kref_to_dev(d) container_of(d, struct em28xx, ref)
> -- 
> 2.53.0
> 
> 

-- 
Kees Cook