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

[Rosen Penev <rosenp@gmail.com>]

On Fri, Mar 20, 2026 at 11:42 AM Kees Cook <kees@kernel.org> wrote:
>
> 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.
I did that globally. Too many pre-existing issues with that.

I mostly just run make menuconfig on linux-next and see what happens.
>
> 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