[RESEND] drm/msm/dpu: Delete a variable initialisation before a null pointer check in two functions

Commit Message

Markus Elfring March 2, 2025, 8:56 p.m. UTC

From: Markus Elfring <[e-mail]>
Date: Tue, 11 Apr 2023 18:24:24 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the functions “dpu_hw_pp_enable_te” and “dpu_hw_pp_get_vsync_info”.

Thus avoid the risk for undefined behaviour by removing extra
initialisations for the variable “c” (also because it was already
reassigned with the same value behind this pointer check).

This issue was detected by using the Coccinelle software.

Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Markus Elfring <[e-mail]>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--
2.40.0

Comments

Dmitry Baryshkov March 2, 2025, 11:01 p.m. UTC | #1

On Sun, Mar 02, 2025 at 09:56:00PM +0100, Markus Elfring wrote:
> From: Markus Elfring <[e-mail]>
> Date: Tue, 11 Apr 2023 18:24:24 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the functions “dpu_hw_pp_enable_te” and “dpu_hw_pp_get_vsync_info”.
> 
> Thus avoid the risk for undefined behaviour by removing extra
> initialisations for the variable “c” (also because it was already
> reassigned with the same value behind this pointer check).
> 
> This issue was detected by using the Coccinelle software.

Please don't send resends and/or new iterations in response to your
previous patchsets. Otherwise they have a pretty high chance to be
ignored by the maintainers. Use a fresh git-send-email command to send
new patchset.

