[v1] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

[PATCH] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Posted by Baichuan Qi 1 year, 2 months ago

Change the OR to AND.
The previous code used OR within parentheses to check for
NON-NULL pointer on one of pipe->dest_ring and pipe->status_ring.
The previous code can not guarantee the pipe->dest_ring pointer
is NON-NULL. When certain errors occur, causing pipe->dest_ring
to be NULL while pipe->status_ring remains NON-NULL ,
the subsequent call to ath11k_ce_rx_buf_enqueue_pipe() will
access the NULL pointer, resulting in a driver crash.
If it is assumed that these two pointers will not become NULL
for any reason , then only need to check pipe->dest_ring is or
not a NULL pointer, and no need to check NULL pointer
on pipe->status_ring.

Signed-off-by: Baichuan Qi <zghbqbc@gmail.com>
---
 drivers/net/wireless/ath/ath11k/ce.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
index e66e86bdec20..cc9ad014d800 100644
--- a/drivers/net/wireless/ath/ath11k/ce.c
+++ b/drivers/net/wireless/ath/ath11k/ce.c
@@ -324,7 +324,7 @@ static int ath11k_ce_rx_post_pipe(struct ath11k_ce_pipe *pipe)
 	dma_addr_t paddr;
 	int ret = 0;
 
-	if (!(pipe->dest_ring || pipe->status_ring))
+	if (!(pipe->dest_ring && pipe->status_ring))
 		return 0;
 
 	spin_lock_bh(&ab->ce.ce_lock);
-- 
2.34.1

Re: [PATCH] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Posted by Markus Elfring 1 year, 2 months ago

> Change the OR to AND.
> The previous code …

I would appreciate further improvements for the change description.

* How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12#n145

* See also:
  https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22


…
> +++ b/drivers/net/wireless/ath/ath11k/ce.c
> @@ -324,7 +324,7 @@ static int ath11k_ce_rx_post_pipe(struct ath11k_ce_pipe *pipe)
>  	dma_addr_t paddr;
>  	int ret = 0;
>
> -	if (!(pipe->dest_ring || pipe->status_ring))
> +	if (!(pipe->dest_ring && pipe->status_ring))
>  		return 0;
…

Is there a need to reconsider also such a return value?

Regards,
Markus

Re: Re: [PATCH] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Posted by zgBCQ 1 year, 2 months ago

thanks for your reply

> Is there a need to reconsider also such a return value?

According to the current code (commit d5c65159f289) of 
this function, this function should be terminated directly
after checking the null pointer, the return value may be 
meaningless.

Baichuan Qi

Re: wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Posted by Markus Elfring 1 year, 2 months ago

>> Is there a need to reconsider also such a return value?
>
> According to the current code (commit d5c65159f289) of
> this function, this function should be terminated directly
> after checking the null pointer, the return value may be
> meaningless.
The return value is checked at two source code places at least.

* ath11k_ce_recv_process_cb():
  https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/wireless/ath/ath11k/ce.c#L450

* ath11k_ce_rx_post_buf():
  https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/wireless/ath/ath11k/ce.c#L893


May the detection of a null pointer for the data structure members
“dest_ring” or “status_ring” really be interpreted as a successful execution
of the function “ath11k_ce_rx_post_pipe”?

Regards,
Markus

Re: Re: wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe() ERROR code

Posted by Baichuan Qi 1 year, 2 months ago

thanks for your reply

> May the detection of a null pointer for the data structure members
> “dest_ring” or “status_ring” really be interpreted as a successful execution
> of the function “ath11k_ce_rx_post_pipe”?

i submit this patch just want to ensure the last codes after
`if(...)` in `ath11k_ce_rx_post_pipe` can be successfully executed.

There is no error handling for NULL dest_ring (or status_ring)
in other funtions calling `ath11k_ce_rx_post_pipe()`, because it 
would return 0, like successful end.

I think this patch can not change so many codes in other functions in
ce. This may involve a lot of error handling operations,
and depending on the severity of situation, the driver may even
re_setup the ring.

I will submit a v5 patch, and in that patch an error code will be
returned.

Baichuan Qi

[PATCH v5] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Posted by Baichuan Qi 1 year, 2 months ago

Current implementation of `ath11k_ce_rx_post_pipe()` checks for
NON-NULL of either `dest_ring` or `status_ring` using an OR (||).
Both rings, especially `dest_ring`, should be ensured to be
NON-NULL in this function.

If only one of the rings is valid, such as `dest_ring` is NULL
and `status_ring` is NON-NULL, the OR (||) check would not stop
`ath11k_ce_rx_post_pipe()`, the subsequent call to
`ath11k_ce_rx_buf_enqueue_pipe()` will access the NULL pointer,
resulting in a driver crash.

