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

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

[Dmitry Baryshkov]

On 11/04/2023 19:38, Markus Elfring wrote:
> 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: 25fdd5933e4c0f5fe2ea5cd59994f8ac5fbe90ef ("drm/msm: Add SDM845 DPU support")

Plese follow the format for the Fixes tags and limit the hash to 12 
chars. Proper tag:

Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")

Other than that LGTM.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   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
> 

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

On 4/11/2023 9:38 AM, Markus Elfring wrote:
> 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: 25fdd5933e4c0f5fe2ea5cd59994f8ac5fbe90ef ("drm/msm: Add SDM845 DPU support")

Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")

We usually have 12 chars of the hash. Other than that,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   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
> 

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

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

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

[Dmitry Baryshkov]

On Sun, Mar 02, 2025 at 09:56:00PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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 <elfring@users.sourceforge.net>
> ---
>  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
> 

-- 
With best wishes
Dmitry

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

[Dan Carpenter]

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 <elfring@users.sourceforge.net>
> > 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 <elfring@users.sourceforge.net>

regards,
dan carpenter

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

[Markus Elfring]

>>> 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

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

[Dan Carpenter]

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

Re: [RFC] Clarification for “undefined behaviour”?

[Markus Elfring]

>>> 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

Re: [RFC] Clarification for “undefined behaviour”?

[Dan Carpenter]

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

Re: [RFC] Clarification for “undefined behaviour”?

[Markus Elfring]

>> 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

Re: [RFC] Clarification for “undefined behaviour”?

[David Laight]

On Wed, 5 Mar 2025 11:51:59 +0300
Dan Carpenter <dan.carpenter@linaro.org> 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

Re: [RFC] Clarification for “undefined behaviour”?

[Dan Carpenter]

On Wed, Mar 05, 2025 at 02:17:32PM +0000, David Laight wrote:
> On Wed, 5 Mar 2025 11:51:59 +0300
> Dan Carpenter <dan.carpenter@linaro.org> 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

Re: [RFC] Clarification for “undefined behaviour”?

[David Laight]

On Wed, 5 Mar 2025 17:30:28 +0300
Dan Carpenter <dan.carpenter@linaro.org> 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

Re: [PATCH] perf map: Delete two variable initialisations before null pointer checks in sort__sym_from_cmp()

[Ian Rogers]

