Hi all,
Rainy w/e here and I got a bit bored, so looked at coverity again. I've closed all outstanding issues in drivers/gpu now either as false positives or fixed in this series expect for vmwgfx/ttm stuff (not enough clue) and one insane savage init issue (no desire to wake dragons today).
Cheers, Daniel
Daniel Vetter (13): drm/mgag200: Remove unnecessary NULL check in bo_unref drm/mgag200: Remove unecessary NULL check in gem_free drm/cirrus: Remove unnecessary NULL check in bo_unref drm/cirrus: Remove unecessary NULL check in gem_free drm/ast: Remove unnecessary NULL check in bo_unref drm/mgag200: Remove unecessary NULL check in gem_free drm/via: Remove unecessary NULL check drm/ast: Remove dead code from cbr_scan2 drm/udl: Initialize ret in udl_driver_load drm/bochs: Remove unnecessary NULL check in bo_unref drm/bochs: Remove unecessary NULL check in gem_free drm/i2c/tda998x: Fix signed overflow issue drm: Fix error handling in drm_master_create
drivers/gpu/drm/ast/ast_main.c | 7 ++----- drivers/gpu/drm/ast/ast_post.c | 2 -- drivers/gpu/drm/bochs/bochs_mm.c | 6 +----- drivers/gpu/drm/cirrus/cirrus_main.c | 6 +----- drivers/gpu/drm/drm_stub.c | 5 ++++- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- drivers/gpu/drm/mgag200/mgag200_main.c | 6 +----- drivers/gpu/drm/udl/udl_main.c | 1 + drivers/gpu/drm/via/via_mm.c | 2 +- 9 files changed, 12 insertions(+), 25 deletions(-)
Hi all,
Rainy w/e here and I got a bit bored, so looked at coverity again. I've closed all outstanding issues in drivers/gpu now either as false positives or fixed in this series expect for vmwgfx/ttm stuff (not enough clue) and one insane savage init issue (no desire to wake dragons today).
Cheers, Daniel
Daniel Vetter (13): drm/mgag200: Remove unnecessary NULL check in bo_unref drm/mgag200: Remove unecessary NULL check in gem_free drm/cirrus: Remove unnecessary NULL check in bo_unref drm/cirrus: Remove unecessary NULL check in gem_free drm/ast: Remove unnecessary NULL check in bo_unref drm/mgag200: Remove unecessary NULL check in gem_free drm/via: Remove unecessary NULL check drm/ast: Remove dead code from cbr_scan2 drm/udl: Initialize ret in udl_driver_load drm/bochs: Remove unnecessary NULL check in bo_unref drm/bochs: Remove unecessary NULL check in gem_free drm/i2c/tda998x: Fix signed overflow issue drm: Fix error handling in drm_master_create
drivers/gpu/drm/ast/ast_main.c | 7 ++----- drivers/gpu/drm/ast/ast_post.c | 2 -- drivers/gpu/drm/bochs/bochs_mm.c | 6 +----- drivers/gpu/drm/cirrus/cirrus_main.c | 6 +----- drivers/gpu/drm/drm_stub.c | 5 ++++- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- drivers/gpu/drm/mgag200/mgag200_main.c | 6 +----- drivers/gpu/drm/udl/udl_main.c | 1 + drivers/gpu/drm/via/via_mm.c | 2 +- 9 files changed, 12 insertions(+), 25 deletions(-)
Hi
On Sat, Apr 5, 2014 at 11:44 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
Hi all,
Rainy w/e here and I got a bit bored, so looked at coverity again. I've closed all outstanding issues in drivers/gpu now either as false positives or fixed in this series expect for vmwgfx/ttm stuff (not enough clue) and one insane savage init issue (no desire to wake dragons today).
All patches besides "drm/ast: Remove dead code from cbr_scan2" are:
Reviewed-by: David Herrmann dh.herrmann@gmail.com
However, a few notes: - ttm_bo_unref() sets "bo = NULL;", so the following condition is always true. Your commit-message argues with an unconditional kref_unref(). I think that's misleading (but still true) as that does not change the fact that bo is always NULL afterwards. - I am a big fan of "if (!obj) return;" in destructors. It simplifies error-paths considerably and we can call them despite objects being already cleared. But that's just personal style, and I don't deal much with TTM anyway. But generally, I think we shouldn't remove these checks blindly. - The AST patch should be reviewed by someone who knows that hw. The code is obviously wrong, but that doesn't mean we cannot fix it properly.
Thanks for the cleanups, the coverity reports have been pending here for months.. David
ttm_bo_unref unconditionally calls kref_put on it's argument, so the thing can't be NULL without already causing Oopses.
Spotted by coverity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/mgag200/mgag200_main.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index 26868e5c55b0..0722d18992f4 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -322,9 +322,7 @@ static void mgag200_bo_unref(struct mgag200_bo **bo)
tbo = &((*bo)->bo); ttm_bo_unref(&tbo); - if (tbo == NULL) - *bo = NULL; - + *bo = NULL; }
void mgag200_gem_free_object(struct drm_gem_object *obj)
On 04/05/2014 02:44 AM, Daniel Vetter wrote:
ttm_bo_unref unconditionally calls kref_put on it's argument, so the thing can't be NULL without already causing Oopses.
Doesn't this mean the NULL check is in the wrong place (rather than the NULL check should be removed)?
Spotted by coverity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/mgag200/mgag200_main.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index 26868e5c55b0..0722d18992f4 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -322,9 +322,7 @@ static void mgag200_bo_unref(struct mgag200_bo **bo)
tbo = &((*bo)->bo); ttm_bo_unref(&tbo);
- if (tbo == NULL)
*bo = NULL;
- *bo = NULL;
}
void mgag200_gem_free_object(struct drm_gem_object *obj)
On Mon, Apr 7, 2014 at 5:19 PM, Ian Romanick idr@freedesktop.org wrote:
On 04/05/2014 02:44 AM, Daniel Vetter wrote:
ttm_bo_unref unconditionally calls kref_put on it's argument, so the thing can't be NULL without already causing Oopses.
Doesn't this mean the NULL check is in the wrong place (rather than the NULL check should be removed)?
Afaics chasing callchains it's a bug to call this with NULL pointer and no one really should be doing it. Like David Herrmann said it's sometimes useful if unref/free functions automatically cope with NULL, but ttm buffers don't seem to be of this kind. So consistency with other ttm drivers seems better, same with all the gem_free_object callbacks. -Daniel
On 04/07/2014 12:56 PM, Daniel Vetter wrote:
On Mon, Apr 7, 2014 at 5:19 PM, Ian Romanick idr@freedesktop.org wrote:
On 04/05/2014 02:44 AM, Daniel Vetter wrote:
ttm_bo_unref unconditionally calls kref_put on it's argument, so the thing can't be NULL without already causing Oopses.
Doesn't this mean the NULL check is in the wrong place (rather than the NULL check should be removed)?
Afaics chasing callchains it's a bug to call this with NULL pointer and no one really should be doing it. Like David Herrmann said it's sometimes useful if unref/free functions automatically cope with NULL, but ttm buffers don't seem to be of this kind. So consistency with other ttm drivers seems better, same with all the gem_free_object callbacks.
That's fair. I'm convinced. Patches 1, 3, and 5 are also
Reviewed-by: Ian Romanick ian.d.romanick@intel.com
-Daniel
The ->gem_free_object never gets called with a NULL pointer, the check is redundant. Also checking after the upcast allows compilers to elide it anyway.
Spotted by coverity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/mgag200/mgag200_main.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index 0722d18992f4..f6b283b8375e 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -329,8 +329,6 @@ void mgag200_gem_free_object(struct drm_gem_object *obj) { struct mgag200_bo *mgag200_bo = gem_to_mga_bo(obj);
- if (!mgag200_bo) - return; mgag200_bo_unref(&mgag200_bo); }
On 04/05/2014 02:44 AM, Daniel Vetter wrote:
The ->gem_free_object never gets called with a NULL pointer, the check is redundant. Also checking after the upcast allows compilers to elide it anyway.
Right... because if obj was NULL, subtracting some offset from it won't still be NULL.
Spotted by coverity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
Reviewed-by: Ian Romanick ian.d.romanick@intel.com
drivers/gpu/drm/mgag200/mgag200_main.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index 0722d18992f4..f6b283b8375e 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -329,8 +329,6 @@ void mgag200_gem_free_object(struct drm_gem_object *obj) { struct mgag200_bo *mgag200_bo = gem_to_mga_bo(obj);
- if (!mgag200_bo)
return;}
ttm_bo_unref unconditionally calls kref_put on it's argument, so the thing can't be NULL without already causing Oopses.
Spotted by coverity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/cirrus/cirrus_main.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c index 4b0170cf53fd..b119f66fed92 100644 --- a/drivers/gpu/drm/cirrus/cirrus_main.c +++ b/drivers/gpu/drm/cirrus/cirrus_main.c @@ -264,9 +264,7 @@ static void cirrus_bo_unref(struct cirrus_bo **bo)
tbo = &((*bo)->bo); ttm_bo_unref(&tbo); - if (tbo == NULL) - *bo = NULL; - + *bo = NULL; }
void cirrus_gem_free_object(struct drm_gem_object *obj)
The ->gem_free_object never gets called with a NULL pointer, the check is redundant. Also checking after the upcast allows compilers to elide it anyway.
Spotted by coverity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/cirrus/cirrus_main.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c index b119f66fed92..99c1983f99d2 100644 --- a/drivers/gpu/drm/cirrus/cirrus_main.c +++ b/drivers/gpu/drm/cirrus/cirrus_main.c @@ -271,8 +271,6 @@ void cirrus_gem_free_object(struct drm_gem_object *obj) { struct cirrus_bo *cirrus_bo = gem_to_cirrus_bo(obj);
- if (!cirrus_bo) - return; cirrus_bo_unref(&cirrus_bo); }
On 04/05/2014 02:44 AM, Daniel Vetter wrote:
The ->gem_free_object never gets called with a NULL pointer, the check is redundant. Also checking after the upcast allows compilers to elide it anyway.
Spotted by coverity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
Same as MGA change.
Reviewed-by: Ian Romanick ian.d.romanick@intel.com
drivers/gpu/drm/cirrus/cirrus_main.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c index b119f66fed92..99c1983f99d2 100644 --- a/drivers/gpu/drm/cirrus/cirrus_main.c +++ b/drivers/gpu/drm/cirrus/cirrus_main.c @@ -271,8 +271,6 @@ void cirrus_gem_free_object(struct drm_gem_object *obj) { struct cirrus_bo *cirrus_bo = gem_to_cirrus_bo(obj);
- if (!cirrus_bo)
return;}
ttm_bo_unref unconditionally calls kref_put on it's argument, so the thing can't be NULL without already causing Oopses.
Spotted by coverity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/ast/ast_main.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 50535fd5a88d..38941a656312 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -411,10 +411,9 @@ static void ast_bo_unref(struct ast_bo **bo)
tbo = &((*bo)->bo); ttm_bo_unref(&tbo); - if (tbo == NULL) - *bo = NULL; - + *bo = NULL; } + void ast_gem_free_object(struct drm_gem_object *obj) { struct ast_bo *ast_bo = gem_to_ast_bo(obj);
The ->gem_free_object never gets called with a NULL pointer, the check is redundant. Also checking after the upcast allows compilers to elide it anyway.
Spotted by coverity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/ast/ast_main.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 38941a656312..01bf9e730acf 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -418,8 +418,6 @@ void ast_gem_free_object(struct drm_gem_object *obj) { struct ast_bo *ast_bo = gem_to_ast_bo(obj);
- if (!ast_bo) - return; ast_bo_unref(&ast_bo); }
The ->gem_free_object never gets called with a NULL pointer, the check is redundant. Also checking after the upcast allows compilers to elide it anyway.
Spotted by coverity.
v2: Fix patch subject.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/ast/ast_main.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 38941a656312..01bf9e730acf 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -418,8 +418,6 @@ void ast_gem_free_object(struct drm_gem_object *obj) { struct ast_bo *ast_bo = gem_to_ast_bo(obj);
- if (!ast_bo) - return; ast_bo_unref(&ast_bo); }
On 04/05/2014 02:44 AM, Daniel Vetter wrote:
The ->gem_free_object never gets called with a NULL pointer, the check is redundant. Also checking after the upcast allows compilers to elide it anyway.
Spotted by coverity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
Same as MGA change.
Reviewed-by: Ian Romanick ian.d.romanick@intel.com
drivers/gpu/drm/ast/ast_main.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 38941a656312..01bf9e730acf 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -418,8 +418,6 @@ void ast_gem_free_object(struct drm_gem_object *obj) { struct ast_bo *ast_bo = gem_to_ast_bo(obj);
- if (!ast_bo)
return;}
The context_dtor callback is only called once we've successfully loaded the driver, which means dev->dev_private is set up. The check is hence pointless.
Also dev->dev_private is deref already above, so compilers are free to elide it anyway.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/via/via_mm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/via/via_mm.c b/drivers/gpu/drm/via/via_mm.c index 927889105483..d70b1e1544bf 100644 --- a/drivers/gpu/drm/via/via_mm.c +++ b/drivers/gpu/drm/via/via_mm.c @@ -79,7 +79,7 @@ int via_final_context(struct drm_device *dev, int context)
/* Linux specific until context tracking code gets ported to BSD */ /* Last context, perform cleanup */ - if (list_is_singular(&dev->ctxlist) && dev->dev_private) { + if (list_is_singular(&dev->ctxlist)) { DRM_DEBUG("Last Context\n"); drm_irq_uninstall(dev); via_cleanup_futex(dev_priv);
Hi
On Sat, Apr 5, 2014 at 11:44 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
The context_dtor callback is only called once we've successfully loaded the driver, which means dev->dev_private is set up. The check is hence pointless.
Also dev->dev_private is deref already above, so compilers are free to elide it anyway.
Are you sure compilers can assume "*ptr" implies "ptr != NULL"? I doubt that and depending on CONFIG_DEFAULT_MMAP_MIN_ADDR I think you can even build user-space that can successfully mmap(MAP_FIXED) at address 0. Anyhow, I guess no-one cares besides me, so patch looks good :)
Thanks David
On Mon, Apr 7, 2014 at 5:51 PM, David Herrmann dh.herrmann@gmail.com wrote:
On Sat, Apr 5, 2014 at 11:44 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
The context_dtor callback is only called once we've successfully loaded the driver, which means dev->dev_private is set up. The check is hence pointless.
Also dev->dev_private is deref already above, so compilers are free to elide it anyway.
Are you sure compilers can assume "*ptr" implies "ptr != NULL"? I doubt that and depending on CONFIG_DEFAULT_MMAP_MIN_ADDR I think you can even build user-space that can successfully mmap(MAP_FIXED) at address 0. Anyhow, I guess no-one cares besides me, so patch looks good :)
Yeah, my understand has been that every time you deref a pointer somewhere the compiler is allowed to presume that the pointer isn't NULL. Which makes mmap(MAP_FIXED) at address NULL such a dangerous thing and iirc there's been patches floating around to severely restrict that to make exploiting such bugs much harder. Iirc it's only emulators like dosemu who really need to be able to map something at NULL. Since if gcc drops the NULL check the last line of defense (namely Oopsing on the NULL deref) can be disabled by userspace. The usual exploit is to put a real data structure at NULL and use that (thorugh vtables if possible) to take over the kernel.
I'm not always entirely sure on what the precise rules are really in detail, but since coverity screamed at me about this here I've figured coverity is probably right ;-) -Daniel
The outer if already checks for data != 0, so it can't really be 0. Hence remove it.
Now I don't have specs or anything for this beast, so I have no idea whether this was actually intended or whether the logic should be different. At least the code still seems to be doing something useful.
Spotted by coverity.
Cc: Dave Airlie airlied@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/ast/ast_post.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c index 977cfb35837a..6263116054b6 100644 --- a/drivers/gpu/drm/ast/ast_post.c +++ b/drivers/gpu/drm/ast/ast_post.c @@ -572,8 +572,6 @@ static u32 cbr_scan2(struct ast_private *ast) for (loop = 0; loop < CBR_PASSNUM2; loop++) { if ((data = cbr_test2(ast)) != 0) { data2 &= data; - if (!data) - return 0; break; } }
On 04/05/2014 02:44 AM, Daniel Vetter wrote:
The outer if already checks for data != 0, so it can't really be 0. Hence remove it.
Now I don't have specs or anything for this beast, so I have no idea whether this was actually intended or whether the logic should be different. At least the code still seems to be doing something useful.
Spotted by coverity.
Cc: Dave Airlie airlied@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/ast/ast_post.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c index 977cfb35837a..6263116054b6 100644 --- a/drivers/gpu/drm/ast/ast_post.c +++ b/drivers/gpu/drm/ast/ast_post.c @@ -572,8 +572,6 @@ static u32 cbr_scan2(struct ast_private *ast) for (loop = 0; loop < CBR_PASSNUM2; loop++) { if ((data = cbr_test2(ast)) != 0) { data2 &= data;
if (!data)return 0;
That feels like a typo... was that supposed to be 'if (!data2)'?
break; } }
On Mon, Apr 7, 2014 at 5:28 PM, Ian Romanick idr@freedesktop.org wrote:
On 04/05/2014 02:44 AM, Daniel Vetter wrote:
The outer if already checks for data != 0, so it can't really be 0. Hence remove it.
Now I don't have specs or anything for this beast, so I have no idea whether this was actually intended or whether the logic should be different. At least the code still seems to be doing something useful.
Spotted by coverity.
Cc: Dave Airlie airlied@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/ast/ast_post.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c index 977cfb35837a..6263116054b6 100644 --- a/drivers/gpu/drm/ast/ast_post.c +++ b/drivers/gpu/drm/ast/ast_post.c @@ -572,8 +572,6 @@ static u32 cbr_scan2(struct ast_private *ast) for (loop = 0; loop < CBR_PASSNUM2; loop++) { if ((data = cbr_test2(ast)) != 0) { data2 &= data;
if (!data)return 0;That feels like a typo... was that supposed to be 'if (!data2)'?
Yeah this one really needs a close look, since I have no idea what's actually intended behaviour. The patch just removes the dead code as it is now, and the double-loop still makes some sense imo after this change. But I really don't know the spec for this hw. -Daniel
We need to set it to -ENODEV when we don't recognize the device. Otherwise we return/print stack garbage.
Spotted by coverity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/udl/udl_main.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index f5ae57406f34..e1038a945f40 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -294,6 +294,7 @@ int udl_driver_load(struct drm_device *dev, unsigned long flags) dev->dev_private = udl;
if (!udl_parse_vendor_descriptor(dev, dev->usbdev)) { + ret = -ENODEV; DRM_ERROR("firmware not recognized. Assume incompatible device\n"); goto err; }
ttm_bo_unref unconditionally calls kref_put on it's argument, so the thing can't be NULL without already causing Oopses.
Spotted by coverity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/bochs/bochs_mm.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c index f488be55d650..4a239e931aff 100644 --- a/drivers/gpu/drm/bochs/bochs_mm.c +++ b/drivers/gpu/drm/bochs/bochs_mm.c @@ -434,9 +434,7 @@ static void bochs_bo_unref(struct bochs_bo **bo)
tbo = &((*bo)->bo); ttm_bo_unref(&tbo); - if (tbo == NULL) - *bo = NULL; - + *bo = NULL; }
void bochs_gem_free_object(struct drm_gem_object *obj)
The ->gem_free_object never gets called with a NULL pointer, the check is redundant. Also checking after the upcast allows compilers to elide it anyway.
Noticed while chasing coverity reports, somehow this one here was not flagged.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/bochs/bochs_mm.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c index 4a239e931aff..b9a695d92792 100644 --- a/drivers/gpu/drm/bochs/bochs_mm.c +++ b/drivers/gpu/drm/bochs/bochs_mm.c @@ -441,8 +441,6 @@ void bochs_gem_free_object(struct drm_gem_object *obj) { struct bochs_bo *bochs_bo = gem_to_bochs_bo(obj);
- if (!bochs_bo) - return; bochs_bo_unref(&bochs_bo); }
On 04/05/2014 02:45 AM, Daniel Vetter wrote:
The ->gem_free_object never gets called with a NULL pointer, the check is redundant. Also checking after the upcast allows compilers to elide it anyway.
Noticed while chasing coverity reports, somehow this one here was not flagged.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
Same as MGA change.
Reviewed-by: Ian Romanick ian.d.romanick@intel.com
drivers/gpu/drm/bochs/bochs_mm.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c index 4a239e931aff..b9a695d92792 100644 --- a/drivers/gpu/drm/bochs/bochs_mm.c +++ b/drivers/gpu/drm/bochs/bochs_mm.c @@ -441,8 +441,6 @@ void bochs_gem_free_object(struct drm_gem_object *obj) { struct bochs_bo *bochs_bo = gem_to_bochs_bo(obj);
- if (!bochs_bo)
return;}
This is C standard hair-splitting, but afaict - sum will be promoted to signed int in computation since uint8_t fits - signed overflow is undefined.
No we need to add up an awful lot of bytes to actually make it overflow. But I guess the real risk is gcc spotting this and going bananas. Fix this by simply using unsigned in to force all computations to use the well-defined unsigned behaviour.
Spotted by coverity.
Cc: Russell King rmk+kernel@arm.linux.org.uk Cc: Rob Clark robdclark@gmail.com Cc: Jean-Francois Moine moinejf@free.fr Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 48af5cac1902..ae2754760d77 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -568,7 +568,7 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes) { - uint8_t sum = 0; + unsigned sum = 0;
while (bytes--) sum += *buf++;
On 04/05/2014 02:45 AM, Daniel Vetter wrote:
This is C standard hair-splitting, but afaict
- sum will be promoted to signed int in computation since uint8_t fits
- signed overflow is undefined.
No we need to add up an awful lot of bytes to actually make it
^^ Now
overflow. But I guess the real risk is gcc spotting this and going bananas. Fix this by simply using unsigned in to force all computations to use the well-defined unsigned behaviour.
Seems reasonable... it also seems impossible (ha!) to break anything.
Reviewed-by: Ian Romanick ian.d.romanick@intel.com
Spotted by coverity.
Cc: Russell King rmk+kernel@arm.linux.org.uk Cc: Rob Clark robdclark@gmail.com Cc: Jean-Francois Moine moinejf@free.fr Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 48af5cac1902..ae2754760d77 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -568,7 +568,7 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes) {
- uint8_t sum = 0;
unsigned sum = 0;
while (bytes--) sum += *buf++;
We need to check whether drm_ht_create succeed and clean up if not.
Spotted by coverity.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_stub.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c index dc2c6095d850..a7f22822371c 100644 --- a/drivers/gpu/drm/drm_stub.c +++ b/drivers/gpu/drm/drm_stub.c @@ -131,7 +131,10 @@ struct drm_master *drm_master_create(struct drm_minor *minor) kref_init(&master->refcount); spin_lock_init(&master->lock.spinlock); init_waitqueue_head(&master->lock.lock_queue); - drm_ht_create(&master->magiclist, DRM_MAGIC_HASH_ORDER); + if (drm_ht_create(&master->magiclist, DRM_MAGIC_HASH_ORDER)) { + kfree(master); + return NULL; + } INIT_LIST_HEAD(&master->magicfree); master->minor = minor;
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
David Herrmann -
Ian Romanick