| Message ID | YNH0WU7BcQ/60UNG@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l2-subdev: fix some NULL vs IS_ERR() checks | expand |
Hi Dan, Thank you for the patch. On Tue, Jun 22, 2021 at 05:31:53PM +0300, Dan Carpenter wrote: > The v4l2_subdev_alloc_state() function returns error pointers, it > doesn't return NULL. It's funny you send this patch today, I've been thinking about the exact same issue yesterday, albeit more globally, when trying to figure out if a function I called returned NULL or an error pointer on error. Would it make to create an __err_ptr annotation to mark functions that return an error pointer ? This would both give a simple indication to the user and allow tools such as smatch to detect errors. > Fixes: 0d346d2a6f54 ("media: v4l2-subdev: add subdev-wide state struct") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Mauro, can you pick this patch up ? > --- > drivers/media/platform/vsp1/vsp1_entity.c | 4 ++-- > drivers/staging/media/tegra-video/vi.c | 4 ++-- > drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++-- > 3 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c > index 6f51e5c75543..823c15facd1b 100644 > --- a/drivers/media/platform/vsp1/vsp1_entity.c > +++ b/drivers/media/platform/vsp1/vsp1_entity.c > @@ -676,9 +676,9 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity, > * rectangles. > */ > entity->config = v4l2_subdev_alloc_state(&entity->subdev); > - if (entity->config == NULL) { > + if (IS_ERR(entity->config)) { > media_entity_cleanup(&entity->subdev.entity); > - return -ENOMEM; > + return PTR_ERR(entity->config); > } > > return 0; > diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c > index 89709cd06d4d..d321790b07d9 100644 > --- a/drivers/staging/media/tegra-video/vi.c > +++ b/drivers/staging/media/tegra-video/vi.c > @@ -508,8 +508,8 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan, > return -ENODEV; > > sd_state = v4l2_subdev_alloc_state(subdev); > - if (!sd_state) > - return -ENOMEM; > + if (IS_ERR(sd_state)) > + return PTR_ERR(sd_state); > /* > * Retrieve the format information and if requested format isn't > * supported, keep the current format. > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > index cca15a10c0b3..0d141155f0e3 100644 > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > @@ -253,8 +253,8 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which, > int ret; > > sd_state = v4l2_subdev_alloc_state(sd); > - if (sd_state == NULL) > - return -ENOMEM; > + if (IS_ERR(sd_state)) > + return PTR_ERR(sd_state); > > if (!rvin_format_from_pixel(vin, pix->pixelformat)) > pix->pixelformat = RVIN_DEFAULT_FORMAT;
On Tue, Jun 22, 2021 at 06:08:30PM +0300, Laurent Pinchart wrote: > Hi Dan, > > Thank you for the patch. > > On Tue, Jun 22, 2021 at 05:31:53PM +0300, Dan Carpenter wrote: > > The v4l2_subdev_alloc_state() function returns error pointers, it > > doesn't return NULL. > > It's funny you send this patch today, I've been thinking about the exact > same issue yesterday, albeit more globally, when trying to figure out if > a function I called returned NULL or an error pointer on error. > > Would it make to create an __err_ptr annotation to mark functions that > return an error pointer ? This would both give a simple indication to > the user and allow tools such as smatch to detect errors. > If you have the cross function DB enabled then Smatch can figure out if it returns error pointers or NULL. The big problem is that Smatch works on the precompiled code and doesn't understand ifdeffed code. I haven't pushed all the Smatch checks. I told someone last month, I'd give them a month to fix any bugs since it was their idea. But I'll push it soon. #if IS_ENABLED(CONFIG) function returns error pointer or valid #else struct foo *function() { return NULL; } #endif I believe that there are also people who use a two pass Coccinelle system where they make a list of functions that return error pointers and then check the callers. The Huawei devs find a bunch of these bugs through static analysis but I don't know which tools they are using. Today, I accidentally introduced a bug by converting a call that can "in theory/the future return error pointers" but also returns NULL at the end of a list. I thought it was only supposed to be checked for NULLs. Fortunately Colin King found it right away. That was just sloppiness on my part :/ and it's pretty rare to find code like that. regards, dan carpenter
On Tue, Jun 22, 2021 at 05:31:53PM +0300, Dan Carpenter wrote: > The v4l2_subdev_alloc_state() function returns error pointers, it > doesn't return NULL. > > Fixes: 0d346d2a6f54 ("media: v4l2-subdev: add subdev-wide state struct") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Thanks, Dan! Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
在 2021/6/22 23:58, Dan Carpenter 写道: > On Tue, Jun 22, 2021 at 06:08:30PM +0300, Laurent Pinchart wrote: >> Hi Dan, >> >> Thank you for the patch. >> >> On Tue, Jun 22, 2021 at 05:31:53PM +0300, Dan Carpenter wrote: >>> The v4l2_subdev_alloc_state() function returns error pointers, it >>> doesn't return NULL. >> It's funny you send this patch today, I've been thinking about the exact >> same issue yesterday, albeit more globally, when trying to figure out if >> a function I called returned NULL or an error pointer on error. >> >> Would it make to create an __err_ptr annotation to mark functions that >> return an error pointer ? This would both give a simple indication to >> the user and allow tools such as smatch to detect errors. >> > If you have the cross function DB enabled then Smatch can figure out if > it returns error pointers or NULL. The big problem is that Smatch works > on the precompiled code and doesn't understand ifdeffed code. > > I haven't pushed all the Smatch checks. I told someone last month, I'd > give them a month to fix any bugs since it was their idea. But I'll > push it soon. > > #if IS_ENABLED(CONFIG) > function returns error pointer or valid > #else > struct foo *function() { return NULL; } > #endif > > I believe that there are also people who use a two pass Coccinelle > system where they make a list of functions that return error pointers > and then check the callers. > > The Huawei devs find a bunch of these bugs through static analysis but > I don't know which tools they are using. Hi Dan, We are using Coccinelle script to found them. First step we using coccinelle script to found all the functions return ERR_PTR or NULL, and do filter by checking all the users: at least we found at least 5 callers, and all the caller check only NULL or ERR_PTR, then we add them to function list. Then using coccinelle script do analysis base on the function list give in step 1. Just do the same thing like smatch. Regards, Wei Yongjun
Hi Dan, On Tue, Jun 22, 2021 at 06:58:58PM +0300, Dan Carpenter wrote: > On Tue, Jun 22, 2021 at 06:08:30PM +0300, Laurent Pinchart wrote: > > On Tue, Jun 22, 2021 at 05:31:53PM +0300, Dan Carpenter wrote: > > > The v4l2_subdev_alloc_state() function returns error pointers, it > > > doesn't return NULL. > > > > It's funny you send this patch today, I've been thinking about the exact > > same issue yesterday, albeit more globally, when trying to figure out if > > a function I called returned NULL or an error pointer on error. > > > > Would it make to create an __err_ptr annotation to mark functions that > > return an error pointer ? This would both give a simple indication to > > the user and allow tools such as smatch to detect errors. > > If you have the cross function DB enabled then Smatch can figure out if > it returns error pointers or NULL. The big problem is that Smatch works > on the precompiled code and doesn't understand ifdeffed code. > > I haven't pushed all the Smatch checks. I told someone last month, I'd > give them a month to fix any bugs since it was their idea. But I'll > push it soon. > > #if IS_ENABLED(CONFIG) > function returns error pointer or valid > #else > struct foo *function() { return NULL; } > #endif Ouch, that hurts. > I believe that there are also people who use a two pass Coccinelle > system where they make a list of functions that return error pointers > and then check the callers. > > The Huawei devs find a bunch of these bugs through static analysis but > I don't know which tools they are using. > > Today, I accidentally introduced a bug by converting a call that can > "in theory/the future return error pointers" but also returns NULL at > the end of a list. I thought it was only supposed to be checked for > NULLs. Fortunately Colin King found it right away. That was just > sloppiness on my part :/ and it's pretty rare to find code like that. Do you think an annotation could still help, by making it explicit in headers whether a function returns NULL or an error pointer, thus helping developers get it right in the first place ?
On Wed, Jun 23, 2021 at 05:34:16AM +0300, Laurent Pinchart wrote: > > Do you think an annotation could still help, by making it explicit in > headers whether a function returns NULL or an error pointer, thus > helping developers get it right in the first place ? Not really. It wouldn't help with Smatch. I really think error pointer bugs are handled pretty well currently. Sometimes I have seen syzbot find them before the static checkers but I don't see them affecting users and production kernels. There are few other things that Smatch looks for like passing positives, valid pointers or NULLs to PTR_ERR(). I do wish that when functions return a mix of negative error codes, 0 and 1 that they had comment explaining what the 1 means. regards, dan carpenter
On Wed, Jun 23, 2021 at 12:03:26PM +0300, Dan Carpenter wrote: > On Wed, Jun 23, 2021 at 05:34:16AM +0300, Laurent Pinchart wrote: > > > > Do you think an annotation could still help, by making it explicit in > > headers whether a function returns NULL or an error pointer, thus > > helping developers get it right in the first place ? > > Not really. It wouldn't help with Smatch. I really think error pointer > bugs are handled pretty well currently. Sometimes I have seen syzbot > find them before the static checkers but I don't see them affecting > users and production kernels. I meant to ask if it would be useful for developers, not for smatch. When I use a function and have to figure out whether to use IS_ERR() or != NULL, I first look at the header, and most of the time I then need to find the corresponding implementation, wherever it may be. If we had an annotation, the second step could be skipped. Of course the annotation would need to match the implementation, and that's an area where smatch could help. > There are few other things that Smatch looks for like passing positives, > valid pointers or NULLs to PTR_ERR(). I do wish that when functions > return a mix of negative error codes, 0 and 1 that they had comment > explaining what the 1 means.
diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c index 6f51e5c75543..823c15facd1b 100644 --- a/drivers/media/platform/vsp1/vsp1_entity.c +++ b/drivers/media/platform/vsp1/vsp1_entity.c @@ -676,9 +676,9 @@ int vsp1_entity_init(struct vsp1_device *vsp1, struct vsp1_entity *entity, * rectangles. */ entity->config = v4l2_subdev_alloc_state(&entity->subdev); - if (entity->config == NULL) { + if (IS_ERR(entity->config)) { media_entity_cleanup(&entity->subdev.entity); - return -ENOMEM; + return PTR_ERR(entity->config); } return 0; diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c index 89709cd06d4d..d321790b07d9 100644 --- a/drivers/staging/media/tegra-video/vi.c +++ b/drivers/staging/media/tegra-video/vi.c @@ -508,8 +508,8 @@ static int __tegra_channel_try_format(struct tegra_vi_channel *chan, return -ENODEV; sd_state = v4l2_subdev_alloc_state(subdev); - if (!sd_state) - return -ENOMEM; + if (IS_ERR(sd_state)) + return PTR_ERR(sd_state); /* * Retrieve the format information and if requested format isn't * supported, keep the current format. diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c index cca15a10c0b3..0d141155f0e3 100644 --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c @@ -253,8 +253,8 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which, int ret; sd_state = v4l2_subdev_alloc_state(sd); - if (sd_state == NULL) - return -ENOMEM; + if (IS_ERR(sd_state)) + return PTR_ERR(sd_state); if (!rvin_format_from_pixel(vin, pix->pixelformat)) pix->pixelformat = RVIN_DEFAULT_FORMAT;
The v4l2_subdev_alloc_state() function returns error pointers, it doesn't return NULL. Fixes: 0d346d2a6f54 ("media: v4l2-subdev: add subdev-wide state struct") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/media/platform/vsp1/vsp1_entity.c | 4 ++-- drivers/staging/media/tegra-video/vi.c | 4 ++-- drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++-- 3 file changed, 6 insertions(+), 6 deletions(-)