1. Patchew
  2. linux
  3. View series

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

Markus Elfring posted 1 patch 11 months, 2 weeks ago 
drivers/net/ethernet/qlogic/qed/qed_ll2.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
[PATCH RESEND] qed: Move a variable assignment behind a null pointer check in two functions
Posted by Markus Elfring 11 months, 2 weeks ago 
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
Posted by Michal Swiatkowski 11 months, 2 weeks ago 
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
Posted by Dan Carpenter 11 months, 2 weeks ago 
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
Posted by Markus Elfring 11 months, 2 weeks ago 
> 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
Posted by Dan Carpenter 11 months, 2 weeks ago 
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: [RESEND] qed: Move a variable assignment behind a null pointer check in two functions
Posted by Kory Maincent 11 months, 2 weeks ago 
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
Posted by Markus Elfring 11 months, 2 weeks ago 
> 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
Re: qed: Move a variable assignment behind a null pointer check in two functions
Posted by Markus Elfring 11 months, 2 weeks ago 
> 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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.