> 
> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> Signed-off-by: Markus Elfring <[e-mail]>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> index 0fcad9760b6f..870ab3ebbc94 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
> @@ -176,7 +176,7 @@ static int dpu_hw_pp_enable_te(struct dpu_hw_pingpong *pp, bool enable)
>  static int dpu_hw_pp_connect_external_te(struct dpu_hw_pingpong *pp,
>  		bool enable_external_te)
>  {
> -	struct dpu_hw_blk_reg_map *c = &pp->hw;
> +	struct dpu_hw_blk_reg_map *c;
>  	u32 cfg;
>  	int orig;
> 
> @@ -221,7 +221,7 @@ static int dpu_hw_pp_get_vsync_info(struct dpu_hw_pingpong *pp,
> 
>  static u32 dpu_hw_pp_get_line_count(struct dpu_hw_pingpong *pp)
>  {
> -	struct dpu_hw_blk_reg_map *c = &pp->hw;
> +	struct dpu_hw_blk_reg_map *c;
>  	u32 height, init;
>  	u32 line = 0xFFFF;
> 
> --
> 2.40.0
>

Dan Carpenter March 3, 2025, 7:14 a.m. UTC | #2

On Mon, Mar 03, 2025 at 01:01:40AM +0200, Dmitry Baryshkov wrote:
> On Sun, Mar 02, 2025 at 09:56:00PM +0100, Markus Elfring wrote:
> > From: Markus Elfring <[e-mail]>
> > Date: Tue, 11 Apr 2023 18:24:24 +0200
> > 
> > The address of a data structure member was determined before
> > a corresponding null pointer check in the implementation of
> > the functions “dpu_hw_pp_enable_te” and “dpu_hw_pp_get_vsync_info”.
> > 
> > Thus avoid the risk for undefined behaviour by removing extra
> > initialisations for the variable “c” (also because it was already
> > reassigned with the same value behind this pointer check).

There is no undefined behavior here.

> > 
> > This issue was detected by using the Coccinelle software.
> 
> Please don't send resends and/or new iterations in response to your
> previous patchsets. Otherwise they have a pretty high chance to be
> ignored by the maintainers. Use a fresh git-send-email command to send
> new patchset.
> 
> > 
> > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")

Remove the Fixes tag.  This patch is fine as a clean up.

> > Signed-off-by: Markus Elfring <[e-mail]>

regards,
dan carpenter

Markus Elfring March 3, 2025, 8:15 a.m. UTC | #3

>>> The address of a data structure member was determined before
>>> a corresponding null pointer check in the implementation of
>>> the functions “dpu_hw_pp_enable_te” and “dpu_hw_pp_get_vsync_info”.
>>>
>>> Thus avoid the risk for undefined behaviour by removing extra
>>> initialisations for the variable “c” (also because it was already
>>> reassigned with the same value behind this pointer check).
>
> There is no undefined behavior here.
Will any software development concerns evolve further also according to
undesirable null pointer dereferences?
https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers

Regards,
Markus

Dan Carpenter March 3, 2025, 8:24 a.m. UTC | #4

On Mon, Mar 03, 2025 at 09:15:14AM +0100, Markus Elfring wrote:
> >>> The address of a data structure member was determined before
> >>> a corresponding null pointer check in the implementation of
> >>> the functions “dpu_hw_pp_enable_te” and “dpu_hw_pp_get_vsync_info”.
> >>>
> >>> Thus avoid the risk for undefined behaviour by removing extra
> >>> initialisations for the variable “c” (also because it was already
> >>> reassigned with the same value behind this pointer check).
> >
> > There is no undefined behavior here.
> Will any software development concerns evolve further also according to
> undesirable null pointer dereferences?
> https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
> 

It's not a NULL pointer dereference.  It's just pointer math.  It was
a common way to implement offsetof() before we had a builtin for that.

samples/bpf/test_lru_dist.c
# define offsetof(TYPE, MEMBER) ((size_t)&((TYPE *)0)->MEMBER)

regards,
dan carpenter

Markus Elfring March 5, 2025, 8:40 a.m. UTC | #5

>>> The address of a data structure member was determined before
>>> a corresponding null pointer check in the implementation of
>>> the functions “dpu_hw_pp_enable_te” and “dpu_hw_pp_get_vsync_info”.
>>>
>>> Thus avoid the risk for undefined behaviour by removing extra
>>> initialisations for the variable “c” (also because it was already
>>> reassigned with the same value behind this pointer check).
> There is no undefined behavior here.

Is there a need to improve the wording precision?

There are words which denote a special meaning according to aspects of
the programming language “C”.
https://en.cppreference.com/w/c/language/behavior

Dereferences of null pointers are treated in special ways.
The system might be configurable to collaborate also with data accesses
together with the address “zero”.
Would you like to distinguish supported software functionality any further?

Regards,
Markus

Dan Carpenter March 5, 2025, 8:51 a.m. UTC | #6

On Wed, Mar 05, 2025 at 09:40:43AM +0100, Markus Elfring wrote:
> >>> The address of a data structure member was determined before
> >>> a corresponding null pointer check in the implementation of
> >>> the functions “dpu_hw_pp_enable_te” and “dpu_hw_pp_get_vsync_info”.
> >>>
> >>> Thus avoid the risk for undefined behaviour by removing extra
> >>> initialisations for the variable “c” (also because it was already
> >>> reassigned with the same value behind this pointer check).
> > There is no undefined behavior here.
> 
> Is there a need to improve the wording precision?
> 
> There are words which denote a special meaning according to aspects of
> the programming language “C”.
> https://en.cppreference.com/w/c/language/behavior
> 
> Dereferences of null pointers are treated in special ways.

This not a dereference.  It's just pointer math.

regards,
dan carpenter

Markus Elfring March 5, 2025, 9:20 a.m. UTC | #7

>> Dereferences of null pointers are treated in special ways.
>
> This not a dereference.  It's just pointer math.
Do you prefer the wording “member access through pointer” occasionally?
https://en.cppreference.com/w/c/language/operator_member_access

How do you tend to describe (and detect) null pointer dereferences so far?
https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers#EXP34C.Donotdereferencenullpointers-AutomatedDetection

Regards,
Markus

David Laight March 5, 2025, 2:17 p.m. UTC | #8

On Wed, 5 Mar 2025 11:51:59 +0300
Dan Carpenter <[e-mail]> wrote:

> On Wed, Mar 05, 2025 at 09:40:43AM +0100, Markus Elfring wrote:
> > >>> The address of a data structure member was determined before
> > >>> a corresponding null pointer check in the implementation of
> > >>> the functions “dpu_hw_pp_enable_te” and “dpu_hw_pp_get_vsync_info”.
> > >>>
> > >>> Thus avoid the risk for undefined behaviour by removing extra
> > >>> initialisations for the variable “c” (also because it was already
> > >>> reassigned with the same value behind this pointer check).  
> > > There is no undefined behavior here.  
> > 
> > Is there a need to improve the wording precision?
> > 
> > There are words which denote a special meaning according to aspects of
> > the programming language “C”.
> > https://en.cppreference.com/w/c/language/behavior
> > 
> > Dereferences of null pointers are treated in special ways.  
> 
> This not a dereference.  It's just pointer math.

And the 'fun' starts because NULL isn't required to use the all-zero
bit pattern.
Regardless of the bit-pattern, things like (void *)(1 - 1) are valid
NULL pointers.

Of course, while C allows this, I doubt NULL has ever been other than 0.
(It was 0 on a system I used many years ago where the O/S invalid pointer
was ~0.)

I know Clang has started warning about arithmetic on NULL.
I wonder when it is going to start warning about memset(p, 0, sz)
for anything that contains a pointer - equally invalid.

	David

Dan Carpenter March 5, 2025, 2:30 p.m. UTC | #9

On Wed, Mar 05, 2025 at 02:17:32PM +0000, David Laight wrote:
> On Wed, 5 Mar 2025 11:51:59 +0300
> Dan Carpenter <[e-mail]> wrote:
> 
> > On Wed, Mar 05, 2025 at 09:40:43AM +0100, Markus Elfring wrote:
> > > >>> The address of a data structure member was determined before
> > > >>> a corresponding null pointer check in the implementation of
> > > >>> the functions “dpu_hw_pp_enable_te” and “dpu_hw_pp_get_vsync_info”.
> > > >>>
> > > >>> Thus avoid the risk for undefined behaviour by removing extra
> > > >>> initialisations for the variable “c” (also because it was already
> > > >>> reassigned with the same value behind this pointer check).  
> > > > There is no undefined behavior here.  
> > > 
> > > Is there a need to improve the wording precision?
> > > 
> > > There are words which denote a special meaning according to aspects of
> > > the programming language “C”.
> > > https://en.cppreference.com/w/c/language/behavior
> > > 
> > > Dereferences of null pointers are treated in special ways.  
> > 
> > This not a dereference.  It's just pointer math.
> 
> And the 'fun' starts because NULL isn't required to use the all-zero
> bit pattern.
> Regardless of the bit-pattern, things like (void *)(1 - 1) are valid
> NULL pointers.
> 
> Of course, while C allows this, I doubt NULL has ever been other than 0.
> (It was 0 on a system I used many years ago where the O/S invalid pointer
> was ~0.)

Kernel style guidelines don't even allow if (p == NULL) so we would be
screwed.  :P

> 
> I know Clang has started warning about arithmetic on NULL.

Huh.  You're right.

$ clang -Weverything test.c
test.c:13:22: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   13 |         printf("%p\n", NULL + 1);
      |                        ~~~~ ^
test.c:13:22: warning: arithmetic on a pointer to void is a GNU extension [-Wgnu-pointer-arith]
   13 |         printf("%p\n", NULL + 1);
      |                        ~~~~ ^
test.c:11:14: warning: unused parameter 'argc' [-Wunused-parameter]
   11 | int main(int argc, char *argv[])
      |              ^
test.c:11:26: warning: unused parameter 'argv' [-Wunused-parameter]
   11 | int main(int argc, char *argv[])
      |                          ^
test.c:13:17: warning: unsafe pointer arithmetic [-Wunsafe-buffer-usage]
   13 |         printf("%p\n", NULL + 1);
      |                        ^~~~
/usr/lib/llvm-19/lib/clang/19/include/__stddef_null.h:26:14: note: expanded from macro 'NULL'
   26 | #define NULL ((void*)0)
      |              ^~~~~~~~~~
5 warnings generated.

Well, that's stupid.

regards,
dan carpenter

David Laight March 5, 2025, 9:35 p.m. UTC | #10

On Wed, 5 Mar 2025 17:30:28 +0300
Dan Carpenter <[e-mail]> wrote:

> On Wed, Mar 05, 2025 at 02:17:32PM +0000, David Laight wrote:
...
> > And the 'fun' starts because NULL isn't required to use the all-zero
> > bit pattern.
> > Regardless of the bit-pattern, things like (void *)(1 - 1) are valid
> > NULL pointers.
> > 
> > Of course, while C allows this, I doubt NULL has ever been other than 0.
> > (It was 0 on a system I used many years ago where the O/S invalid pointer
> > was ~0.)  
> 
> Kernel style guidelines don't even allow if (p == NULL) so we would be
> screwed.  :P

Doesn't matter:
	if (!p) ...
	if (p == 0) ...
	if (p == (void *)0) ...
	if (p == NULL) ...
	if (p == (void *)(constant integer expression with value 0)) ...
and the equivalent assignments all behave the same regardless of the
bit-pattern use for NULL.
So:
	union { long l; void *p; } lpu;
	lpu.p = 0;
	return lpu.l;
Returns ABI (implementation) defined constant value.
I think the only requirement is that it can never be the address
of a valid variable.

	David

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
index 0fcad9760b6f..870ab3ebbc94 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
@@ -176,7 +176,7 @@  static int dpu_hw_pp_enable_te(struct dpu_hw_pingpong *pp, bool enable)
 static int dpu_hw_pp_connect_external_te(struct dpu_hw_pingpong *pp,
 		bool enable_external_te)
 {
-	struct dpu_hw_blk_reg_map *c = &pp->hw;
+	struct dpu_hw_blk_reg_map *c;
 	u32 cfg;
 	int orig;

@@ -221,7 +221,7 @@  static int dpu_hw_pp_get_vsync_info(struct dpu_hw_pingpong *pp,

 static u32 dpu_hw_pp_get_line_count(struct dpu_hw_pingpong *pp)
 {
-	struct dpu_hw_blk_reg_map *c = &pp->hw;
+	struct dpu_hw_blk_reg_map *c;
 	u32 height, init;
 	u32 line = 0xFFFF;