| Message ID | 20210805183100.49071-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] wwan: core: Avoid returning error pointer from wwan_create_dev() | expand |
On Thu, 5 Aug 2021 at 20:38, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > wwan_create_dev() is expected to return either valid pointer or NULL, > In some cases it might return the error pointer. Prevent this by converting > it to NULL after wwan_dev_get_by_parent(). wwan_create_dev is called both from wwan_register_ops() and wwan_create_port(), one using IS_ERR and the other using NULL testing, they should be aligned as well. Regards, Loic
On Thu, Aug 05, 2021 at 09:53:57PM +0200, Loic Poulain wrote: > On Thu, 5 Aug 2021 at 20:38, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > wwan_create_dev() is expected to return either valid pointer or NULL, > > In some cases it might return the error pointer. Prevent this by converting > > it to NULL after wwan_dev_get_by_parent(). > > wwan_create_dev is called both from wwan_register_ops() and > wwan_create_port(), one using IS_ERR and the other using NULL testing, > they should be aligned as well. Ah, good catch! I just sent v2, but eventually I have decided to switch to error pointer since it seems the most used pattern in the code. -- With Best Regards, Andy Shevchenko
Hello Andy, On Fri, Aug 6, 2021 at 12:08 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Thu, Aug 05, 2021 at 09:53:57PM +0200, Loic Poulain wrote: >> On Thu, 5 Aug 2021 at 20:38, Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> wrote: >>> >>> wwan_create_dev() is expected to return either valid pointer or NULL, >>> In some cases it might return the error pointer. Prevent this by converting >>> it to NULL after wwan_dev_get_by_parent(). >> >> wwan_create_dev is called both from wwan_register_ops() and >> wwan_create_port(), one using IS_ERR and the other using NULL testing, >> they should be aligned as well. > > Ah, good catch! > > I just sent v2, but eventually I have decided to switch to error pointer since > it seems the most used pattern in the code. I agree that returning the error pointer is a good solution here. Thank you for the fix. -- Sergey
diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c index 674a81d79db3..35e10a98e774 100644 --- a/drivers/net/wwan/wwan_core.c +++ b/drivers/net/wwan/wwan_core.c @@ -160,7 +160,9 @@ static struct wwan_device *wwan_create_dev(struct device *parent) /* If wwandev already exists, return it */ wwandev = wwan_dev_get_by_parent(parent); - if (!IS_ERR(wwandev)) + if (IS_ERR(wwandev)) + wwandev = NULL; + else goto done_unlock; id = ida_alloc(&wwan_dev_ids, GFP_KERNEL);
wwan_create_dev() is expected to return either valid pointer or NULL, In some cases it might return the error pointer. Prevent this by converting it to NULL after wwan_dev_get_by_parent(). Fixes: 9a44c1cc6388 ("net: Add a WWAN subsystem") Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/net/wwan/wwan_core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)