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

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