| Message ID | 20220111072500.GI11243@kili (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | counter: fix an IS_ERR() vs NULL bug | expand |
On Tue, Jan 11, 2022 at 10:25:00AM +0300, Dan Carpenter wrote: > There are 8 callers for devm_counter_alloc() and they all check for NULL > instead of error pointers. I think NULL is the better thing to return > for allocation functions so update counter_alloc() and devm_counter_alloc() > to return NULL instead of error pointers. > > Fixes: c18e2760308e ("counter: Provide alternative counter registration functions") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Oh indeed. I wonder how you found that problem. There is a possibility that counter_alloc() (as it's implemented now in next) returns an error value != -ENOMEM. (ida_alloc() can return -ENOSPC, counter_chrdev_add() can return -EINVAL). Still returning NULL on error looks sane. Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks Uwe
On Tue, Jan 11, 2022 at 09:20:55AM +0100, Uwe Kleine-König wrote: > On Tue, Jan 11, 2022 at 10:25:00AM +0300, Dan Carpenter wrote: > > There are 8 callers for devm_counter_alloc() and they all check for NULL > > instead of error pointers. I think NULL is the better thing to return > > for allocation functions so update counter_alloc() and devm_counter_alloc() > > to return NULL instead of error pointers. > > > > Fixes: c18e2760308e ("counter: Provide alternative counter registration functions") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Oh indeed. I wonder how you found that problem. Of course from static analysis. Each caller triggers a "this NULL check should be an IS_ERR() check warning" followed by a "potential error pointer dereference" warning. regards, dan carpenter
On Tue, Jan 11, 2022 at 10:25:00AM +0300, Dan Carpenter wrote: > There are 8 callers for devm_counter_alloc() and they all check for NULL > instead of error pointers. I think NULL is the better thing to return > for allocation functions so update counter_alloc() and devm_counter_alloc() > to return NULL instead of error pointers. > > Fixes: c18e2760308e ("counter: Provide alternative counter registration functions") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Hi Dan, Thank you for catching this bug. I do request a minor adjustment to this patch for the sake of matching the format of the rest of the code in this file: please remove the goto err_alloc_ch and simply exit early with a return NULL. > --- > drivers/counter/counter-core.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c > index 7e0957eea094..0abb28cd3d16 100644 > --- a/drivers/counter/counter-core.c > +++ b/drivers/counter/counter-core.c > @@ -90,10 +90,8 @@ struct counter_device *counter_alloc(size_t sizeof_priv) > int err; > > ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL); > - if (!ch) { > - err = -ENOMEM; > + if (!ch) > goto err_alloc_ch; So return NULL here. > - } > > counter = &ch->counter; > dev = &counter->dev; > @@ -125,7 +123,7 @@ struct counter_device *counter_alloc(size_t sizeof_priv) > kfree(ch); > err_alloc_ch: Remove this label. Thank you, William Breathitt Gray > > - return ERR_PTR(err); > + return NULL; > } > EXPORT_SYMBOL_GPL(counter_alloc); > > @@ -208,12 +206,12 @@ struct counter_device *devm_counter_alloc(struct device *dev, size_t sizeof_priv > int err; > > counter = counter_alloc(sizeof_priv); > - if (IS_ERR(counter)) > - return counter; > + if (!counter) > + return NULL; > > err = devm_add_action_or_reset(dev, devm_counter_put, counter); > if (err < 0) > - return ERR_PTR(err); > + return NULL; > > return counter; > } > -- > 2.20.1 >
diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c index 7e0957eea094..0abb28cd3d16 100644 --- a/drivers/counter/counter-core.c +++ b/drivers/counter/counter-core.c @@ -90,10 +90,8 @@ struct counter_device *counter_alloc(size_t sizeof_priv) int err; ch = kzalloc(sizeof(*ch) + sizeof_priv, GFP_KERNEL); - if (!ch) { - err = -ENOMEM; + if (!ch) goto err_alloc_ch; - } counter = &ch->counter; dev = &counter->dev; @@ -125,7 +123,7 @@ struct counter_device *counter_alloc(size_t sizeof_priv) kfree(ch); err_alloc_ch: - return ERR_PTR(err); + return NULL; } EXPORT_SYMBOL_GPL(counter_alloc); @@ -208,12 +206,12 @@ struct counter_device *devm_counter_alloc(struct device *dev, size_t sizeof_priv int err; counter = counter_alloc(sizeof_priv); - if (IS_ERR(counter)) - return counter; + if (!counter) + return NULL; err = devm_add_action_or_reset(dev, devm_counter_put, counter); if (err < 0) - return ERR_PTR(err); + return NULL; return counter; }
There are 8 callers for devm_counter_alloc() and they all check for NULL instead of error pointers. I think NULL is the better thing to return for allocation functions so update counter_alloc() and devm_counter_alloc() to return NULL instead of error pointers. Fixes: c18e2760308e ("counter: Provide alternative counter registration functions") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/counter/counter-core.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)