[v1,3/8] iio: core: Switch to krealloc_array()

  Let the krealloc_array() copy the original data and
check for a multiplication overflow.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/industrialio-core.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)
  

On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:
> Let the krealloc_array() copy the original data and
> check for a multiplication overflow.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/iio/industrialio-core.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 90e59223b178..be154879983e 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1465,7 +1465,7 @@ int iio_device_register_sysfs_group(struct iio_dev
> *indio_dev,
>         const struct attribute_group **new, **old = iio_dev_opaque->groups;
>         unsigned int cnt = iio_dev_opaque->groupcounter;
>  
> -       new = krealloc(old, sizeof(*new) * (cnt + 2), GFP_KERNEL);
> +       new = krealloc_array(old, cnt + 2, sizeof(*new), GFP_KERNEL);
>         if (!new)
>                 return -ENOMEM;
>  
> @@ -1483,13 +1483,13 @@ static int iio_device_register_sysfs(struct iio_dev
> *indio_dev)
>  {
>         struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>         int i, ret = 0, attrcount, attrn, attrcount_orig = 0;
> +       struct attribute **attrs, **attr, *clk = NULL;
>         struct iio_dev_attr *p;
> -       struct attribute **attr, *clk = NULL;
>  
>         /* First count elements in any existing group */
> -       if (indio_dev->info->attrs) {
> -               attr = indio_dev->info->attrs->attrs;
> -               while (*attr++ != NULL)
> +       attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs : NULL;
> +       if (attrs) {
> +               for (attr = attrs; *attr; attr++)
>                         attrcount_orig++;

not really related with the change... maybe just mention it in the commit?

>         }
>         attrcount = attrcount_orig;
> @@ -1521,20 +1521,14 @@ static int iio_device_register_sysfs(struct iio_dev
> *indio_dev)
>         if (clk)
>                 attrcount++;
>  
> +       /* Copy across original attributes, and point to original binary
> attributes */
>         iio_dev_opaque->chan_attr_group.attrs =
> -               kcalloc(attrcount + 1,
> -                       sizeof(iio_dev_opaque->chan_attr_group.attrs[0]),
> -                       GFP_KERNEL);
> +               krealloc_array(attrs, attrcount + 1, sizeof(*attrs),
> GFP_KERNEL);
>         if (iio_dev_opaque->chan_attr_group.attrs == NULL) {

since you're here and you also already did some style cleanups above, maybe
change it to 'if (!iio_dev_opaque->chan_attr_group.attrs)'?

- Nuno Sá
  

On Fri, Jul 21, 2023 at 09:59:37AM +0200, Nuno Sá wrote:
> On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:

...

> > +       struct attribute **attrs, **attr, *clk = NULL;
> >         struct iio_dev_attr *p;
> > -       struct attribute **attr, *clk = NULL;
> >  
> >         /* First count elements in any existing group */
> > -       if (indio_dev->info->attrs) {
> > -               attr = indio_dev->info->attrs->attrs;
> > -               while (*attr++ != NULL)
> > +       attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs : NULL;
> > +       if (attrs) {
> > +               for (attr = attrs; *attr; attr++)
> >                         attrcount_orig++;

> not really related with the change... maybe just mention it in the commit?

Hmm... It's related to make krealloc_array() to work as expected.

> >         }

...

> >         iio_dev_opaque->chan_attr_group.attrs =
> > -               kcalloc(attrcount + 1,
> > -                       sizeof(iio_dev_opaque->chan_attr_group.attrs[0]),
> > -                       GFP_KERNEL);
> > +               krealloc_array(attrs, attrcount + 1, sizeof(*attrs),
> > GFP_KERNEL);
> >         if (iio_dev_opaque->chan_attr_group.attrs == NULL) {
> 
> since you're here and you also already did some style cleanups above, maybe
> change it to 'if (!iio_dev_opaque->chan_attr_group.attrs)'?

I don't think it's related (but you can tell that this check related to
the allocator, and since we touch it, we may touch this), if Jonathan
wants this, I definitely do.

...

Thank you for the review!
  

On Fri, 2023-07-21 at 13:14 +0300, Andy Shevchenko wrote:
> On Fri, Jul 21, 2023 at 09:59:37AM +0200, Nuno Sá wrote:
> > On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > +       struct attribute **attrs, **attr, *clk = NULL;
> > >         struct iio_dev_attr *p;
> > > -       struct attribute **attr, *clk = NULL;
> > >  
> > >         /* First count elements in any existing group */
> > > -       if (indio_dev->info->attrs) {
> > > -               attr = indio_dev->info->attrs->attrs;
> > > -               while (*attr++ != NULL)
> > > +       attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs :
> > > NULL;
> > > +       if (attrs) {
> > > +               for (attr = attrs; *attr; attr++)
> > >                         attrcount_orig++;
> 
> > not really related with the change... maybe just mention it in the commit?
> 
> Hmm... It's related to make krealloc_array() to work as expected.
> 

Hmm, I think it's arguable :). while() -> for() it's not really needed unless
I'm missing something. You could even initialize 'attrs' to NULL at declaration
and keep the above diff minimum.

That said, I actually prefer this style (even though some people don't like much
the ternary operator).

> > >         }
> 
> ...
> 
> > >         iio_dev_opaque->chan_attr_group.attrs =
> > > -               kcalloc(attrcount + 1,
> > > -                       sizeof(iio_dev_opaque->chan_attr_group.attrs[0]),
> > > -                       GFP_KERNEL);
> > > +               krealloc_array(attrs, attrcount + 1, sizeof(*attrs),
> > > GFP_KERNEL);
> > >         if (iio_dev_opaque->chan_attr_group.attrs == NULL) {
> > 
> > since you're here and you also already did some style cleanups above, maybe
> > change it to 'if (!iio_dev_opaque->chan_attr_group.attrs)'?
> 
> I don't think it's related (but you can tell that this check related to
> the allocator, and since we touch it, we may touch this), if Jonathan
> wants this, I definitely do.

Fair enough...


- Nuno Sá
  

On Fri, Jul 21, 2023 at 12:53:53PM +0200, Nuno Sá wrote:
> On Fri, 2023-07-21 at 13:14 +0300, Andy Shevchenko wrote:
> > On Fri, Jul 21, 2023 at 09:59:37AM +0200, Nuno Sá wrote:
> > > On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:

...

> > > > +       struct attribute **attrs, **attr, *clk = NULL;
> > > >         struct iio_dev_attr *p;
> > > > -       struct attribute **attr, *clk = NULL;
> > > >  
> > > >         /* First count elements in any existing group */
> > > > -       if (indio_dev->info->attrs) {
> > > > -               attr = indio_dev->info->attrs->attrs;
> > > > -               while (*attr++ != NULL)
> > > > +       attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs :
> > > > NULL;
> > > > +       if (attrs) {
> > > > +               for (attr = attrs; *attr; attr++)
> > > >                         attrcount_orig++;
> > 
> > > not really related with the change... maybe just mention it in the commit?
> > 
> > Hmm... It's related to make krealloc_array() to work as expected.
> > 
> 
> Hmm, I think it's arguable :). while() -> for() it's not really needed unless
> I'm missing something. You could even initialize 'attrs' to NULL at declaration
> and keep the above diff minimum.

I'm not a fan of the assignments in the declarations when it potentially can be
disrupted by a chunk of code and reading the code itself may be harder due to
an interruption for checking the initial value. Hence, having

+	attr = attrs;
 	while (... != NULL)

seems enough to be replaced with one liner for-loop.

> That said, I actually prefer this style (even though some people don't like much
> the ternary operator).

Thanks!

> > > >         }
  

On Fri, Jul 21, 2023 at 02:28:36PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 21, 2023 at 12:53:53PM +0200, Nuno Sá wrote:
> > On Fri, 2023-07-21 at 13:14 +0300, Andy Shevchenko wrote:
> > > On Fri, Jul 21, 2023 at 09:59:37AM +0200, Nuno Sá wrote:
> > > > On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:

...

> > > > > +       struct attribute **attrs, **attr, *clk = NULL;
> > > > >         struct iio_dev_attr *p;
> > > > > -       struct attribute **attr, *clk = NULL;
> > > > >  
> > > > >         /* First count elements in any existing group */
> > > > > -       if (indio_dev->info->attrs) {
> > > > > -               attr = indio_dev->info->attrs->attrs;
> > > > > -               while (*attr++ != NULL)
> > > > > +       attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs :
> > > > > NULL;
> > > > > +       if (attrs) {
> > > > > +               for (attr = attrs; *attr; attr++)
> > > > >                         attrcount_orig++;
> > > 
> > > > not really related with the change... maybe just mention it in the commit?
> > > 
> > > Hmm... It's related to make krealloc_array() to work as expected.
> > > 
> > 
> > Hmm, I think it's arguable :). while() -> for() it's not really needed unless
> > I'm missing something. You could even initialize 'attrs' to NULL at declaration
> > and keep the above diff minimum.
> 
> I'm not a fan of the assignments in the declarations when it potentially can be
> disrupted by a chunk of code and reading the code itself may be harder due to
> an interruption for checking the initial value. Hence, having
> 
> +	attr = attrs;
>  	while (... != NULL)
> 
> seems enough to be replaced with one liner for-loop.

Note that attrs is reused later, so the above assignment makes it cleaner that
some value is assigned to it. With the original code it's not so obvious.

> > That said, I actually prefer this style (even though some people don't like much
> > the ternary operator).
> 
> Thanks!
> 
> > > > >         }