diff mbox series

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

Message ID 6958583a-77c0-41ca-8f80-7ff647b385bb@web.de (mailing list archive)
State New
Delegated to: Netdev Maintainers
Headers show
Series [RESEND] qed: Move a variable assignment behind a null pointer check in two functions | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 30 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2025-03-02--18-00 (tests: 893)

Commit Message

Markus Elfring March 2, 2025, 4:55 p.m. UTC 
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(-)

--
2.40.0

Comments

Michal Swiatkowski March 3, 2025, 6:21 a.m. UTC | #1 
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
Dan Carpenter March 3, 2025, 7:40 a.m. UTC | #2 
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
Markus Elfring March 3, 2025, 8:22 a.m. UTC | #3 
> 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
Dan Carpenter March 3, 2025, 8:28 a.m. UTC | #4 
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
Markus Elfring March 3, 2025, 10:25 a.m. UTC | #5 
> 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
Kory Maincent March 3, 2025, 5:35 p.m. UTC | #6 
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,
Markus Elfring March 3, 2025, 5:55 p.m. UTC | #7 
> 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
diff mbox series

Patch

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