block: bic maybe null pointer dereference

[PATCH] block: bic maybe null pointer dereference

Posted by yanlonglong 1 month, 1 week ago

Signed-off-by: yanlonglong <yanlonglong@kylinos.cn>
---
 block/bfq-iosched.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 141c602d5e85..27ef736085b1 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -3035,9 +3035,6 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq,
 static void bfq_bfqq_save_state(struct bfq_queue *bfqq)
 {
 	struct bfq_io_cq *bic = bfqq->bic;
-	unsigned int a_idx = bfqq->actuator_idx;
-	struct bfq_iocq_bfqq_data *bfqq_data = &bic->bfqq_data[a_idx];
-
 	/*
 	 * If !bfqq->bic, the queue is already shared or its requests
 	 * have already been redirected to a shared queue; both idle window
@@ -3046,6 +3043,9 @@ static void bfq_bfqq_save_state(struct bfq_queue *bfqq)
 	if (!bic)
 		return;
 
+	unsigned int a_idx = bfqq->actuator_idx;
+	struct bfq_iocq_bfqq_data *bfqq_data = &bic->bfqq_data[a_idx];
+
 	bfqq_data->saved_last_serv_time_ns = bfqq->last_serv_time_ns;
 	bfqq_data->saved_inject_limit =	bfqq->inject_limit;
 	bfqq_data->saved_decrease_time_jif = bfqq->decrease_time_jif;
-- 
2.43.0

Re: [PATCH] block: bic maybe null pointer dereference

Posted by Jens Axboe 1 month, 1 week ago

On 5/5/26 11:56 PM, yanlonglong wrote:
> Signed-off-by: yanlonglong <yanlonglong@kylinos.cn>

Your subject line is incomplete, and your commit message is even more
incomplete.

> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 141c602d5e85..27ef736085b1 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -3035,9 +3035,6 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq,
>  static void bfq_bfqq_save_state(struct bfq_queue *bfqq)
>  {
>  	struct bfq_io_cq *bic = bfqq->bic;
> -	unsigned int a_idx = bfqq->actuator_idx;
> -	struct bfq_iocq_bfqq_data *bfqq_data = &bic->bfqq_data[a_idx];
> -
>  	/*
>  	 * If !bfqq->bic, the queue is already shared or its requests
>  	 * have already been redirected to a shared queue; both idle window
> @@ -3046,6 +3043,9 @@ static void bfq_bfqq_save_state(struct bfq_queue *bfqq)
>  	if (!bic)
>  		return;
>  
> +	unsigned int a_idx = bfqq->actuator_idx;
> +	struct bfq_iocq_bfqq_data *bfqq_data = &bic->bfqq_data[a_idx];
> +
>  	bfqq_data->saved_last_serv_time_ns = bfqq->last_serv_time_ns;
>  	bfqq_data->saved_inject_limit =	bfqq->inject_limit;
>  	bfqq_data->saved_decrease_time_jif = bfqq->decrease_time_jif;

Ehm no, variable declarations go at the top.

Please try again, correcting the title/subject and ACTUALLY write a
commit message. See:

Documentation/process/submitting-patches.rst

-- 
Jens Axboe

[PATCH v2] block: add NULL checks for bic in bfq_bfqq_save_state function

Posted by yanlonglong 1 month, 1 week ago

When the `bic` variable is null, referencing `bfqq_data` through `bic` will
cause the program to crash. Therefore, the null check for `bic` should be
moved to the beginning of the function to prevent referencing a null pointer.

Fixed:fd571df0ac5b289af8（"block, bfq: turn bfqq_data into an array in bfq_io_cq"）
Signed-off-by: yanlonglong <yanlonglong@kylinos.cn>
---
 block/bfq-iosched.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 141c602d5e85..e952e4ea2dd4 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -3035,9 +3035,8 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq,
 static void bfq_bfqq_save_state(struct bfq_queue *bfqq)
 {
 	struct bfq_io_cq *bic = bfqq->bic;
-	unsigned int a_idx = bfqq->actuator_idx;
-	struct bfq_iocq_bfqq_data *bfqq_data = &bic->bfqq_data[a_idx];
-
+	unsigned int a_idx = 0;
+	struct bfq_iocq_bfqq_data *bfqq_data = NULL;
 	/*
 	 * If !bfqq->bic, the queue is already shared or its requests
 	 * have already been redirected to a shared queue; both idle window
@@ -3046,6 +3045,9 @@ static void bfq_bfqq_save_state(struct bfq_queue *bfqq)
 	if (!bic)
 		return;
 
+	a_idx = bfqq->actuator_idx;
+	bfqq_data = &bic->bfqq_data[a_idx];
+
 	bfqq_data->saved_last_serv_time_ns = bfqq->last_serv_time_ns;
 	bfqq_data->saved_inject_limit =	bfqq->inject_limit;
 	bfqq_data->saved_decrease_time_jif = bfqq->decrease_time_jif;
-- 
2.43.0

Re: [PATCH v2] block: add NULL checks for bic in bfq_bfqq_save_state function

Posted by Jens Axboe 1 month, 1 week ago

On 5/6/26 3:04 AM, yanlonglong wrote:
> When the `bic` variable is null, referencing `bfqq_data` through `bic` will
> cause the program to crash. Therefore, the null check for `bic` should be
> moved to the beginning of the function to prevent referencing a null pointer.

Cap at 72 char line length... And title should start with "block, bfq: ".
And should be "add NULL check", singular, see below.

> Fixed:fd571df0ac5b289af8?"block, bfq: turn bfqq_data into an array in bfq_io_cq"?

This tag is wrong, should be a shortened sha and the format is also
wrong (Fixed vs Fixes).

> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 141c602d5e85..e952e4ea2dd4 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -3035,9 +3035,8 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq,
>  static void bfq_bfqq_save_state(struct bfq_queue *bfqq)
>  {
>  	struct bfq_io_cq *bic = bfqq->bic;
> -	unsigned int a_idx = bfqq->actuator_idx;
> -	struct bfq_iocq_bfqq_data *bfqq_data = &bic->bfqq_data[a_idx];
> -
> +	unsigned int a_idx = 0;
> +	struct bfq_iocq_bfqq_data *bfqq_data = NULL;

You're killing the empty line between variables and the comment. And why
is a_idx being moved? This is !bic being NULL, presumably?

-- 
Jens Axboe

[PATCH v3] block: add NULL checks for bic in bfq_bfqq_save_state function

Posted by yanlonglong 1 month, 1 week ago

When the `bic` variable is null, referencing `bfqq_data` through `bic` will
cause the program to crash. Therefore, the null check for `bic` should be
moved to the beginning of the function to prevent referencing a null pointer.

Fixes:fd571df0ac5b("block, bfq: turn bfqq_data into an array in bfq_io_cq")
Signed-off-by: yanlonglong <yanlonglong@kylinos.cn>
---
 block/bfq-iosched.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 141c602d5e85..c8cf8764d48d 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -3036,7 +3036,7 @@ static void bfq_bfqq_save_state(struct bfq_queue *bfqq)
 {
 	struct bfq_io_cq *bic = bfqq->bic;
 	unsigned int a_idx = bfqq->actuator_idx;
-	struct bfq_iocq_bfqq_data *bfqq_data = &bic->bfqq_data[a_idx];
+	struct bfq_iocq_bfqq_data *bfqq_data = NULL;
 
 	/*
 	 * If !bfqq->bic, the queue is already shared or its requests
@@ -3046,6 +3046,7 @@ static void bfq_bfqq_save_state(struct bfq_queue *bfqq)
 	if (!bic)
 		return;
 
+	bfqq_data = &bic->bfqq_data[a_idx];
 	bfqq_data->saved_last_serv_time_ns = bfqq->last_serv_time_ns;
 	bfqq_data->saved_inject_limit =	bfqq->inject_limit;
 	bfqq_data->saved_decrease_time_jif = bfqq->decrease_time_jif;
-- 
2.43.0

Re: [PATCH v3] block: add NULL checks for bic in bfq_bfqq_save_state function

Posted by Jens Axboe 1 month, 1 week ago

On 5/6/26 7:28 PM, yanlonglong wrote:
> When the `bic` variable is null, referencing `bfqq_data` through `bic` will
> cause the program to crash. Therefore, the null check for `bic` should be
> moved to the beginning of the function to prevent referencing a null pointer.

This part is obvious, what really needs explaining here is what conditions
can lead to bic being NULL, as that is supposedly the problem being fixed.

So, which conditions lead to bic being NULL here? A good commit message
should explain the "why" of why a change is being made. It's obvious
from the code change what is being done, what is not obvious is why it's
necessary.

> Fixes:fd571df0ac5b("block, bfq: turn bfqq_data into an array in bfq_io_cq")

Still incorrect, need space after Fixes:

> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 141c602d5e85..c8cf8764d48d 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -3036,7 +3036,7 @@ static void bfq_bfqq_save_state(struct bfq_queue *bfqq)
>  {
>  	struct bfq_io_cq *bic = bfqq->bic;
>  	unsigned int a_idx = bfqq->actuator_idx;
> -	struct bfq_iocq_bfqq_data *bfqq_data = &bic->bfqq_data[a_idx];
> +	struct bfq_iocq_bfqq_data *bfqq_data = NULL;

Minor nit, but why initialize it to NULL?

-- 
Jens Axboe