Fix the NON-NULL check by changing the OR (||) to AND (&&),
and return an error code `-EIO` to indicate
`ath11k_ce_rx_post_pipe()` is stopped with an NULL pointer 
error, ensuring that the function only proceeds when both 
`dest_ring` and `status_ring` are NON-NULL.

Link: https://lore.kernel.org/ath11k/a9ccc947-20b2-4322-84e5-c96aaa604e63@web.de
Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
Signed-off-by: Baichuan Qi <zghbqbc@gmail.com>
---
V4 -> V5: add err code in NULL check
V3 -> V4: reorder describe info
V2 -> V3: add Link URL to mailing list archives
V1 -> V2: rewrite commit message and fix tag

 drivers/net/wireless/ath/ath11k/ce.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
index e66e86bdec20..223dab928453 100644
--- a/drivers/net/wireless/ath/ath11k/ce.c
+++ b/drivers/net/wireless/ath/ath11k/ce.c
@@ -324,8 +324,10 @@ static int ath11k_ce_rx_post_pipe(struct ath11k_ce_pipe *pipe)
 	dma_addr_t paddr;
 	int ret = 0;
 
-	if (!(pipe->dest_ring || pipe->status_ring))
-		return 0;
+	if (!(pipe->dest_ring && pipe->status_ring)) {
+		ret = -EIO;
+		return ret;
+	}
 
 	spin_lock_bh(&ab->ce.ce_lock);
 	while (pipe->rx_buf_needed) {
-- 
2.34.1

Re: [PATCH v5] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Posted by Vasanthakumar Thiagarajan 1 year, 2 months ago


On 11/27/2024 8:08 PM, Baichuan Qi wrote:
> Current implementation of `ath11k_ce_rx_post_pipe()` checks for
> NON-NULL of either `dest_ring` or `status_ring` using an OR (||).
> Both rings, especially `dest_ring`, should be ensured to be
> NON-NULL in this function.
> 
> If only one of the rings is valid, such as `dest_ring` is NULL
> and `status_ring` is NON-NULL, the OR (||) check would not stop
> `ath11k_ce_rx_post_pipe()`, the subsequent call to
> `ath11k_ce_rx_buf_enqueue_pipe()` will access the NULL pointer,
> resulting in a driver crash.
> 
> Fix the NON-NULL check by changing the OR (||) to AND (&&),
> and return an error code `-EIO` to indicate
> `ath11k_ce_rx_post_pipe()` is stopped with an NULL pointer
> error, ensuring that the function only proceeds when both
> `dest_ring` and `status_ring` are NON-NULL.
> 
> Link: https://lore.kernel.org/ath11k/a9ccc947-20b2-4322-84e5-c96aaa604e63@web.de
> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")

This does not really fix any real issue. Please check ath11k_ce_alloc_pipe()
where initialization would fail if anyone of pipe->dest_ring and
pipe->status_ring allocation fails for ce pipe used for Rx.

> Signed-off-by: Baichuan Qi <zghbqbc@gmail.com>
> ---
> V4 -> V5: add err code in NULL check
> V3 -> V4: reorder describe info
> V2 -> V3: add Link URL to mailing list archives
> V1 -> V2: rewrite commit message and fix tag
> 
>   drivers/net/wireless/ath/ath11k/ce.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
> index e66e86bdec20..223dab928453 100644
> --- a/drivers/net/wireless/ath/ath11k/ce.c
> +++ b/drivers/net/wireless/ath/ath11k/ce.c
> @@ -324,8 +324,10 @@ static int ath11k_ce_rx_post_pipe(struct ath11k_ce_pipe *pipe)
>   	dma_addr_t paddr;
>   	int ret = 0;
>   
> -	if (!(pipe->dest_ring || pipe->status_ring))
> -		return 0;
> +	if (!(pipe->dest_ring && pipe->status_ring)) {
> +		ret = -EIO;
> +		return ret;
> +	}

This will always fail as the caller loops through all the supported ce pipes
and ce pipes used for Tx will not have either dest_ring or status_ring.
Please ensure the patch is tested properly.

So NAK

Vasanth

Re: Re: [PATCH v5] NAK

Posted by Baichuan Qi 1 year, 2 months ago

Thanks for your reply

The reason I submit this patch is that the current 
`ath11k_ce_rx_post_pipe()` NULL pointer check does not ensure 
that `dest_ring` is NON-NULL. And it is not clear to show the 
filtering of tx ce pipes

> This does not really fix any real issue. Please check ath11k_ce_alloc_pipe()
> where initialization would fail if anyone of pipe->dest_ring and
> pipe->status_ring allocation fails for ce pipe used for Rx.

When the driver is running normally, the results of the 
following three are equal:
---
(pipe->dest_ring || pipe->status_ring) // current code
(pipe->dest_ring && pipe->status_ring)
(pipe->dest_ring)
---
However, when some errors occur and `dest_ring` is abnormal,
the OR operation cannot guarantee that the pointer is NON-NULL.

> This will always fail as the caller loops through all the supported ce pipes
> and ce pipes used for Tx will not have either dest_ring or status_ring.
> Please ensure the patch is tested properly.

I tested [PATCH v5] and indeed the wrong return value will lead 
to wrong results when the pointer is null.

Please refer to [PATCH v4].
Link: https://lore.kernel.org/ath11k/20241127114310.26085-1-zghbqbc@gmail.com/
Although it does not return an error code, it can ensure that 
when an unknown error occurs and causes the status of 
`dest_ring` and `status_ring` to be different, the subsequent 
code will not access the null pointer, which will only 
cause the driver to fall into loops.

Thanks for you read.

Thanks
Baichuan Qi

[PATCH v2 1/1] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Posted by Baichuan Qi 1 year, 2 months ago

This patch fixes the NON-NULL check by changing the OR (||)
to AND (&&), ensuring that the function only proceeds
when both `dest_ring` and `status_ring` are NON-NULL.

The current implementation of `ath11k_ce_rx_post_pipe` checks for
NON-NULL of either `dest_ring` or `status_ring` using a
logical OR (||). However, both rings, especially `dest_ring`,
should be ensured to be NON-NULL in this function.
If only one of the rings is valid, such as `dest_ring` is NULL
and `status_ring` is NON-NULL, the subsequent call to
`ath11k_ce_rx_buf_enqueue_pipe()` will access the NULL pointer,
resulting in a driver crash.

Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
Signed-off-by: Baichuan Qi <zghbqbc@gmail.com>
---
 drivers/net/wireless/ath/ath11k/ce.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
index e66e86bdec20..cc9ad014d800 100644
--- a/drivers/net/wireless/ath/ath11k/ce.c
+++ b/drivers/net/wireless/ath/ath11k/ce.c
@@ -324,7 +324,7 @@ static int ath11k_ce_rx_post_pipe(struct ath11k_ce_pipe *pipe)
 	dma_addr_t paddr;
 	int ret = 0;
 
-	if (!(pipe->dest_ring || pipe->status_ring))
+	if (!(pipe->dest_ring && pipe->status_ring))
 		return 0;
 
 	spin_lock_bh(&ab->ce.ce_lock);
-- 
2.34.1

Re: [PATCH v4?] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Posted by Markus Elfring 1 year, 2 months ago

> This patch fixes the NON-NULL check by …

> ---
>  drivers/net/wireless/ath/ath11k/ce.c | 2 +-
…

Did you overlook any guidance once more?

See also:
* https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22

* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12#n94


Regards,
Markus

[PATCH v3] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Posted by Baichuan Qi 1 year, 2 months ago

Fix the NON-NULL check by changing the OR (||) to AND (&&),
ensuring that the function only proceeds when both `dest_ring`
and `status_ring` are NON-NULL.

The current implementation of `ath11k_ce_rx_post_pipe` checks for
NON-NULL of either `dest_ring` or `status_ring` using a
logical OR (||). However, both rings, especially `dest_ring`,
should be ensured to be NON-NULL in this function.
If only one of the rings is valid, such as `dest_ring` is NULL
and `status_ring` is NON-NULL, the subsequent call to
`ath11k_ce_rx_buf_enqueue_pipe()` will access the NULL pointer,
resulting in a driver crash.

Link: https://lore.kernel.org/ath11k/a9ccc947-20b2-4322-84e5-c96aaa604e63@web.de
Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
Signed-off-by: Baichuan Qi <zghbqbc@gmail.com>
---
 drivers/net/wireless/ath/ath11k/ce.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
index e66e86bdec20..cc9ad014d800 100644
--- a/drivers/net/wireless/ath/ath11k/ce.c
+++ b/drivers/net/wireless/ath/ath11k/ce.c
@@ -324,7 +324,7 @@ static int ath11k_ce_rx_post_pipe(struct ath11k_ce_pipe *pipe)
 	dma_addr_t paddr;
 	int ret = 0;
 
-	if (!(pipe->dest_ring || pipe->status_ring))
+	if (!(pipe->dest_ring && pipe->status_ring))
 		return 0;
 
 	spin_lock_bh(&ab->ce.ce_lock);
-- 
2.34.1

Re: [PATCH v4?] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Posted by Markus Elfring 1 year, 2 months ago

> Fix the NON-NULL check by …

How do you think about to reorder any information from paragraphs?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12#n45


…
> ---
>  drivers/net/wireless/ath/ath11k/ce.c | 2 +-
…

Will you become more familiar with patch version descriptions?
https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12#n310

Regards,
Markus

[PATCH v4] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Posted by Baichuan Qi 1 year, 2 months ago

Current implementation of `ath11k_ce_rx_post_pipe()` checks for
NON-NULL of either `dest_ring` or `status_ring` using an OR (||).
Both rings, especially `dest_ring`, should be ensured to be
NON-NULL in this function.

If only one of the rings is valid, such as `dest_ring` is NULL
and `status_ring` is NON-NULL, the OR (||) check would not stop
`ath11k_ce_rx_post_pipe()`, the subsequent call to
`ath11k_ce_rx_buf_enqueue_pipe()` will access the NULL pointer,
resulting in a driver crash.

Fix the NON-NULL check by changing the OR (||) to AND (&&),
ensuring that the function only proceeds when both `dest_ring`
and `status_ring` are NON-NULL.

Link: https://lore.kernel.org/ath11k/a9ccc947-20b2-4322-84e5-c96aaa604e63@web.de
Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
Signed-off-by: Baichuan Qi <zghbqbc@gmail.com>
---
V3 -> V4: reorder describe info
V2 -> V3: add Link URL to mailing list archives
V1 -> V2: rewrite commit message and fix tag

 drivers/net/wireless/ath/ath11k/ce.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
index e66e86bdec20..cc9ad014d800 100644
--- a/drivers/net/wireless/ath/ath11k/ce.c
+++ b/drivers/net/wireless/ath/ath11k/ce.c
@@ -324,7 +324,7 @@ static int ath11k_ce_rx_post_pipe(struct ath11k_ce_pipe *pipe)
 	dma_addr_t paddr;
 	int ret = 0;
 
-	if (!(pipe->dest_ring || pipe->status_ring))
+	if (!(pipe->dest_ring && pipe->status_ring))
 		return 0;
 
 	spin_lock_bh(&ab->ce.ce_lock);
-- 
2.34.1

Re: [PATCH v3] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Posted by Kang Yang 1 year, 2 months ago


On 11/27/2024 5:43 PM, Baichuan Qi wrote:
> Fix the NON-NULL check by changing the OR (||) to AND (&&),
> ensuring that the function only proceeds when both `dest_ring`
> and `status_ring` are NON-NULL.
> 
> The current implementation of `ath11k_ce_rx_post_pipe` checks for
> NON-NULL of either `dest_ring` or `status_ring` using a
> logical OR (||). However, both rings, especially `dest_ring`,
> should be ensured to be NON-NULL in this function.
> If only one of the rings is valid, such as `dest_ring` is NULL
> and `status_ring` is NON-NULL, the subsequent call to
> `ath11k_ce_rx_buf_enqueue_pipe()` will access the NULL pointer,
> resulting in a driver crash.
> 
> Link: https://lore.kernel.org/ath11k/a9ccc947-20b2-4322-84e5-c96aaa604e63@web.de
> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
> Signed-off-by: Baichuan Qi <zghbqbc@gmail.com>
> ---

So this is version 3, please remember adding you version change here🙂:

v3: add link URL.
v2: rewrite commit message, add fix tag.

---
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages

>   drivers/net/wireless/ath/ath11k/ce.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
> index e66e86bdec20..cc9ad014d800 100644
> --- a/drivers/net/wireless/ath/ath11k/ce.c
> +++ b/drivers/net/wireless/ath/ath11k/ce.c
> @@ -324,7 +324,7 @@ static int ath11k_ce_rx_post_pipe(struct ath11k_ce_pipe *pipe)
>   	dma_addr_t paddr;
>   	int ret = 0;
>   
> -	if (!(pipe->dest_ring || pipe->status_ring))
> +	if (!(pipe->dest_ring && pipe->status_ring))
>   		return 0;
>   
>   	spin_lock_bh(&ab->ce.ce_lock);

Re: Re: [PATCH v3] wifi: ath11k: Fix NULL pointer check in ath11k_ce_rx_post_pipe()

Posted by Baichuan Qi 1 year, 2 months ago

thanks for your reply

thanks for your help. With your help and Documentation 
I now understand how to write patch change logs.

Baichuan Qi