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++;
}
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) {
ret = -ENOMEM;
goto error_clear_attrs;
}
- /* Copy across original attributes, and point to original binary attributes */
if (indio_dev->info->attrs) {
- memcpy(iio_dev_opaque->chan_attr_group.attrs,
- indio_dev->info->attrs->attrs,
- sizeof(iio_dev_opaque->chan_attr_group.attrs[0])
- *attrcount_orig);
iio_dev_opaque->chan_attr_group.is_visible =
indio_dev->info->attrs->is_visible;
iio_dev_opaque->chan_attr_group.bin_attrs =
--
2.40.0.1.gaa8946217a0bOn 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!
--
With Best Regards,
Andy Shevchenko
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!
> > > > }
--
With Best Regards,
Andy Shevchenko
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!
>
> > > > > }
--
With Best Regards,
Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.