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

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

[Markus Elfring]

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.

This issue was detected by using the Coccinelle software.

Fixes: ad85094b293e40e7a2f831b0311a389d952ebd5e ("Revert 'media: staging: atomisp: Remove driver'")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/media/atomisp/pci/atomisp_cmd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 47f18ac5e40e..733a239b5d8a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -2723,11 +2723,13 @@ int atomisp_cp_general_isp_parameters(struct atomisp_sub_device *asd,
 				      struct atomisp_css_params *css_param,
 				      bool from_user)
 {
-	struct atomisp_parameters *cur_config = &css_param->update_flag;
+	struct atomisp_parameters *cur_config;

 	if (!arg || !asd || !css_param)
 		return -EINVAL;

+	cur_config = &css_param->update_flag;
+
 	if (arg->wb_config && (from_user || !cur_config->wb_config)) {
 		if (copy_from_compatible(&css_param->wb_config, arg->wb_config,
 					 sizeof(struct ia_css_wb_config),
--
2.40.0

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: atomisp: Move a variable assignment behind a null pointer check in atomisp_cp_general_isp_parameters()

[Markus Elfring]

> Instead of this patch, can you please audit the callers of this function?

Did any analysis tool publish a call tree already (according to your enquiry)?
https://elixir.bootlin.com/linux/v6.3-rc6/source/drivers/staging/media/atomisp/pci/atomisp_cmd.c#L2721


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

Are you really looking for ways to omit null pointer checks
for three input parameters here?

Would you like to increase the application of any annotations
or API attributes?

Regards,
Markus

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

[Markus Elfring]

> Instead of this patch, can you please audit the callers of this function?

How will the determination evolve further if null pointer checks would be still
relevant for three input parameters according to this function implementation
at the moment?
https://elixir.bootlin.com/linux/v6.3-rc7/source/drivers/staging/media/atomisp/pci/atomisp_cmd.c#L2721


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

Will affected implementation details be reconsidered any more?

Regards,
Markus

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

[Markus Elfring]

> Instead of this patch, can you please audit the callers of this function?

Would you like to add any advices for a similarly affected implementation
of the function “atomisp_set_parameters”?
https://elixir.bootlin.com/linux/v6.3-rc7/source/drivers/staging/media/atomisp/pci/atomisp_cmd.c#L3619

Regards,
Markus

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

[Markus Elfring]

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

[PATCH 1/4] staging: rtl8712: Delete null pointer checks in process_link_qual()

[Markus Elfring]

Date: Thu, 20 Apr 2023 09:54:06 +0200

Omit extra null pointer checks for the input parameter validation
in the implementation of the function “process_link_qual” because
it should be called only with valid pointers.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/rtl8712/rtl8712_recv.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c
index 7da014ab0723..f0789b5ef59b 100644
--- a/drivers/staging/rtl8712/rtl8712_recv.c
+++ b/drivers/staging/rtl8712/rtl8712_recv.c
@@ -866,8 +866,6 @@ static void process_link_qual(struct _adapter *padapter,
 	struct rx_pkt_attrib *pattrib;
 	struct smooth_rssi_data *sqd = &padapter->recvpriv.signal_qual_data;

-	if (!prframe || !padapter)
-		return;
 	pattrib = &prframe->u.hdr.attrib;
 	if (pattrib->signal_qual != 0) {
 		/*
--
2.40.0

[PATCH 2/4] staging: rtl8712: Delete two variables in process_link_qual()

[Markus Elfring]

Date: Thu, 20 Apr 2023 10:42:40 +0200

* Use two values for computations (in one if branch)
  without storing them in intermediate variables.

* Remove the local variables “last_evm” and “tmpVal”
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/rtl8712/rtl8712_recv.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c
index f0789b5ef59b..3a72d0601dc0 100644
--- a/drivers/staging/rtl8712/rtl8712_recv.c
+++ b/drivers/staging/rtl8712/rtl8712_recv.c
@@ -862,7 +862,6 @@ static void query_rx_phy_status(struct _adapter *padapter,
 static void process_link_qual(struct _adapter *padapter,
 			      union recv_frame *prframe)
 {
-	u32	last_evm = 0, tmpVal;
 	struct rx_pkt_attrib *pattrib;
 	struct smooth_rssi_data *sqd = &padapter->recvpriv.signal_qual_data;

@@ -873,8 +872,7 @@ static void process_link_qual(struct _adapter *padapter,
 		 */
 		if (sqd->total_num++ >= PHY_LINKQUALITY_SLID_WIN_MAX) {
 			sqd->total_num = PHY_LINKQUALITY_SLID_WIN_MAX;
-			last_evm = sqd->elements[sqd->index];
-			sqd->total_val -= last_evm;
+			sqd->total_val -= sqd->elements[sqd->index];
 		}
 		sqd->total_val += pattrib->signal_qual;
 		sqd->elements[sqd->index++] = pattrib->signal_qual;
@@ -882,8 +880,8 @@ static void process_link_qual(struct _adapter *padapter,
 			sqd->index = 0;

 		/* <1> Showed on UI for user, in percentage. */
-		tmpVal = sqd->total_val / sqd->total_num;
-		padapter->recvpriv.signal = (u8)tmpVal;
+		padapter->recvpriv.signal = (u8)(sqd->total_val
+						/ sqd->total_num);
 	}
 }

--
2.40.0

[PATCH 3/4] staging: rtl8712: Reduce scope for the variable “sqd” in process_link_qual()

[Markus Elfring]

Date: Thu, 20 Apr 2023 11:00:11 +0200

A local variable was used only within an if branch.
Thus move the definition for the variable “sqd” into
the corresponding code block.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/rtl8712/rtl8712_recv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c
index 3a72d0601dc0..9135c92906ac 100644
--- a/drivers/staging/rtl8712/rtl8712_recv.c
+++ b/drivers/staging/rtl8712/rtl8712_recv.c
@@ -863,10 +863,11 @@ static void process_link_qual(struct _adapter *padapter,
 			      union recv_frame *prframe)
 {
 	struct rx_pkt_attrib *pattrib;
-	struct smooth_rssi_data *sqd = &padapter->recvpriv.signal_qual_data;

 	pattrib = &prframe->u.hdr.attrib;
 	if (pattrib->signal_qual != 0) {
+		struct smooth_rssi_data *sqd = &padapter->recvpriv.signal_qual_data;
+
 		/*
 		 * 1. Record the general EVM to the sliding window.
 		 */
--
2.40.0

[PATCH 4/4] staging: rtl8712: Simplify the usage of an expression in process_link_qual()

[Markus Elfring]

Date: Thu, 20 Apr 2023 11:44:01 +0200

An expression was used at three source code places.
Thus use its value by a corresponding variable.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/rtl8712/rtl8712_recv.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c
index 9135c92906ac..3d0c48918510 100644
--- a/drivers/staging/rtl8712/rtl8712_recv.c
+++ b/drivers/staging/rtl8712/rtl8712_recv.c
@@ -862,10 +862,9 @@ static void query_rx_phy_status(struct _adapter *padapter,
 static void process_link_qual(struct _adapter *padapter,
 			      union recv_frame *prframe)
 {
-	struct rx_pkt_attrib *pattrib;
+	u8 signal_qual = prframe->u.hdr.attrib.signal_qual;

-	pattrib = &prframe->u.hdr.attrib;
-	if (pattrib->signal_qual != 0) {
+	if (signal_qual) {
 		struct smooth_rssi_data *sqd = &padapter->recvpriv.signal_qual_data;

 		/*
@@ -875,8 +874,10 @@ static void process_link_qual(struct _adapter *padapter,
 			sqd->total_num = PHY_LINKQUALITY_SLID_WIN_MAX;
 			sqd->total_val -= sqd->elements[sqd->index];
 		}
-		sqd->total_val += pattrib->signal_qual;
-		sqd->elements[sqd->index++] = pattrib->signal_qual;
+
+		sqd->total_val += signal_qual;
+		sqd->elements[sqd->index++] = signal_qual;
+
 		if (sqd->index >= PHY_LINKQUALITY_SLID_WIN_MAX)
 			sqd->index = 0;

--
2.40.0

[PATCH 0/4] staging: rtl8723bs: Adjustments for rtw_set_802_11_bssid_list_scan()

[Markus Elfring]

Date: Thu, 20 Apr 2023 16:16:32 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Delete a null pointer check
  Return directly after a failed initialisation check
  Delete an unnecessary variable initialisation
  Move a variable assignment behind a condition check

 drivers/staging/rtl8723bs/core/rtw_ioctl_set.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

--
2.40.0

[PATCH 1/4] staging: rtl8723bs: Delete a null pointer check in rtw_set_802_11_bssid_list_scan()

[Markus Elfring]

Date: Thu, 20 Apr 2023 15:30:23 +0200

Omit an extra null pointer check for the input parameter “padapter”
in the implementation of the function “rtw_set_802_11_bssid_list_scan”
because it should be called only with a valid pointer.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/rtl8723bs/core/rtw_ioctl_set.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
index 3b44f0dd5b0a..7c902f50d38b 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
@@ -371,10 +371,6 @@ u8 rtw_set_802_11_bssid_list_scan(struct adapter *padapter, struct ndis_802_11_s
 	struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 	u8 res = true;

-	if (!padapter) {
-		res = false;
-		goto exit;
-	}
 	if (padapter->hw_init_completed == false) {
 		res = false;
 		goto exit;
--
2.40.0

[PATCH 2/4] staging: rtl8723bs: Return directly after a failed initialisation check in rtw_set_802_11_bssid_list_scan()

[Markus Elfring]

Date: Thu, 20 Apr 2023 15:46:37 +0200

1. Return directly if the hardware initialisation was not completed so far.

2. Delete the label “exit” which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/rtl8723bs/core/rtw_ioctl_set.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
index 7c902f50d38b..0c2df854afe7 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
@@ -371,10 +371,8 @@ u8 rtw_set_802_11_bssid_list_scan(struct adapter *padapter, struct ndis_802_11_s
 	struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 	u8 res = true;

-	if (padapter->hw_init_completed == false) {
-		res = false;
-		goto exit;
-	}
+	if (!padapter->hw_init_completed)
+		return false;

 	if ((check_fwstate(pmlmepriv, _FW_UNDER_SURVEY|_FW_UNDER_LINKING) == true) ||
 		(pmlmepriv->LinkDetectInfo.bBusyTraffic == true)) {
@@ -391,8 +389,6 @@ u8 rtw_set_802_11_bssid_list_scan(struct adapter *padapter, struct ndis_802_11_s

 		spin_unlock_bh(&pmlmepriv->lock);
 	}
-exit:
-
 	return res;
 }

--
2.40.0

[PATCH 3/4] staging: rtl8723bs: Delete an unnecessary variable initialisation in rtw_set_802_11_bssid_list_scan()

[Markus Elfring]

Date: Thu, 20 Apr 2023 15:55:48 +0200

The variable “res” will eventually be set to an appropriate value
a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/rtl8723bs/core/rtw_ioctl_set.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
index 0c2df854afe7..0716b704106d 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
@@ -369,7 +369,7 @@ u8 rtw_set_802_11_disassociate(struct adapter *padapter)
 u8 rtw_set_802_11_bssid_list_scan(struct adapter *padapter, struct ndis_802_11_ssid *pssid, int ssid_max_num)
 {
 	struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
-	u8 res = true;
+	u8 res;

 	if (!padapter->hw_init_completed)
 		return false;
--
2.40.0

[PATCH 4/4] staging: rtl8723bs: Move a variable assignment behind a condition check in rtw_set_802_11_bssid_list_scan()

[Markus Elfring]

Date: Thu, 20 Apr 2023 16:06:08 +0200

Convert the explicit initialisation of the local variable “pmlmepriv”
at the beginning to an assignment statement directly before
the source code place where this pointer is used.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/rtl8723bs/core/rtw_ioctl_set.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
index 0716b704106d..fc6875b4697e 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
@@ -368,12 +368,13 @@ u8 rtw_set_802_11_disassociate(struct adapter *padapter)

 u8 rtw_set_802_11_bssid_list_scan(struct adapter *padapter, struct ndis_802_11_ssid *pssid, int ssid_max_num)
 {
-	struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
+	struct mlme_priv *pmlmepriv;
 	u8 res;

 	if (!padapter->hw_init_completed)
 		return false;

+	pmlmepriv = &padapter->mlmepriv;
 	if ((check_fwstate(pmlmepriv, _FW_UNDER_SURVEY|_FW_UNDER_LINKING) == true) ||
 		(pmlmepriv->LinkDetectInfo.bBusyTraffic == true)) {
 		/*  Scan or linking is in progress, do nothing. */
--
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