On Thu, Apr 13, 2023 at 6:03 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> Date: Thu, 13 Apr 2023 14:46:39 +0200
>
> Addresses of two data structure members were determined before
> corresponding null pointer checks in the implementation of
> the function “sort__sym_from_cmp”.
>
> Thus avoid the risk for undefined behaviour by removing extra
> initialisations for the local variables “from_l” and “from_r”
> (also because they were already reassigned with the same value
> behind this pointer check).
>
> This issue was detected by using the Coccinelle software.
>
> Fixes: 1b9e97a2a95e4941dcfa968c4b2e04022e9a343e ("perf tools: Fix report -F symbol_from for data without branch info")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/sort.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 80c9960c37e5..f2ffaf90648e 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -1020,8 +1020,7 @@ static int hist_entry__dso_to_filter(struct hist_entry *he, int type,
>  static int64_t
>  sort__sym_from_cmp(struct hist_entry *left, struct hist_entry *right)
>  {
> -       struct addr_map_symbol *from_l = &left->branch_info->from;
> -       struct addr_map_symbol *from_r = &right->branch_info->from;
> +       struct addr_map_symbol *from_l, *from_r;
>
>         if (!left->branch_info || !right->branch_info)
>                 return cmp_null(left->branch_info, right->branch_info);
> --
> 2.40.0
>

Re: [PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()

[Alison Schofield]

On Fri, Apr 14, 2023 at 12:12:37PM +0200, Markus Elfring wrote:
> Date: Fri, 14 Apr 2023 12:01:15 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “nd_pfn_validate”.
> 
> Thus avoid the risk for undefined behaviour by replacing the usage of
> the local variable “parent_uuid” by a direct function call within
> a later condition check.

Hi Markus,

I think I understand what you are saying above, but I don't follow
how that applies here. This change seems to be a nice simplification,
parent_uuid, is used once, just grab it when needed.

What is the risk of undefined behavior?

> 
> This issue was detected by using the Coccinelle software.
Which cocci script?

> 
> Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add uuid helpers")

This fixes tag seems to be the wrong tag. It is a tag from when the
uuid helpers were introduce, not where parent_uuid was first introduced
and used. I'm not clear this warrants a Fixes tag anyway. Is there
really a bug here? Perhaps I'm missing something in the previous
explanation of risk.

checkpatch is WARNING on the tag format:
WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: d1c6e08e7503 ("libnvdimm/labels: Add uuid helpers")'
#17:
    Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add uuid helpers")

checkpatch is also WARNING on the commit msg:
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#5:
    nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()

Also, possible only my pet peeve, the long commit message spoils my
pretty 80 column view. Please trim it to not wrap here:

$git log --oneline pfn_devs.c
52b639e56a46 nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()
c91d71363084 nvdimm: Support sizeof(struct page) > MAX_STRUCT_PAGE_SIZE
6e9f05dc66f9 libnvdimm/pfn_dev: increase MAX_STRUCT_PAGE_SIZE
81beea55cb74 nvdimm: Drop nd_device_lock()
4a0079bc7aae nvdimm: Replace lockdep_mutex with local lock classes
322cbb50de71 block: remove genhd.h

Alison


> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/nvdimm/pfn_devs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index af7d9301520c..f14cbfa500ed 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -456,7 +456,6 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
>  	unsigned long align, start_pad;
>  	struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
>  	struct nd_namespace_common *ndns = nd_pfn->ndns;
> -	const uuid_t *parent_uuid = nd_dev_to_uuid(&ndns->dev);
> 
>  	if (!pfn_sb || !ndns)
>  		return -ENODEV;
> @@ -476,7 +475,7 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
>  		return -ENODEV;
>  	pfn_sb->checksum = cpu_to_le64(checksum);
> 
> -	if (memcmp(pfn_sb->parent_uuid, parent_uuid, 16) != 0)
> +	if (memcmp(pfn_sb->parent_uuid, nd_dev_to_uuid(&ndns->dev), 16) != 0)
>  		return -ENODEV;
> 
>  	if (__le16_to_cpu(pfn_sb->version_minor) < 1) {
> --
> 2.40.0
> 

Re: [PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()

[Alison Schofield]

On Fri, Apr 14, 2023 at 06:50:59PM +0200, Markus Elfring wrote:
> >> The address of a data structure member was determined before
> >> a corresponding null pointer check in the implementation of
> >> the function “nd_pfn_validate”.
> >>
> >> Thus avoid the risk for undefined behaviour by replacing the usage of
> >> the local variable “parent_uuid” by a direct function call within
> >> a later condition check.
> >
> > Hi Markus,
> >
> > I think I understand what you are saying above, but I don't follow
> > how that applies here. This change seems to be a nice simplification,
> > parent_uuid, is used once, just grab it when needed.
> 
> Thanks for your positive feedback.

Hi Markus,

FYI - I'm a tiny bit taken aback that in response to me applying, and
providing feedback, on your patch, you respond with 2 links for me to
follow and cut off a chunk of my feedback.

Seems like it would taken the same amount of time to just answer my
two questions directly.

Was this part of a larger patch set? Andy's comment seems to indicate
that. Would have been nice to be CC'd on the cover letter.


More below...

> 
> 
> > What is the risk of undefined behavior?
> 
> See also:
> https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers?focusedCommentId=405504137#comment-405504137

Where is the NULL pointer dereference here?

> 
> 
> >> This issue was detected by using the Coccinelle software.
> > Which cocci script?
> 
> See also:
> Reconsidering pointer dereferences before null pointer checks (with SmPL)
> https://lore.kernel.org/cocci/1a11455f-ab57-dce0-1677-6beb8492a257@web.de/
> https://sympa.inria.fr/sympa/arc/cocci/2023-04/msg00021.html
> 

The cocci script linked above does not seem to apply here.

> 
> How do you think about to review and improve any similarly affected software components?
> 
> Regards,
> Markus
> 

Re: [PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()

[Andy Shevchenko]

On Fri, Apr 14, 2023 at 12:12:37PM +0200, Markus Elfring wrote:
> Date: Fri, 14 Apr 2023 12:01:15 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “nd_pfn_validate”.
> 
> Thus avoid the risk for undefined behaviour by replacing the usage of
> the local variable “parent_uuid” by a direct function call within
> a later condition check.

> This issue was detected by using the Coccinelle software.
> 
> Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add uuid helpers")

Same issues as per patch 1.

...

> -	if (memcmp(pfn_sb->parent_uuid, parent_uuid, 16) != 0)
> +	if (memcmp(pfn_sb->parent_uuid, nd_dev_to_uuid(&ndns->dev), 16) != 0)

If parent_uuid is of uuid_t type, you better to replace memcmp() with
uuid_equal().

>  		return -ENODEV;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] media: atomisp: Move a variable assignment behind a null pointer check in atomisp_cp_general_isp_parameters()

[Andy Shevchenko]

On Thu, Apr 13, 2023 at 10:20:15PM +0200, Markus Elfring wrote:
> Date: Thu, 13 Apr 2023 22:08:42 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “atomisp_cp_general_isp_parameters”.
> 
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “cur_config” behind the null pointer check.

I don't think this is what is happening here. The check might be removed by
optimizer in the compiler.

> This issue was detected by using the Coccinelle software.
> 
> Fixes: ad85094b293e40e7a2f831b0311a389d952ebd5e ("Revert 'media: staging: atomisp: Remove driver'")

Wrong tag format.

Code-wise I'm not against this, but it's up to Hans.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] media: atomisp: Move a variable assignment behind a null pointer check in atomisp_cp_general_isp_parameters()

[Hans de Goede]

Hi,

On 4/14/23 19:13, Andy Shevchenko wrote:
> On Thu, Apr 13, 2023 at 10:20:15PM +0200, Markus Elfring wrote:
>> Date: Thu, 13 Apr 2023 22:08:42 +0200
>>
>> The address of a data structure member was determined before
>> a corresponding null pointer check in the implementation of
>> the function “atomisp_cp_general_isp_parameters”.
>>
>> Thus avoid the risk for undefined behaviour by moving the assignment
>> for the variable “cur_config” behind the null pointer check.
> 
> I don't think this is what is happening here. The check might be removed by
> optimizer in the compiler.

Right, Markus thank you for the patch.

Instead of this patch, can you please audit the callers of this
function? I expect that you will likely find that the callers
already unconditionally deref css_param themselves, or they
guarantee a non NULL value in other ways (e.g. address of local
variable).

So I expect that the check can simply be dropped completely,
which would be a much better fix.

Regards,

Hans





> 
>> This issue was detected by using the Coccinelle software.
>>
>> Fixes: ad85094b293e40e7a2f831b0311a389d952ebd5e ("Revert 'media: staging: atomisp: Remove driver'")
> 
> Wrong tag format.
> 
> Code-wise I'm not against this, but it's up to Hans.
> 

Re: [PATCH] media: au0828: Move a variable assignment behind condition checks in au0828_isoc_copy()

[Hans Verkuil]

On 14/04/2023 21:10, Markus Elfring wrote:
> Date: Fri, 14 Apr 2023 21:00:45 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “au0828_isoc_copy”.
> 
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “vbi_dma_q” behind some condition checks.

Just drop these lines instead:

        if (!dev)
                return 0;

If you analyze the code then you'll see that dev is never NULL.

> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 7f8eacd2162a39ca7fc1240883118a786f147ccb ("V4L/DVB: Add closed captioning support for the HVR-950q")

This doesn't actually fix anything since nothing was wrong. Drop the
Fixes tag. Same for all the other patches.

Regards,

	Hans

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/usb/au0828/au0828-video.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c
> index fd9fc43d47e0..c0c5f2ed65e3 100644
> --- a/drivers/media/usb/au0828/au0828-video.c
> +++ b/drivers/media/usb/au0828/au0828-video.c
> @@ -491,7 +491,7 @@ static inline int au0828_isoc_copy(struct au0828_dev *dev, struct urb *urb)
>  	struct au0828_buffer    *buf;
>  	struct au0828_buffer    *vbi_buf;
>  	struct au0828_dmaqueue  *dma_q = urb->context;
> -	struct au0828_dmaqueue  *vbi_dma_q = &dev->vbiq;
> +	struct au0828_dmaqueue  *vbi_dma_q;
>  	unsigned char *outp = NULL;
>  	unsigned char *vbioutp = NULL;
>  	int i, len = 0, rc = 1;
> @@ -521,6 +521,8 @@ static inline int au0828_isoc_copy(struct au0828_dev *dev, struct urb *urb)
>  	if (vbi_buf != NULL)
>  		vbioutp = vb2_plane_vaddr(&vbi_buf->vb.vb2_buf, 0);
> 
> +	vbi_dma_q = &dev->vbiq;
> +
>  	for (i = 0; i < urb->number_of_packets; i++) {
>  		int status = urb->iso_frame_desc[i].status;
> 
> --
> 2.40.0
> 

Re: [PATCH] media: mediatek: vcodec: Move a variable assignment behind condition checks in vdec_vp9_slice_single_decode()

[AngeloGioacchino Del Regno]

Il 14/04/23 20:30, Markus Elfring ha scritto:
> Date: Fri, 14 Apr 2023 20:07:01 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “vdec_vp9_slice_single_decode”.
> 
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “pfc” behind some condition checks.
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: b0f407c19648ae9110c932c91d6e1b9381ec0aeb ("media: mediatek: vcodec: add vp9 decoder driver for mt8186")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH] media: mediatek: vcodec: Move a variable assignment behind condition checks in vdec_vp9_slice_single_decode()

[Hans Verkuil]

On 14/04/2023 20:30, Markus Elfring wrote:
> Date: Fri, 14 Apr 2023 20:07:01 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “vdec_vp9_slice_single_decode”.
> 
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “pfc” behind some condition checks.
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: b0f407c19648ae9110c932c91d6e1b9381ec0aeb ("media: mediatek: vcodec: add vp9 decoder driver for mt8186")

Not a fix, it was never broken.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  .../media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c  | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> index cf16cf2807f0..22b27f7b57bf 100644
> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> @@ -1990,7 +1990,7 @@ static int vdec_vp9_slice_single_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
>  					struct vdec_fb *fb, bool *res_chg)
>  {
>  	struct vdec_vp9_slice_instance *instance = h_vdec;

Just drop these lines instead:

        if (!instance || !instance->ctx)
                return -EINVAL;

That can never happen.

Regards,

	Hans

> -	struct vdec_vp9_slice_pfc *pfc = &instance->sc_pfc;
> +	struct vdec_vp9_slice_pfc *pfc;
>  	struct vdec_vp9_slice_vsi *vsi;
>  	struct mtk_vcodec_ctx *ctx;
>  	int ret;
> @@ -2007,6 +2007,7 @@ static int vdec_vp9_slice_single_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
>  	if (!fb)
>  		return -EBUSY;
> 
> +	pfc = &instance->sc_pfc;
>  	vsi = &pfc->vsi;
> 
>  	ret = vdec_vp9_slice_setup_single(instance, bs, fb, pfc);
> --
> 2.40.0
> 

Re: [PATCH 0/9] GPU-DRM-nouveau: Adjustments for seven function implementations

[Karol Herbst]

On Sun, Apr 16, 2023 at 11:30 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> Date: Sun, 16 Apr 2023 11:22:23 +0200
>
> Several update suggestions were taken into account
> from static source code analysis.
>

Reviewed-by: Karol Herbst <kherbst@redhat.com>

> Markus Elfring (9):
>   debugfs: Move an expression into a function call parameter
>     in nouveau_debugfs_pstate_set()
>   debugfs: Move a variable assignment behind a null pointer check
>     in nouveau_debugfs_pstate_get()
>   debugfs: Use seq_putc()
>     in nouveau_debugfs_pstate_get()
>   debugfs: Replace five seq_printf() calls by seq_puts()
>     in nouveau_debugfs_pstate_get()
>   power_budget: Move an expression into a macro call parameter
>     in nvbios_power_budget_header()
>   clk: Move a variable assignment behind a null pointer check
>     in nvkm_pstate_new()
>   pci: Move a variable assignment behind condition checks
>     in nvkm_pcie_set_link()
>   pci: Move an expression into a function call parameter
>     in nvkm_pcie_set_link()
>   therm: Move an assignment statement behind a null pointer check
>     in two functions
>
>  drivers/gpu/drm/nouveau/nouveau_debugfs.c     | 19 ++++++++++---------
>  .../nouveau/nvkm/subdev/bios/power_budget.c   |  3 +--
>  .../gpu/drm/nouveau/nvkm/subdev/clk/base.c    |  2 +-
>  .../gpu/drm/nouveau/nvkm/subdev/pci/pcie.c    |  7 +++----
>  .../drm/nouveau/nvkm/subdev/therm/fanpwm.c    |  2 +-
>  .../drm/nouveau/nvkm/subdev/therm/fantog.c    |  2 +-
>  6 files changed, 17 insertions(+), 18 deletions(-)
>
> --
> 2.40.0
>

Re: [PATCH] gfs2: Move a variable assignment behind a null pointer check in inode_go_dump()

[Andreas Gruenbacher]

Hi Markus,

On Thu, Apr 13, 2023 at 9:23 PM Markus Elfring <Markus.Elfring@web.de> wrote:
> Date: Thu, 13 Apr 2023 20:54:30 +0200
>
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “inode_go_dump”.
>
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “inode” behind the null pointer check.
>
> This issue was detected by using the Coccinelle software.
>
> Fixes: 27a2660f1ef944724956d92e8a312b6da0936fae ("gfs2: Dump nrpages for inodes and their glocks")

Okay, that's a worthwhile cleanup. It doesn't actually fix a bug, so
I'm not going to add a Fixes tag, though.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  fs/gfs2/glops.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> index b65950e76be5..6e33c8058059 100644
> --- a/fs/gfs2/glops.c
> +++ b/fs/gfs2/glops.c
> @@ -535,12 +535,13 @@ static void inode_go_dump(struct seq_file *seq, struct gfs2_glock *gl,
>                           const char *fs_id_buf)
>  {
>         struct gfs2_inode *ip = gl->gl_object;
> -       struct inode *inode = &ip->i_inode;
> +       struct inode *inode;
>         unsigned long nrpages;
>
>         if (ip == NULL)
>                 return;
>
> +       inode = &ip->i_inode;
>         xa_lock_irq(&inode->i_data.i_pages);
>         nrpages = inode->i_data.nrpages;
>         xa_unlock_irq(&inode->i_data.i_pages);
> --
> 2.40.0
>

Thanks,
Andreas

Re: [PATCH] ASoC: SOF: Intel: hda-stream: Move three variable assignments behind condition checks in hda_dsp_iccmax_stream_hw_params()

[Pierre-Louis Bossart]

On 4/19/23 13:54, Markus Elfring wrote:
> Date: Wed, 19 Apr 2023 20:42:19 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “hda_dsp_iccmax_stream_hw_params”.
> 
> Thus avoid the risk for undefined behaviour by moving the assignment
> for three local variables behind some condition checks.
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 7d88b9608142f95ccdd3dfb190da4a5faddb1cc7 ("ASoC: SOF: Intel: hdac_ext_stream: consistent prefixes for variables/members")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Yes indeed, for some reason this was fixed in
hda_dsp_stream_hw_params() but not in the
hda_dsp_iccmax_stream_hw_params() variant.

Could we however use the same code as in hda_dsp_stream_hw_params() for
consistency?

	if (!hext_stream) {
		dev_err(sdev->dev, "error: no stream available\n");
		return -ENODEV;
	}

	if (!dmab) {
		dev_err(sdev->dev, "error: no dma buffer allocated!\n");
		return -ENODEV;
	}

	hstream = &hext_stream->hstream;
	sd_offset = SOF_STREAM_SD_OFFSET(hstream);
	mask = BIT(hstream->index);

Thanks!

> ---
>  sound/soc/sof/intel/hda-stream.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
> index 79d818e6a0fa..9c44127014fc 100644
> --- a/sound/soc/sof/intel/hda-stream.c
> +++ b/sound/soc/sof/intel/hda-stream.c
> @@ -407,10 +407,10 @@ int hda_dsp_iccmax_stream_hw_params(struct snd_sof_dev *sdev, struct hdac_ext_st
>  				    struct snd_dma_buffer *dmab,
>  				    struct snd_pcm_hw_params *params)
>  {
> -	struct hdac_stream *hstream = &hext_stream->hstream;
> -	int sd_offset = SOF_STREAM_SD_OFFSET(hstream);
> +	struct hdac_stream *hstream;
> +	int sd_offset;
>  	int ret;
> -	u32 mask = 0x1 << hstream->index;
> +	u32 mask;
> 
>  	if (!hext_stream) {
>  		dev_err(sdev->dev, "error: no stream available\n");
> @@ -422,9 +422,12 @@ int hda_dsp_iccmax_stream_hw_params(struct snd_sof_dev *sdev, struct hdac_ext_st
>  		return -ENODEV;
>  	}
> 
> +	hstream = &hext_stream->hstream;
>  	if (hstream->posbuf)
>  		*hstream->posbuf = 0;
> 
> +	sd_offset = SOF_STREAM_SD_OFFSET(hstream);
> +
>  	/* reset BDL address */
>  	snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR,
>  			  sd_offset + SOF_HDA_ADSP_REG_SD_BDLPL,
> @@ -459,6 +462,8 @@ int hda_dsp_iccmax_stream_hw_params(struct snd_sof_dev *sdev, struct hdac_ext_st
>  				sd_offset + SOF_HDA_ADSP_REG_SD_LVI,
>  				0xffff, (hstream->frags - 1));
> 
> +	mask = 0x1 << hstream->index;
> +
>  	/* decouple host and link DMA, enable DSP features */
>  	snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPCTL,
>  				mask, mask);
> --
> 2.40.0
> 

Re: [PATCH 0/4] staging: rtl8712: Adjustments for process_link_qual()

[Philipp Hortmann]

On 4/20/23 12:54, Markus Elfring wrote:
> Date: Thu, 20 Apr 2023 12:05:12 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (4):
>    Delete null pointer checks
>    Delete two variables
>    Reduce scope for the variable “sqd”
>    Simplify the usage of an expression
> 
>   drivers/staging/rtl8712/rtl8712_recv.c | 24 +++++++++++-------------
>   1 file changed, 11 insertions(+), 13 deletions(-)
> 
> --
> 2.40.0
> 
> 

Hi, you get the following checkpatch warning. Is this wanted?

WARNING: From:/Signed-off-by: email address mismatch: 'From: Markus 
Elfring <Markus.Elfring@web.de>' != 'Signed-off-by: Markus Elfring 
<elfring@users.sourceforge.net>'


Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> AW-NU120

Re: [PATCH] drm/bridge: it6505: Move a variable assignment behind a null pointer check in receive_timing_debugfs_show()

[Robert Foss]

Hey Markus,

This patch seems to be a part of a series without being marked as
such, this causes issues when importing this patch with maintainer
tools like b4 which automatically pull in the entire series and not
just the specific patch. Either label the patch as being part of a
series ( [PATCH 1/XX] ), or submit it separately.

On Sun, Apr 16, 2023 at 5:47 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> Date: Sun, 16 Apr 2023 17:30:46 +0200
>
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “receive_timing_debugfs_show”.
>
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “vid” behind the null pointer check.
>
> This issue was detected by using the Coccinelle software.
>
> Fixes: b5c84a9edcd418cd055becad6a22439e7c5e3bf8 ("drm/bridge: add it6505 driver")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

The email in the Signed-off tag should match the email of the sender,
which it doesn't.

With the two above issues fixed, please add my r-b.
Reviewed-by: Robert Foss <rfoss@kernel.org>

> ---
>  drivers/gpu/drm/bridge/ite-it6505.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index abaf6e23775e..45f579c365e7 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -3207,7 +3207,7 @@ static ssize_t receive_timing_debugfs_show(struct file *file, char __user *buf,
>                                            size_t len, loff_t *ppos)
>  {
>         struct it6505 *it6505 = file->private_data;
> -       struct drm_display_mode *vid = &it6505->video_info;
> +       struct drm_display_mode *vid;
>         u8 read_buf[READ_BUFFER_SIZE];
>         u8 *str = read_buf, *end = read_buf + READ_BUFFER_SIZE;
>         ssize_t ret, count;
> @@ -3216,6 +3216,7 @@ static ssize_t receive_timing_debugfs_show(struct file *file, char __user *buf,
>                 return -ENODEV;
>
>         it6505_calc_video_info(it6505);
> +       vid = &it6505->video_info;
>         str += scnprintf(str, end - str, "---video timing---\n");
>         str += scnprintf(str, end - str, "PCLK:%d.%03dMHz\n",
>                          vid->clock / 1000, vid->clock % 1000);
> --
> 2.40.0
>

Re: drm/bridge: it6505: Move a variable assignment behind a null pointer check in receive_timing_debugfs_show()

[Robert Foss]

On Tue, Apr 25, 2023 at 4:16 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > This patch seems to be a part of a series without being marked as such,
>
> The mentioned patch affects only a single function implementation.
>
>
> > this causes issues when importing this patch with maintainer tools
> > like b4 which automatically pull in the entire series and not
> > just the specific patch.
>
> It is a pity that there are special technical difficulties.
>
>
> > Either label the patch as being part of a series ( [PATCH 1/XX] ),
>
> Further software modules were similarly affected.
>
> See also:
> Reconsidering pointer dereferences before null pointer checks (with SmPL)
> https://lore.kernel.org/cocci/1a11455f-ab57-dce0-1677-6beb8492a257@web.de/
> https://sympa.inria.fr/sympa/arc/cocci/2023-04/msg00021.html
>
>
> > or submit it separately.
>
> I thought that I did that (in principle).

You can have a look at LKML for the email message-id to see the whole
thread of patches.
https://lore.kernel.org/all/14636275-4d26-d639-5f6e-293fc6d1c4c6@web.de/#r

Or https://lore.kernel.org/all/$MSG_ID

Fix the email Sign-off email != Sender email issue, resubmit and I'll
be able to apply this.

Re: drm/bridge: it6505: Move a variable assignment behind a null pointer check in receive_timing_debugfs_show()

[Robert Foss]

On Thu, Apr 27, 2023 at 9:40 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > Fix the email Sign-off email != Sender email issue, resubmit and I'll
> > be able to apply this.
>
> You can pick the email from my tag “Signed-off-by” up also directly
> as an ordinary patch author email, can't you?

Of course, I can change the email to anything, but drm maintainer
scripts checks for this, presumably for a reason, so it should be
correctly submitted.

>
> Regards,
> Markus
>

Re: [PATCH resent] drm/bridge: it6505: Move a variable assignment behind a null pointer check in receive_timing_debugfs_show()

[Robert Foss]

On Fri, Apr 28, 2023 at 5:56 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 16 Apr 2023 17:30:46 +0200
>
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “receive_timing_debugfs_show”.
>
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “vid” behind the null pointer check.
>
> This issue was detected by using the Coccinelle software.
>
> Fixes: b5c84a9edcd418cd055becad6a22439e7c5e3bf8 ("drm/bridge: add it6505 driver")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/bridge/ite-it6505.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index abaf6e23775e..45f579c365e7 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -3207,7 +3207,7 @@ static ssize_t receive_timing_debugfs_show(struct file *file, char __user *buf,
>                                            size_t len, loff_t *ppos)
>  {
>         struct it6505 *it6505 = file->private_data;
> -       struct drm_display_mode *vid = &it6505->video_info;
> +       struct drm_display_mode *vid;
>         u8 read_buf[READ_BUFFER_SIZE];
>         u8 *str = read_buf, *end = read_buf + READ_BUFFER_SIZE;
>         ssize_t ret, count;
> @@ -3216,6 +3216,7 @@ static ssize_t receive_timing_debugfs_show(struct file *file, char __user *buf,
>                 return -ENODEV;
>
>         it6505_calc_video_info(it6505);
> +       vid = &it6505->video_info;
>         str += scnprintf(str, end - str, "---video timing---\n");
>         str += scnprintf(str, end - str, "PCLK:%d.%03dMHz\n",
>                          vid->clock / 1000, vid->clock % 1000);
> --
> 2.40.0
>

Applied to drm-misc-next.

Re: [PATCH 2/5] drm/amd/display: Move three variable assignments behind condition checks in trigger_hotplug()

[Christian König]

Am 11.04.23 um 15:43 schrieb Markus Elfring:
> Date: Tue, 11 Apr 2023 11:39:02 +0200
>
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “trigger_hotplug”.
>
> Thus avoid the risk for undefined behaviour by moving the assignment
> for three local variables behind some condition checks.

It might be that the NULL check doesn't make sense in the first place, 
but since I'm not an expert for this code I can't fully judge.

On the other hand the patches clearly look like nice cleanups to me, so 
feel free to add an Acked-by: Christian König <christian.koenig@amd.com> 
to the series.

Thanks,
Christian.

>
> This issue was detected by using the Coccinelle software.
>
> Fixes: 6f77b2ac628073f647041a92b36c824ae3aef16e ("drm/amd/display: Add connector HPD trigger debugfs entry")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c  | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> index 827fcb4fb3b3..b3cfd7dfbb28 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> @@ -1205,10 +1205,10 @@ static ssize_t trigger_hotplug(struct file *f, const char __user *buf,
>   							size_t size, loff_t *pos)
>   {
>   	struct amdgpu_dm_connector *aconnector = file_inode(f)->i_private;
> -	struct drm_connector *connector = &aconnector->base;
> +	struct drm_connector *connector;
>   	struct dc_link *link = NULL;
> -	struct drm_device *dev = connector->dev;
> -	struct amdgpu_device *adev = drm_to_adev(dev);
> +	struct drm_device *dev;
> +	struct amdgpu_device *adev;
>   	enum dc_connection_type new_connection_type = dc_connection_none;
>   	char *wr_buf = NULL;
>   	uint32_t wr_buf_size = 42;
> @@ -1253,12 +1253,16 @@ static ssize_t trigger_hotplug(struct file *f, const char __user *buf,
>   		return -EINVAL;
>   	}
>
> +	connector = &aconnector->base;
> +	dev = connector->dev;
> +
>   	if (param[0] == 1) {
>
>   		if (!dc_link_detect_connection_type(aconnector->dc_link, &new_connection_type) &&
>   			new_connection_type != dc_connection_none)
>   			goto unlock;
>
> +		adev = drm_to_adev(dev);
>   		mutex_lock(&adev->dm.dc_lock);
>   		ret = dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD);
>   		mutex_unlock(&adev->dm.dc_lock);
> --
> 2.40.0
>

Re: [PATCH 0/5] drm/amd: Adjustments for three function implementations

[Markus Elfring]

> Date: Tue, 11 Apr 2023 14:36:36 +0200
>
> Some update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (5)
>   amdgpu: Move a variable assignment behind a null pointer check in amdgpu_ras_interrupt_dispatch()
>   display: Move three variable assignments behind condition checks in trigger_hotplug()
>   display: Delete three unnecessary variable initialisations in trigger_hotplug()
>   display: Delete a redundant statement in trigger_hotplug()
>   display: Move an expression into a return statement in dcn201_link_encoder_create()
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c       |  3 ++-
>  .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 19 ++++++++++---------
>  .../amd/display/dc/dcn201/dcn201_resource.c   |  4 +---
>  3 files changed, 13 insertions(+), 13 deletions(-)

Is this patch series still in review queues?

See also:
https://lore.kernel.org/cocci/2258ce64-2a14-6778-8319-b342b06a1f33@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-04/msg00034.html

Regards,
Markus

Re: [PATCH 1/5] drm/amdgpu: Move a variable assignment behind a null pointer check in amdgpu_ras_interrupt_dispatch()

[Felix Kuehling]

Am 2023-04-11 um 09:42 schrieb Markus Elfring:
> Date: Tue, 11 Apr 2023 10:52:48 +0200
>
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “amdgpu_ras_interrupt_dispatch”.
>
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “data” behind the null pointer check.
>
> This issue was detected by using the Coccinelle software.
>
> Fixes: c030f2e4166c3f5597c7e7a70bcd9ab383695de4 ("drm/amdgpu: add amdgpu_ras.c to support ras (v2)")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index 4069bce9479f..a920c7888d07 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -1730,11 +1730,12 @@ int amdgpu_ras_interrupt_dispatch(struct amdgpu_device *adev,
>   		struct ras_dispatch_if *info)
>   {
>   	struct ras_manager *obj = amdgpu_ras_find_obj(adev, &info->head);
> -	struct ras_ih_data *data = &obj->ih_data;
> +	struct ras_ih_data *data;
I'm curious, this only takes the address of obj->ih_data. It doesn't 
dereference the pointer until after the !obj check below. How is this 
undefined behaviour? Is this about the compiler being free to reorder 
stuff for optimization, unaware of the dependency? Is there a link to an 
explanation that could be added to the commit description?

Thanks,
   Felix


>
>   	if (!obj)
>   		return -EINVAL;
>
> +	data = &obj->ih_data;
>   	if (data->inuse == 0)
>   		return 0;
>
> --
> 2.40.0
>

Re: [PATCH 1/5] drm/amdgpu: Move a variable assignment behind a null pointer check in amdgpu_ras_interrupt_dispatch()

[Markus Elfring]

> Date: Tue, 11 Apr 2023 10:52:48 +0200
>
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “amdgpu_ras_interrupt_dispatch”.
>
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “data” behind the null pointer check.
>
> This issue was detected by using the Coccinelle software.
>
> Fixes: c030f2e4166c3f5597c7e7a70bcd9ab383695de4 ("drm/amdgpu: add amdgpu_ras.c to support ras (v2)")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index 4069bce9479f..a920c7888d07 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -1730,11 +1730,12 @@ int amdgpu_ras_interrupt_dispatch(struct amdgpu_device *adev,
>  		struct ras_dispatch_if *info)
>  {
>  	struct ras_manager *obj = amdgpu_ras_find_obj(adev, &info->head);
> -	struct ras_ih_data *data = &obj->ih_data;
> +	struct ras_ih_data *data;
>
>  	if (!obj)
>  		return -EINVAL;
>
> +	data = &obj->ih_data;
>  	if (data->inuse == 0)
>  		return 0;
>

I would like to point out that another software adjustment got the desired
development attention (on 2024-05-11).

See also:
Commit 4c11d30c95576937c6c35e6f29884761f2dddb43 ("drm/amdgpu:
Fix the null pointer dereference to ras_manager")

Regards,
Markus

[PATCH RESEND] qed: Move a variable assignment behind a null pointer check in two functions

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 11 Apr 2023 19:33:53 +0200

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

Thus avoid the risk for undefined behaviour by moving the assignment
for the variables “p_rx” and “p_tx” behind the null pointer check.

This issue was detected by using the Coccinelle software.

Fixes: 0a7fb11c23c0 ("qed: Add Light L2 support")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/ethernet/qlogic/qed/qed_ll2.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_ll2.c b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
index 717a0b3f89bd..941c02fccaaf 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_ll2.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
@@ -346,7 +346,7 @@ static void qed_ll2_txq_flush(struct qed_hwfn *p_hwfn, u8 connection_handle)
 static int qed_ll2_txq_completion(struct qed_hwfn *p_hwfn, void *p_cookie)
 {
 	struct qed_ll2_info *p_ll2_conn = p_cookie;
-	struct qed_ll2_tx_queue *p_tx = &p_ll2_conn->tx_queue;
+	struct qed_ll2_tx_queue *p_tx;
 	u16 new_idx = 0, num_bds = 0, num_bds_in_packet = 0;
 	struct qed_ll2_tx_packet *p_pkt;
 	bool b_last_frag = false;
@@ -356,6 +356,7 @@ static int qed_ll2_txq_completion(struct qed_hwfn *p_hwfn, void *p_cookie)
 	if (!p_ll2_conn)
 		return rc;

+	p_tx = &p_ll2_conn->tx_queue;
 	spin_lock_irqsave(&p_tx->lock, flags);
 	if (p_tx->b_completing_packet) {
 		rc = -EBUSY;
@@ -523,7 +524,7 @@ qed_ll2_rxq_handle_completion(struct qed_hwfn *p_hwfn,
 static int qed_ll2_rxq_completion(struct qed_hwfn *p_hwfn, void *cookie)
 {
 	struct qed_ll2_info *p_ll2_conn = (struct qed_ll2_info *)cookie;
-	struct qed_ll2_rx_queue *p_rx = &p_ll2_conn->rx_queue;
+	struct qed_ll2_rx_queue *p_rx;
 	union core_rx_cqe_union *cqe = NULL;
 	u16 cq_new_idx = 0, cq_old_idx = 0;
 	unsigned long flags = 0;
@@ -532,6 +533,7 @@ static int qed_ll2_rxq_completion(struct qed_hwfn *p_hwfn, void *cookie)
 	if (!p_ll2_conn)
 		return rc;

+	p_rx = &p_ll2_conn->rx_queue;
 	spin_lock_irqsave(&p_rx->lock, flags);

 	if (!QED_LL2_RX_REGISTERED(p_ll2_conn)) {
--
2.40.0

Re: [PATCH RESEND] qed: Move a variable assignment behind a null pointer check in two functions

[Michal Swiatkowski]

On Sun, Mar 02, 2025 at 05:55:40PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 11 Apr 2023 19:33:53 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the functions “qed_ll2_rxq_completion” and “qed_ll2_txq_completion”.
> 
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variables “p_rx” and “p_tx” behind the null pointer check.
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 0a7fb11c23c0 ("qed: Add Light L2 support")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/ethernet/qlogic/qed/qed_ll2.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_ll2.c b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
> index 717a0b3f89bd..941c02fccaaf 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_ll2.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
> @@ -346,7 +346,7 @@ static void qed_ll2_txq_flush(struct qed_hwfn *p_hwfn, u8 connection_handle)
>  static int qed_ll2_txq_completion(struct qed_hwfn *p_hwfn, void *p_cookie)
>  {
>  	struct qed_ll2_info *p_ll2_conn = p_cookie;
> -	struct qed_ll2_tx_queue *p_tx = &p_ll2_conn->tx_queue;
> +	struct qed_ll2_tx_queue *p_tx;
>  	u16 new_idx = 0, num_bds = 0, num_bds_in_packet = 0;
>  	struct qed_ll2_tx_packet *p_pkt;
>  	bool b_last_frag = false;
> @@ -356,6 +356,7 @@ static int qed_ll2_txq_completion(struct qed_hwfn *p_hwfn, void *p_cookie)
>  	if (!p_ll2_conn)
>  		return rc;
> 
> +	p_tx = &p_ll2_conn->tx_queue;
>  	spin_lock_irqsave(&p_tx->lock, flags);
>  	if (p_tx->b_completing_packet) {
>  		rc = -EBUSY;
> @@ -523,7 +524,7 @@ qed_ll2_rxq_handle_completion(struct qed_hwfn *p_hwfn,
>  static int qed_ll2_rxq_completion(struct qed_hwfn *p_hwfn, void *cookie)
>  {
>  	struct qed_ll2_info *p_ll2_conn = (struct qed_ll2_info *)cookie;
> -	struct qed_ll2_rx_queue *p_rx = &p_ll2_conn->rx_queue;
> +	struct qed_ll2_rx_queue *p_rx;
>  	union core_rx_cqe_union *cqe = NULL;
>  	u16 cq_new_idx = 0, cq_old_idx = 0;
>  	unsigned long flags = 0;
> @@ -532,6 +533,7 @@ static int qed_ll2_rxq_completion(struct qed_hwfn *p_hwfn, void *cookie)
>  	if (!p_ll2_conn)
>  		return rc;
> 
> +	p_rx = &p_ll2_conn->rx_queue;
>  	spin_lock_irqsave(&p_rx->lock, flags);
> 
>  	if (!QED_LL2_RX_REGISTERED(p_ll2_conn)) {

For future submission plase specify the target kernel
[PATCH net] for fixes, [PATCH net-next] for other.

Looking at the code callback is always set together with cookie (which
is pointing to p_ll2_conn. I am not sure if this is fixing real issue,
but maybe there are a cases when callback is still connected and cookie
is NULL.

Anyway, with heaving this check for p_ll2_conn it is good to move
assigment after this check.

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

> --
> 2.40.0

Re: [PATCH RESEND] qed: Move a variable assignment behind a null pointer check in two functions

[Dan Carpenter]

On Mon, Mar 03, 2025 at 07:21:28AM +0100, Michal Swiatkowski wrote:
> > @@ -523,7 +524,7 @@ qed_ll2_rxq_handle_completion(struct qed_hwfn *p_hwfn,
> >  static int qed_ll2_rxq_completion(struct qed_hwfn *p_hwfn, void *cookie)
> >  {
> >  	struct qed_ll2_info *p_ll2_conn = (struct qed_ll2_info *)cookie;
> > -	struct qed_ll2_rx_queue *p_rx = &p_ll2_conn->rx_queue;
> > +	struct qed_ll2_rx_queue *p_rx;
> >  	union core_rx_cqe_union *cqe = NULL;
> >  	u16 cq_new_idx = 0, cq_old_idx = 0;
> >  	unsigned long flags = 0;
> > @@ -532,6 +533,7 @@ static int qed_ll2_rxq_completion(struct qed_hwfn *p_hwfn, void *cookie)
> >  	if (!p_ll2_conn)
> >  		return rc;
> > 
> > +	p_rx = &p_ll2_conn->rx_queue;
> >  	spin_lock_irqsave(&p_rx->lock, flags);
> > 
> >  	if (!QED_LL2_RX_REGISTERED(p_ll2_conn)) {
> 
> For future submission plase specify the target kernel
> [PATCH net] for fixes, [PATCH net-next] for other.
> 
> Looking at the code callback is always set together with cookie (which
> is pointing to p_ll2_conn. I am not sure if this is fixing real issue,
> but maybe there are a cases when callback is still connected and cookie
> is NULL.

The assignment:

	p_rx = &p_ll2_conn->rx_queue;

does not dereference "p_ll2_conn".  It just does pointer math.  So the
original code works fine.

The question is, did the original author write it that way intentionally?
I've had this conversation with people before and they eventually are
convinced that "Okay, the code works, but only by sheer chance."  In my
experiences, most of the time, the authors of this type of code are so
used to weirdnesses of C that they write code like this without even
thinking about it.  It never even occurs to them how it could be
confusing.

This commit message is misleading and there should not be a Fixes tag.

regards,
dan carpenter

Re: [RESEND] qed: Move a variable assignment behind a null pointer check in two functions

[Markus Elfring]

> The assignment:
>
> 	p_rx = &p_ll2_conn->rx_queue;
>
> does not dereference "p_ll2_conn".  It just does pointer math.  So the
> original code works fine.

Is there a need to clarify affected implementation details any more?
https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers

Regards,
Markus

Re: [RESEND] qed: Move a variable assignment behind a null pointer check in two functions

[Dan Carpenter]

On Mon, Mar 03, 2025 at 09:22:58AM +0100, Markus Elfring wrote:
> > The assignment:
> >
> > 	p_rx = &p_ll2_conn->rx_queue;
> >
> > does not dereference "p_ll2_conn".  It just does pointer math.  So the
> > original code works fine.
> 
> Is there a need to clarify affected implementation details any more?
> https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers

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

regards,
dan carpenter

Re: qed: Move a variable assignment behind a null pointer check in two functions

[Markus Elfring]

> This is not a NULL dereference.  It's just pointer math.
How does your view fit to information in an article like “Fun with NULL pointers, part 1”
(by Jonathan Corbet from 2009-07-20)?
https://lwn.net/Articles/342330/

Regards,
Markus

Re: [RESEND] qed: Move a variable assignment behind a null pointer check in two functions

[Kory Maincent]

On Mon, 3 Mar 2025 11:28:29 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Mon, Mar 03, 2025 at 09:22:58AM +0100, Markus Elfring wrote:
> > > The assignment:
> > >
> > > 	p_rx = &p_ll2_conn->rx_queue;
> > >
> > > does not dereference "p_ll2_conn".  It just does pointer math.  So the
> > > original code works fine.  
> > 
> > Is there a need to clarify affected implementation details any more?
> > https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers
> >  
> 
> This is not a NULL dereference.  It's just pointer math.
> 
> regards,
> dan carpenter

Feel free to ignore Markus, he has a long history of sending unhelpful reviews,
comments or patches and continues to ignore repeated requests to stop. He is
already on the ignore lists of several maintainers.
There is a lot of chance that he is a bot.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: qed: Move a variable assignment behind a null pointer check in two functions

[Markus Elfring]

> There is a lot of chance that he is a bot.
I hope that corresponding software development discussions can become
more constructive again.

Regards,
Markus

[PATCH RESEND] video: au1100fb: Move a variable assignment behind a null pointer check in au1100fb_setmode()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Apr 2023 21:35:36 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “au1100fb_setmode”.

Thus avoid the risk for undefined behaviour by moving the assignment
for the variable “info” behind the null pointer check.

This issue was detected by using the Coccinelle software.

Fixes: 3b495f2bb749 ("Au1100 FB driver uplift for 2.6.")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/video/fbdev/au1100fb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/au1100fb.c b/drivers/video/fbdev/au1100fb.c
index cb317398e71a..fcb47b350bc9 100644
--- a/drivers/video/fbdev/au1100fb.c
+++ b/drivers/video/fbdev/au1100fb.c
@@ -137,13 +137,15 @@ static int au1100fb_fb_blank(int blank_mode, struct fb_info *fbi)
 	 */
 int au1100fb_setmode(struct au1100fb_device *fbdev)
 {
-	struct fb_info *info = &fbdev->info;
+	struct fb_info *info;
 	u32 words;
 	int index;

 	if (!fbdev)
 		return -EINVAL;

+	info = &fbdev->info;
+
 	/* Update var-dependent FB info */
 	if (panel_is_active(fbdev->panel) || panel_is_color(fbdev->panel)) {
 		if (info->var.bits_per_pixel <= 8) {
--
2.40.0

Re: [PATCH RESEND] video: au1100fb: Move a variable assignment behind a null pointer check in au1100fb_setmode()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

Hello,

On Sun, Mar 02, 2025 at 07:02:12PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 13 Apr 2023 21:35:36 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “au1100fb_setmode”.
> 
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “info” behind the null pointer check.
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 3b495f2bb749 ("Au1100 FB driver uplift for 2.6.")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Should also get

Cc: stable@vger.kernel.org

to ensure this is backported to stable.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RESEND] video: au1100fb: Move a variable assignment behind a null pointer check in au1100fb_setmode()

[Dan Carpenter]

On Mon, Mar 03, 2025 at 10:19:06AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Sun, Mar 02, 2025 at 07:02:12PM +0100, Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Thu, 13 Apr 2023 21:35:36 +0200
> > 
> > The address of a data structure member was determined before
> > a corresponding null pointer check in the implementation of
> > the function “au1100fb_setmode”.
> > 
> > Thus avoid the risk for undefined behaviour by moving the assignment
> > for the variable “info” behind the null pointer check.
> > 
> > This issue was detected by using the Coccinelle software.
> > 
> > Fixes: 3b495f2bb749 ("Au1100 FB driver uplift for 2.6.")
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> 
> Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> 
> Should also get
> 
> Cc: stable@vger.kernel.org
> 
> to ensure this is backported to stable.

It's not a bugfix, it's a cleanup.  That's not a dereference, it's
just pointer math.  It shouldn't have a Fixes tag.

Real bugs where we dereference a pointer and then check for NULL don't
last long in the kernel.  Most of the stuff Markus is sending is false
positives like this.

regards,
dan carpenter

Re: [PATCH RESEND] video: au1100fb: Move a variable assignment behind a null pointer check in au1100fb_setmode()

[Dan Carpenter]

On Mon, Mar 03, 2025 at 10:19:06AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Sun, Mar 02, 2025 at 07:02:12PM +0100, Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Thu, 13 Apr 2023 21:35:36 +0200
> > 
> > The address of a data structure member was determined before
> > a corresponding null pointer check in the implementation of
> > the function “au1100fb_setmode”.
> > 
> > Thus avoid the risk for undefined behaviour by moving the assignment
> > for the variable “info” behind the null pointer check

Re: [PATCH RESEND] video: au1100fb: Move a variable assignment behind a null pointer check in au1100fb_setmode()

[Dan Carpenter]

On Mon, Mar 03, 2025 at 01:08:29PM +0300, Dan Carpenter wrote:
> Real bugs where we dereference a pointer and then check for NULL don't
> last long in the kernel.  Most of the stuff Markus is sending is false
> positives like this.

Maybe I was too optimistic.  Here are the Smatch warnings from the
Friday's linux-next.

Common false positives are that the pointer can sometimes be NULL but
there are other ways to determine without checking explicitly.  For
example, maybe the caller passes a flag which means it's non-NULL.

regards,
dan carpenter

arch/arm64/kvm/../../../virt/kvm/kvm_main.c:1639 kvm_prepare_memory_region() warn: variable dereferenced before check 'new' (see line 1622)
arch/x86/kernel/fpu/xstate.c:1567 fpstate_realloc() warn: variable dereferenced before check 'curfps' (see line 1546)
arch/x86/kvm/../../../virt/kvm/kvm_main.c:1639 kvm_prepare_memory_region() warn: variable dereferenced before check 'new' (see line 1622)
drivers/base/dd.c:696 really_probe() warn: variable dereferenced before check 'dev->bus' (see line 640)
drivers/base/dd.c:720 really_probe() warn: variable dereferenced before check 'dev->bus' (see line 640)
drivers/block/drbd/drbd_worker.c:588 make_resync_request() warn: variable dereferenced before check 'peer_device' (see line 587)
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:12341 parse_edid_displayid_vrr() warn: variable dereferenced before check 'edid_ext' (see line 12337)
drivers/gpu/drm/amd/amdgpu/../display/dc/clk_mgr/dcn21/rn_clk_mgr.c:775 rn_clk_mgr_construct() warn: variable dereferenced before check 'ctx->dc_bios->integrated_info' (see line 743)
drivers/gpu/drm/amd/amdgpu/../display/dc/clk_mgr/dcn301/vg_clk_mgr.c:734 vg_clk_mgr_construct() warn: variable dereferenced before check 'ctx->dc_bios->integrated_info' (see line 720)
drivers/gpu/drm/amd/amdgpu/../display/dc/clk_mgr/dcn314/dcn314_clk_mgr.c:899 dcn314_clk_mgr_construct() warn: variable dereferenced before check 'ctx->dc_bios->integrated_info' (see line 838)
drivers/gpu/drm/amd/amdgpu/../display/dc/clk_mgr/dcn315/dcn315_clk_mgr.c:715 dcn315_clk_mgr_construct() warn: variable dereferenced before check 'ctx->dc_bios->integrated_info' (see line 654)
drivers/gpu/drm/amd/amdgpu/../display/dc/clk_mgr/dcn316/dcn316_clk_mgr.c:655 dcn316_clk_mgr_construct() warn: variable dereferenced before check 'ctx->dc_bios->integrated_info' (see line 634)
drivers/gpu/drm/amd/amdgpu/../display/dc/clk_mgr/dcn31/dcn31_clk_mgr.c:789 dcn31_clk_mgr_construct() warn: variable dereferenced before check 'ctx->dc_bios->integrated_info' (see line 728)
drivers/gpu/drm/amd/amdgpu/../display/dc/clk_mgr/dcn35/dcn35_clk_mgr.c:1380 dcn35_clk_mgr_construct() warn: variable dereferenced before check 'ctx->dc_bios->integrated_info' (see line 1307)
drivers/gpu/drm/i915/gem/i915_gem_shmem.c:174 shmem_sg_alloc_table() warn: variable dereferenced before check 'sg' (see line 163)
drivers/gpu/drm/i915/gt/gen6_ppgtt.c:274 gen6_ppgtt_cleanup() warn: variable dereferenced before check 'ppgtt->base.pd' (see line 271)
drivers/gpu/drm/i915/gt/intel_execlists_submission.c:3649 rcu_virtual_context_destroy() warn: variable dereferenced before check 've->base.sched_engine' (see line 3623)
drivers/gpu/drm/nouveau/nouveau_dp.c:494 nouveau_dp_irq() warn: variable dereferenced before check 'outp' (see line 489)
drivers/hid/hid-debug.c:3727 hid_debug_events_read() warn: variable dereferenced before check 'list->hdev' (see line 3713)
drivers/net/ethernet/amd/pcnet32.c:1923 pcnet32_probe1() warn: variable dereferenced before check 'pdev' (see line 1843)
drivers/net/ethernet/apm/xgene/xgene_enet_main.c:267 xgene_enet_tx_completion() warn: variable dereferenced before check 'skb' (see line 243)
drivers/net/ethernet/natsemi/ns83820.c:884 rx_irq() warn: variable dereferenced before check 'skb' (see line 883)
drivers/net/ethernet/pensando/ionic/ionic_txrx.c:205 ionic_rx_build_skb() warn: variable dereferenced before check 'buf_info->page' (see line 188)
drivers/net/wireless/intel/iwlwifi/dvm/mac80211.c:1236 iwl_setup_interface() warn: variable dereferenced before check 'priv->lib->bt_params' (see line 1229)
drivers/net/wireless/intel/iwlwifi/dvm/main.c:1114 iwl_init_drv() warn: variable dereferenced before check 'priv->lib->bt_params' (see line 1109)
drivers/net/wireless/mediatek/mt76/mt76_connac_mac.c:307 mt76_connac2_mac_tx_rate_val() warn: variable dereferenced before check 'conf' (see line 300)
drivers/net/wireless/mediatek/mt76/mt7925/mac.c:851 mt7925_tx_check_aggr() warn: variable dereferenced before check 'sta' (see line 847)
drivers/nvme/host/ioctl.c:173 nvme_map_user_request() warn: variable dereferenced before check 'bio' (see line 162)
drivers/platform/mellanox/mlxreg-lc.c:903 mlxreg_lc_probe() warn: variable dereferenced before check 'data->notifier' (see line 828)
drivers/scsi/aacraid/commsup.c:2344 aac_command_thread() warn: variable dereferenced before check 'dev->queues' (see line 2332)
drivers/scsi/aic7xxx/aic79xx_osm.c:1837 ahd_done() warn: variable dereferenced before check 'cmd' (see line 1793)
drivers/scsi/csiostor/csio_mb.c:932 csio_fcoe_vnp_alloc_init_mb() warn: variable dereferenced before check 'vnport_wwnn' (see line 929)
drivers/scsi/ips.c:2560 ips_next() warn: variable dereferenced before check 'scb->scsi_cmd' (see line 2554)
drivers/scsi/ips.c:2568 ips_next() warn: variable dereferenced before check 'scb->scsi_cmd' (see line 2554)
drivers/scsi/ips.c:2593 ips_next() warn: variable dereferenced before check 'scb->scsi_cmd' (see line 2554)
drivers/scsi/libsas/sas_scsi_host.c:119 sas_scsi_task_done() warn: variable dereferenced before check 'sc' (see line 110)
drivers/scsi/lpfc/lpfc_bsg.c:1332 lpfc_bsg_hba_get_event() warn: variable dereferenced before check 'evt_dat' (see line 1299)
drivers/slimbus/qcom-ngd-ctrl.c:884 qcom_slim_ngd_xfer_msg() warn: variable dereferenced before check 'txn->msg' (see line 808)
drivers/spi/spi-offload.c:186 spi_offload_trigger_get() warn: variable dereferenced before check 'trigger->ops' (see line 176)
drivers/staging/media/atomisp/pci/isp/kernels/output/output_1.0/ia_css_output.host.c:58 ia_css_output_config() warn: variable dereferenced before check 'from->info' (see line 53)
drivers/usb/gadget/udc/tegra-xudc.c:2681 tegra_xudc_handle_transfer_completion() warn: variable dereferenced before check 'ep->desc' (see line 2679)
drivers/usb/musb/musb_core.c:964 musb_handle_intr_disconnect() warn: variable dereferenced before check 'musb->hcd' (see line 963)
fs/bcachefs/journal.c:1187 __bch2_set_nr_journal_buckets() warn: variable dereferenced before check 'c' (see line 1184)
fs/efs/inode.c:299 efs_map_block() warn: variable dereferenced before check 'bh' (see line 292)
fs/efs/inode.c:304 efs_map_block() warn: variable dereferenced before check 'bh' (see line 292)
fs/efs/inode.c:309 efs_map_block() warn: variable dereferenced before check 'bh' (see line 292)
fs/nfs/write.c:808 nfs_inode_remove_request() warn: variable dereferenced before check 'folio' (see line 805)
fs/ocfs2/dlm/dlmrecovery.c:1590 dlm_mig_lockres_worker() warn: variable dereferenced before check 'res' (see line 1563)
fs/ocfs2/move_extents.c:322 ocfs2_defrag_extent() warn: variable dereferenced before check 'context->data_ac' (see line 279)
fs/ocfs2/namei.c:1455 ocfs2_rename() warn: variable dereferenced before check 'newfe_bh' (see line 1452)
fs/ocfs2/namei.c:1637 ocfs2_rename() warn: variable dereferenced before check 'old_dir_bh' (see line 1618)
fs/smb/client/file.c:3085 cifs_oplock_break() warn: variable dereferenced before check 'inode' (see line 3056)
lib/reed_solomon/decode_rs.c:315 decode_rs16() warn: variable dereferenced before check 'par' (see line 81)
net/core/failover.c:85 failover_slave_register() warn: variable dereferenced before check 'fops' (see line 66)
net/mpls/mpls_iptunnel.c:156 mpls_xmit() warn: variable dereferenced before check 'out_dev' (see line 56)

Re: [PATCH RESEND] video: au1100fb: Move a variable assignment behind a null pointer check in au1100fb_setmode()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1855 bytes --]

On Mon, Mar 03, 2025 at 01:08:29PM +0300, Dan Carpenter wrote:
> On Mon, Mar 03, 2025 at 10:19:06AM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Sun, Mar 02, 2025 at 07:02:12PM +0100, Markus Elfring wrote:
> > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > Date: Thu, 13 Apr 2023 21:35:36 +0200
> > > 
> > > The address of a data structure member was determined before
> > > a corresponding null pointer check in the implementation of
> > > the function “au1100fb_setmode”.
> > > 
> > > Thus avoid the risk for undefined behaviour by moving the assignment
> > > for the variable “info” behind the null pointer check.
> > > 
> > > This issue was detected by using the Coccinelle software.
> > > 
> > > Fixes: 3b495f2bb749 ("Au1100 FB driver uplift for 2.6.")
> > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > 
> > Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > 
> > Should also get
> > 
> > Cc: stable@vger.kernel.org
> > 
> > to ensure this is backported to stable.
> 
> It's not a bugfix, it's a cleanup.  That's not a dereference, it's
> just pointer math.  It shouldn't have a Fixes tag.
> 
> Real bugs where we dereference a pointer and then check for NULL don't
> last long in the kernel.  Most of the stuff Markus is sending is false
> positives like this.

I thought a compiler translating the code

	struct fb_info *info = &fbdev->info;

	if (!fbdev)
		return -EINVAL;

is free (and expected) to just drop the if block. I wasn't aware that
this only applies when the pointer is actually dereferenced. Testing
that with arm-linux-gnueabihf-gcc 14.2.0 seems to confirm what you're
saying.

Thanks for letting me know. With that learned I agree that the Fixes tag
should be dropped (and Cc: stable not added).

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: video: au1100fb: Move a variable assignment behind a null pointer check in au1100fb_setmode()

[Markus Elfring]

> 	struct fb_info *info = &fbdev->info;
>
> 	if (!fbdev)
> 		return -EINVAL;

Is such a null pointer check still relevant for the discussed function implementation?

Regards,
Markus

Re: [PATCH RESEND] video: au1100fb: Move a variable assignment behind a null pointer check in au1100fb_setmode()

[Dan Carpenter]

On Mon, Mar 03, 2025 at 11:30:46AM +0100, Uwe Kleine-König wrote:
> On Mon, Mar 03, 2025 at 01:08:29PM +0300, Dan Carpenter wrote:
> > On Mon, Mar 03, 2025 at 10:19:06AM +0100, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > On Sun, Mar 02, 2025 at 07:02:12PM +0100, Markus Elfring wrote:
> > > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > > Date: Thu, 13 Apr 2023 21:35:36 +0200
> > > > 
> > > > The address of a data structure member was determined before
> > > > a corresponding null pointer check in the implementation of
> > > > the function “au1100fb_setmode”.
> > > > 
> > > > Thus avoid the risk for undefined behaviour by moving the assignment
> > > > for the variable “info” behind the null pointer check.
> > > > 
> > > > This issue was detected by using the Coccinelle software.
> > > > 
> > > > Fixes: 3b495f2bb749 ("Au1100 FB driver uplift for 2.6.")
> > > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > > 
> > > Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > > 
> > > Should also get
> > > 
> > > Cc: stable@vger.kernel.org
> > > 
> > > to ensure this is backported to stable.
> > 
> > It's not a bugfix, it's a cleanup.  That's not a dereference, it's
> > just pointer math.  It shouldn't have a Fixes tag.
> > 
> > Real bugs where we dereference a pointer and then check for NULL don't
> > last long in the kernel.  Most of the stuff Markus is sending is false
> > positives like this.
> 
> I thought a compiler translating the code
> 
> 	struct fb_info *info = &fbdev->info;
> 
> 	if (!fbdev)
> 		return -EINVAL;
> 
> is free (and expected) to just drop the if block.

If you dereference a pointer then it doesn't make sense to have a NULL
check after that because the kernel would already have crashed.

In 2009, we had the famous tun.c bug where there was a dereference
followed by a NULL check and the compiler deleted it as you say.
And then, it turned out that you could remap the NULL pointer to and so
the NULL dereference didn't lead to a crash and the missing NULL meant
the kernel kept running happily.  The remapped memory was full of
function pointers to get root.

We changed min_mmap_addr so that we can't remap the NULL pointer and
we started using the -fno-delete-null-pointer-checks compiler option so
that it wouldn't remove the NULL check even in places where it didn't
make sense.  We also started doing more static analysis.  We've also
tried to randomize where functions are in the memory so it's trickier
to hardcode function pointers.

A couple years later we had another bug where it turned out you could
still remap NULL.  I forget how the details.  No one wrote an exploit
and it wasn't publicized as much.

Anyway, none of that applies here, because this is just pointer math.

regards,
dan carpenter

Re: video: au1100fb: Move a variable assignment behind a null pointer check in au1100fb_setmode()

[Markus Elfring]

> Anyway, none of that applies here, because this is just pointer math.
Which data processing do you expect to be generally supported at the discussed
source code place (according to the rules of the programming language “C”)?
https://en.cppreference.com/w/c/language/behavior

Regards,
Markus

Re: video: au1100fb: Move a variable assignment behind a null pointer check in au1100fb_setmode()

[Helge Deller]

On 3/5/25 13:14, Markus Elfring wrote:
>> Anyway, none of that applies here, because this is just pointer math.
> Which data processing do you expect to be generally supported at the discussed
> source code place (according to the rules of the programming language “C”)?
> https://en.cppreference.com/w/c/language/behavior

There is nothing to discuss.
Dan is correct.

We have:
struct au1100fb_device {
         struct fb_info info;
...

so:

struct fb_info *info = &fbdev->info;
gets translated by the compiler to a trivial pointer math:
info = <value of fbdev> + 0     # 0 is there offset of "info" in the struct.

No crash or anything can or will happen here.

Markus, maybe you missed the "&" in front of "&fbdev->info" ?

Helge

Re: video: au1100fb: Move a variable assignment behind a null pointer check in au1100fb_setmode()

[Markus Elfring]

> No crash or anything can or will happen here.
>
> Markus, maybe you missed the "&" in front of "&fbdev->info" ?
Would you get into the mood to adjust development views
if you would take any feedback and further background information
(by David Svoboda for example) better into account?
https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers?focusedCommentId=405504139#comment-405504139

Regards,
Markus

Re: [PATCH RESEND] video: au1100fb: Move a variable assignment behind a null pointer check in au1100fb_setmode()

[Helge Deller]

On 3/3/25 10:19, Uwe Kleine-König wrote:
> Hello,
>
> On Sun, Mar 02, 2025 at 07:02:12PM +0100, Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Thu, 13 Apr 2023 21:35:36 +0200
>>
>> The address of a data structure member was determined before
>> a corresponding null pointer check in the implementation of
>> the function “au1100fb_setmode”.
>>
>> Thus avoid the risk for undefined behaviour by moving the assignment
>> for the variable “info” behind the null pointer check.
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Fixes: 3b495f2bb749 ("Au1100 FB driver uplift for 2.6.")
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>
> Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

applied to fbdev git tree (with minor modifications to commit
message and without stable tags).

Helge

Re: [PATCH] usb: dwc2: gadget: Move a variable assignment behind condition checks in dwc2_hsotg_handle_outdone()

[Minas Harutyunyan]

On 4/19/23 22:12, Markus Elfring wrote:
> Date: Wed, 19 Apr 2023 20:06:25 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “dwc2_hsotg_handle_outdone”.
> 
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “req” behind some condition checks.
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 5b7d70c6dbf2db786395cbd21750a1a4ce222f84 ("USB: Gadget driver for Samsung HS/OtG block")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Acked-by: Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>

> ---
>   drivers/usb/dwc2/gadget.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 8b15742d9e8a..cab04816dd6c 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2389,7 +2389,7 @@ static void dwc2_hsotg_handle_outdone(struct dwc2_hsotg *hsotg, int epnum)
>   	u32 epsize = dwc2_readl(hsotg, DOEPTSIZ(epnum));
>   	struct dwc2_hsotg_ep *hs_ep = hsotg->eps_out[epnum];
>   	struct dwc2_hsotg_req *hs_req = hs_ep->req;
> -	struct usb_request *req = &hs_req->req;
> +	struct usb_request *req;
>   	unsigned int size_left = DXEPTSIZ_XFERSIZE_GET(epsize);
>   	int result = 0;
> 
> @@ -2408,6 +2408,8 @@ static void dwc2_hsotg_handle_outdone(struct dwc2_hsotg *hsotg, int epnum)
>   	if (using_desc_dma(hsotg))
>   		size_left = dwc2_gadget_get_xfersize_ddma(hs_ep);
> 
> +	req = &hs_req->req;
> +
>   	if (using_dma(hsotg)) {
>   		unsigned int size_done;
> 
> --
> 2.40.0
> 

[PATCH RESEND] usb: dwc2: gadget: Move a variable assignment behind condition checks in dwc2_hsotg_handle_outdone()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 19 Apr 2023 20:06:25 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “dwc2_hsotg_handle_outdone”.

Thus avoid the risk for undefined behaviour by moving the assignment
for the variable “req” behind some condition checks.

This issue was detected by using the Coccinelle software.

Fixes: 5b7d70c6dbf2 ("USB: Gadget driver for Samsung HS/OtG block")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/usb/dwc2/gadget.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 8b15742d9e8a..cab04816dd6c 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2389,7 +2389,7 @@ static void dwc2_hsotg_handle_outdone(struct dwc2_hsotg *hsotg, int epnum)
 	u32 epsize = dwc2_readl(hsotg, DOEPTSIZ(epnum));
 	struct dwc2_hsotg_ep *hs_ep = hsotg->eps_out[epnum];
 	struct dwc2_hsotg_req *hs_req = hs_ep->req;
-	struct usb_request *req = &hs_req->req;
+	struct usb_request *req;
 	unsigned int size_left = DXEPTSIZ_XFERSIZE_GET(epsize);
 	int result = 0;

@@ -2408,6 +2408,8 @@ static void dwc2_hsotg_handle_outdone(struct dwc2_hsotg *hsotg, int epnum)
 	if (using_desc_dma(hsotg))
 		size_left = dwc2_gadget_get_xfersize_ddma(hs_ep);

+	req = &hs_req->req;
+
 	if (using_dma(hsotg)) {
 		unsigned int size_done;

--
2.40.0

[PATCH RESEND] tipc: Reduce scope for the variable “fdefq” in tipc_link_tnl_prepare()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 13 Apr 2023 17:00:11 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “tipc_link_tnl_prepare”.

Thus avoid the risk for undefined behaviour by moving the definition
for the local variable “fdefq” into an if branch at the end.

This issue was detected by using the Coccinelle software.

Fixes: 58ee86b8c775 ("tipc: adapt link failover for new Gap-ACK algorithm")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 net/tipc/link.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index b3ce24823f50..5aa645e3cb35 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1973,7 +1973,6 @@ void tipc_link_create_dummy_tnl_msg(struct tipc_link *l,
 void tipc_link_tnl_prepare(struct tipc_link *l, struct tipc_link *tnl,
 			   int mtyp, struct sk_buff_head *xmitq)
 {
-	struct sk_buff_head *fdefq = &tnl->failover_deferdq;
 	struct sk_buff *skb, *tnlskb;
 	struct tipc_msg *hdr, tnlhdr;
 	struct sk_buff_head *queue = &l->transmq;
@@ -2100,6 +2099,8 @@ void tipc_link_tnl_prepare(struct tipc_link *l, struct tipc_link *tnl,
 	tipc_link_xmit(tnl, &tnlq, xmitq);

 	if (mtyp == FAILOVER_MSG) {
+		struct sk_buff_head *fdefq = &tnl->failover_deferdq;
+
 		tnl->drop_point = l->rcv_nxt;
 		tnl->failover_reasm_skb = l->reasm_buf;
 		l->reasm_buf = NULL;
--
2.40.0

Re: [PATCH RESEND] tipc: Reduce scope for the variable “fdefq” in tipc_link_tnl_prepare()

[Jakub Kicinski]

On Sun, 2 Mar 2025 21:30:27 +0100 Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 13 Apr 2023 17:00:11 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “tipc_link_tnl_prepare”.
> 
> Thus avoid the risk for undefined behaviour by moving the definition
> for the local variable “fdefq” into an if branch at the end.
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 58ee86b8c775 ("tipc: adapt link failover for new Gap-ACK algorithm")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Applied without the Fixes tag, thanks.

[PATCH RESEND] scsi: hpsa: Move two variable assignments behind condition checks in hpsa_scsi_ioaccel_raid_map()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 14 Apr 2023 11:00:40 +0200

Addresses of two data structure members were determined before
a corresponding null pointer check in the implementation of
the function “hpsa_scsi_ioaccel_raid_map”.

Thus avoid the risk for undefined behaviour by moving the assignment
for two local variables behind some condition checks.

This issue was detected by using the Coccinelle software.

Fixes: 283b4a9b98b1 ("[SCSI] hpsa: add ioaccell mode 1 RAID offload support.")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/scsi/hpsa.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index af18d20f3079..562bb5eab134 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -5104,8 +5104,8 @@ static int hpsa_scsi_ioaccel_raid_map(struct ctlr_info *h,
 {
 	struct scsi_cmnd *cmd = c->scsi_cmd;
 	struct hpsa_scsi_dev_t *dev = cmd->device->hostdata;
-	struct raid_map_data *map = &dev->raid_map;
-	struct raid_map_disk_data *dd = &map->data[0];
+	struct raid_map_data *map;
+	struct raid_map_disk_data *dd;
 	int is_write = 0;
 	u32 map_index;
 	u64 first_block, last_block;
@@ -5209,6 +5209,8 @@ static int hpsa_scsi_ioaccel_raid_map(struct ctlr_info *h,
 	if (is_write && dev->raid_level != 0)
 		return IO_ACCEL_INELIGIBLE;

+	map = &dev->raid_map;
+
 	/* check for invalid block or wraparound */
 	if (last_block >= le64_to_cpu(map->volume_blk_cnt) ||
 		last_block < first_block)
@@ -5397,6 +5399,7 @@ static int hpsa_scsi_ioaccel_raid_map(struct ctlr_info *h,
 	if (!c->phys_disk)
 		return IO_ACCEL_INELIGIBLE;

+	dd = &map->data[0];
 	disk_handle = dd[map_index].ioaccel_handle;
 	disk_block = le64_to_cpu(map->disk_starting_blk) +
 			first_row * le16_to_cpu(map->strip_size) +
--
2.40.0

[PATCH RESEND] drm/mm: Adjust input parameter validation in DECLARE_NEXT_HOLE_ADDR()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 17 Apr 2023 11:26:34 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the macro “DECLARE_NEXT_HOLE_ADDR”.

Thus avoid the risk for undefined behaviour by moving the assignment
for the variable “node” behind the null pointer check.

This issue was detected by using the Coccinelle software.

Fixes: 5fad79fd66ff ("drm/mm: cleanup and improve next_hole_*_addr()")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/drm_mm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index 8257f9d4f619..95c316aa36e5 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -389,9 +389,13 @@ first_hole(struct drm_mm *mm,
 #define DECLARE_NEXT_HOLE_ADDR(name, first, last)			\
 static struct drm_mm_node *name(struct drm_mm_node *entry, u64 size)	\
 {									\
-	struct rb_node *parent, *node = &entry->rb_hole_addr;		\
+	struct rb_node *parent, *node;					\
 									\
-	if (!entry || RB_EMPTY_NODE(node))				\
+	if (!entry)							\
+		return NULL;						\
+									\
+	node = &entry->rb_hole_addr;					\
+	if (RB_EMPTY_NODE(node))					\
 		return NULL;						\
 									\
 	if (usable_hole_addr(node->first, size)) {			\
--
2.40.0

[PATCH RESEND] iwlegacy: Adjust input parameter validation in il_set_ht_add_station()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 19 Apr 2023 18:35:55 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “il_set_ht_add_station”.

Thus avoid the risk for undefined behaviour by moving the assignment
for the variable “sta_ht_inf” behind the null pointer check.

This issue was detected by using the Coccinelle software.


Delete also the jump target “done” by using return statements directly
for two if branches.

Fixes: 046d2e7c50e3 ("mac80211: prepare sta handling for MLO support")
Fixes: e7392364fcd1 ("iwlegacy: indentions and whitespaces")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/intel/iwlegacy/common.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
index 96002121bb8b..8f6fd17b02a8 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -1863,11 +1863,15 @@ EXPORT_SYMBOL(il_send_add_sta);
 static void
 il_set_ht_add_station(struct il_priv *il, u8 idx, struct ieee80211_sta *sta)
 {
-	struct ieee80211_sta_ht_cap *sta_ht_inf = &sta->deflink.ht_cap;
+	struct ieee80211_sta_ht_cap *sta_ht_inf;
 	__le32 sta_flags;

-	if (!sta || !sta_ht_inf->ht_supported)
-		goto done;
+	if (!sta)
+		return;
+
+	sta_ht_inf = &sta->deflink.ht_cap;
+	if (!sta_ht_inf->ht_supported)
+		return;

 	D_ASSOC("spatial multiplexing power save mode: %s\n",
 		(sta->deflink.smps_mode == IEEE80211_SMPS_STATIC) ? "static" :
@@ -1906,8 +1910,6 @@ il_set_ht_add_station(struct il_priv *il, u8 idx, struct ieee80211_sta *sta)
 		sta_flags &= ~STA_FLG_HT40_EN_MSK;

 	il->stations[idx].sta.station_flags = sta_flags;
-done:
-	return;
 }

 /*
--
2.40.0

[PATCH RESEND] iwlwifi: Adjust input parameter validation in iwl_sta_calc_ht_flags()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 19 Apr 2023 19:19:34 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “iwl_sta_calc_ht_flags”.

Thus avoid the risk for undefined behaviour by moving the assignment
for the variable “sta_ht_inf” behind the null pointer check.

This issue was detected by using the Coccinelle software.

Fixes: 046d2e7c50e3 ("mac80211: prepare sta handling for MLO support")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/wireless/intel/iwlwifi/dvm/sta.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/sta.c b/drivers/net/wireless/intel/iwlwifi/dvm/sta.c
index cef43cf80620..74814ce0155e 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/sta.c
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/sta.c
@@ -147,7 +147,7 @@ static void iwl_sta_calc_ht_flags(struct iwl_priv *priv,
 				  struct iwl_rxon_context *ctx,
 				  __le32 *flags, __le32 *mask)
 {
-	struct ieee80211_sta_ht_cap *sta_ht_inf = &sta->deflink.ht_cap;
+	struct ieee80211_sta_ht_cap *sta_ht_inf;

 	*mask = STA_FLG_RTS_MIMO_PROT_MSK |
 		STA_FLG_MIMO_DIS_MSK |
@@ -156,7 +156,11 @@ static void iwl_sta_calc_ht_flags(struct iwl_priv *priv,
 		STA_FLG_AGG_MPDU_DENSITY_MSK;
 	*flags = 0;

-	if (!sta || !sta_ht_inf->ht_supported)
+	if (!sta)
+		return;
+
+	sta_ht_inf = &sta->deflink.ht_cap;
+	if (!sta_ht_inf->ht_supported)
 		return;

 	IWL_DEBUG_INFO(priv, "STA %pM SM PS mode: %s\n",
--
2.40.0

Re: [PATCH RESEND] iwlwifi: Adjust input parameter validation in iwl_sta_calc_ht_flags()

[Berg, Benjamin]

On Mon, 2025-03-03 at 14:18 +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 19 Apr 2023 19:19:34 +0200
> 
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “iwl_sta_calc_ht_flags”.
> 
> Thus avoid the risk for undefined behaviour by moving the assignment
> for the variable “sta_ht_inf” behind the null pointer check.

I am a bit confused, I don't see any risk of undefined behaviour here.

The change is obviously fine, and I guess one can argue that it is less
confusing as the compiler will generate a warning if one uses the
variable before assignment.

However, the code is both well defined and correct. If sta is NULL then
sta_ht_inf is never used, so the fact that it is effectively a NULL
pointer [offsetof(struct ieee80211_sta, deflink.ht_cap)] does not
matter.

Benjamin

> This issue was detected by using the Coccinelle software.
> 
> Fixes: 046d2e7c50e3 ("mac80211: prepare sta handling for MLO
> support")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/net/wireless/intel/iwlwifi/dvm/sta.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/sta.c
> b/drivers/net/wireless/intel/iwlwifi/dvm/sta.c
> index cef43cf80620..74814ce0155e 100644
> --- a/drivers/net/wireless/intel/iwlwifi/dvm/sta.c
> +++ b/drivers/net/wireless/intel/iwlwifi/dvm/sta.c
> @@ -147,7 +147,7 @@ static void iwl_sta_calc_ht_flags(struct iwl_priv
> *priv,
>  				  struct iwl_rxon_context *ctx,
>  				  __le32 *flags, __le32 *mask)
>  {
> -	struct ieee80211_sta_ht_cap *sta_ht_inf = &sta-
> >deflink.ht_cap;
> +	struct ieee80211_sta_ht_cap *sta_ht_inf;
> 
>  	*mask = STA_FLG_RTS_MIMO_PROT_MSK |
>  		STA_FLG_MIMO_DIS_MSK |
> @@ -156,7 +156,11 @@ static void iwl_sta_calc_ht_flags(struct
> iwl_priv *priv,
>  		STA_FLG_AGG_MPDU_DENSITY_MSK;
>  	*flags = 0;
> 
> -	if (!sta || !sta_ht_inf->ht_supported)
> +	if (!sta)
> +		return;
> +
> +	sta_ht_inf = &sta->deflink.ht_cap;
> +	if (!sta_ht_inf->ht_supported)
>  		return;
> 
>  	IWL_DEBUG_INFO(priv, "STA %pM SM PS mode: %s\n",
> --
> 2.40.0
> 

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928