Re: Re: Re: [Patch] bsg: initialize request and reply payloads in bsg_prepare_job

Re: Re: Re: [Patch] bsg: initialize request and reply payloads in bsg_prepare_job

[라종휘]

On 2/6/26 13:58 PM, ??? wrote:
> On 2/6/26 00:45, Hannes Reinecke wrote:
>> On 2/5/26 14:42, Jens Axboe wrote:
>>> On 2/4/26 10:32 PM, ??? wrote:
>>>> bsg: initialize request and reply payloads in bsg_prepare_job
>>>>
>>>> struct bsg_job payloads contain fields that are only populated by
>>>> certain commands, such as sg_list pointers.
>>>>
>>>> Because struct bsg_job is allocated with kmalloc(), memory may be
>>>> reused across requests. If a command does not populate all payload
>>>> fields, stale state from a previous job may remain and later be
>>>> misinterpreted during cleanup, potentially leading to use-after-free
>>>> or double-free issues.
>>>>
>>>> Initialize both request and reply payloads at the beginning of job
>>>> preparation to ensure a clean state for all commands.
>>>>
>>>> Signed-off-by: Jonghwi Rha 
>>>>
>>>> diff --git a/block/bsg-lib.c b/block/bsg-lib.c
>>>> index 32da4a4429ce..0fbf8e311c03 100644
>>>> --- a/block/bsg-lib.c
>>>> +++ b/block/bsg-lib.c
>>>> @@ -234,6 +234,12 @@ static bool bsg_prepare_job(struct device *dev, struct request *req)
>>>>          struct bsg_job *job = blk_mq_rq_to_pdu(req);
>>>>          int ret;
>>>>
>>>> +      /* Clear stale SG state since bsg_job is reused as a request PDU */
>>>> +      job->request_payload.sg_list = NULL;
>>>> +      job->request_payload.sg_cnt = 0;
>>>> +      job->reply_payload.sg_list = NULL;
>>>> +      job->reply_payload.sg_cnt = 0;
>>>> +
>>>>          job->timeout = req->timeout;
>>>>
>>>>          if (req->bio) {
>>>
>>> The patch is white-space damaged, tabs are spaces. But I can fix that
>>> up. Do we just want to do a memset(job, 0, sizeof(*job)) here to avoid
>>> any oddities like this in the future?
>>>
>>
>> That might indeed be better.
>
> The suggested method impairs normal operation. If bsg_prepare_job performs
> a zero‑memset for the job structure, all request‑related information set on
> the driver side before the call will be lost. Therefore, if it runs as is,
> it will go to ufs_bsg_request and cause a null‑pointer access.
>
> Currently, the original patch has no functional impact.
>
> The blank problem seems to be due to a mistake I made while copying and pasting
> the patch. I am reattaching the patch below. If needed, I can attach the patch
> and resend the new email.
>
>
> [PATCH] bsg: initialize request and reply payloads in bsg_prepare_job
>
> struct bsg_job payloads contain fields that are only populated by
> certain commands, such as sg_list pointers.
>
> Because struct bsg_job is allocated with kmalloc(), memory may be
> reused across requests. If a command does not populate all payload
> fields, stale state from a previous job may remain and later be
> misinterpreted during cleanup, potentially leading to use-after-free
> or double-free issues.
>
> Initialize both request and reply payloads at the beginning of job
> preparation to ensure a clean state for all commands.
>
> Signed-off-by: Jonghwi Rha 
> ---
>  block/bsg-lib.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/block/bsg-lib.c b/block/bsg-lib.c
> index 32da4a4429ce..0fbf8e311c03 100644
> --- a/block/bsg-lib.c
> +++ b/block/bsg-lib.c
> @@ -234,6 +234,12 @@ static bool bsg_prepare_job(struct device *dev, struct request *req)
>  	struct bsg_job *job = blk_mq_rq_to_pdu(req);
>  	int ret;
>  
> +	/* Clear stale SG state since bsg_job is reused as a request PDU */
> +	job->request_payload.sg_list = NULL;
> +	job->request_payload.sg_cnt = 0;
> +	job->reply_payload.sg_list = NULL;
> +	job->reply_payload.sg_cnt = 0;
> +
> 	job->timeout = req->timeout;
> 
> 	if (req->bio) {
> -- 

> Regards,
> Jonghwi,

--

Since there was no reply, I am resending the email as a reminder. First,
I have confirmed in my environment that, as you suggested, memset as 0 for
all 'job' struct elements eventually results an error. The reason is, as I 
mentioned above, that the request/reply gets lost before re-using.

Also, since other elements in the structure are reused, so they are not 
relevant to the current issue.

If the code execution point is not ideal, there is also the option of zeroising
after freeing the memory allocation.

Jonghwi,

Re: [Patch] bsg: initialize request and reply payloads in bsg_prepare_job

[Jens Axboe]

On 3/18/26 4:20 AM, ??? wrote:
> On 2/6/26 13:58 PM, ??? wrote:
>> On 2/6/26 00:45, Hannes Reinecke wrote:
>>> On 2/5/26 14:42, Jens Axboe wrote:
>>>> On 2/4/26 10:32 PM, ??? wrote:
>>>>> bsg: initialize request and reply payloads in bsg_prepare_job
>>>>>
>>>>> struct bsg_job payloads contain fields that are only populated by
>>>>> certain commands, such as sg_list pointers.
>>>>>
>>>>> Because struct bsg_job is allocated with kmalloc(), memory may be
>>>>> reused across requests. If a command does not populate all payload
>>>>> fields, stale state from a previous job may remain and later be
>>>>> misinterpreted during cleanup, potentially leading to use-after-free
>>>>> or double-free issues.
>>>>>
>>>>> Initialize both request and reply payloads at the beginning of job
>>>>> preparation to ensure a clean state for all commands.
>>>>>
>>>>> Signed-off-by: Jonghwi Rha 
>>>>>
>>>>> diff --git a/block/bsg-lib.c b/block/bsg-lib.c
>>>>> index 32da4a4429ce..0fbf8e311c03 100644
>>>>> --- a/block/bsg-lib.c
>>>>> +++ b/block/bsg-lib.c
>>>>> @@ -234,6 +234,12 @@ static bool bsg_prepare_job(struct device *dev, struct request *req)
>>>>>          struct bsg_job *job = blk_mq_rq_to_pdu(req);
>>>>>          int ret;
>>>>>
>>>>> +      /* Clear stale SG state since bsg_job is reused as a request PDU */
>>>>> +      job->request_payload.sg_list = NULL;
>>>>> +      job->request_payload.sg_cnt = 0;
>>>>> +      job->reply_payload.sg_list = NULL;
>>>>> +      job->reply_payload.sg_cnt = 0;
>>>>> +
>>>>>          job->timeout = req->timeout;
>>>>>
>>>>>          if (req->bio) {
>>>>
>>>> The patch is white-space damaged, tabs are spaces. But I can fix that
>>>> up. Do we just want to do a memset(job, 0, sizeof(*job)) here to avoid
>>>> any oddities like this in the future?
>>>>
>>>
>>> That might indeed be better.
>>
>> The suggested method impairs normal operation. If bsg_prepare_job performs
>> a zero?memset for the job structure, all request?related information set on
>> the driver side before the call will be lost. Therefore, if it runs as is,
>> it will go to ufs_bsg_request and cause a null?pointer access.
>>
>> Currently, the original patch has no functional impact.
>>
>> The blank problem seems to be due to a mistake I made while copying and pasting
>> the patch. I am reattaching the patch below. If needed, I can attach the patch
>> and resend the new email.
>>
>>
>> [PATCH] bsg: initialize request and reply payloads in bsg_prepare_job
>>
>> struct bsg_job payloads contain fields that are only populated by
>> certain commands, such as sg_list pointers.
>>
>> Because struct bsg_job is allocated with kmalloc(), memory may be
>> reused across requests. If a command does not populate all payload
>> fields, stale state from a previous job may remain and later be
>> misinterpreted during cleanup, potentially leading to use-after-free
>> or double-free issues.
>>
>> Initialize both request and reply payloads at the beginning of job
>> preparation to ensure a clean state for all commands.
>>
>> Signed-off-by: Jonghwi Rha 
>> ---
>>  block/bsg-lib.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/block/bsg-lib.c b/block/bsg-lib.c
>> index 32da4a4429ce..0fbf8e311c03 100644
>> --- a/block/bsg-lib.c
>> +++ b/block/bsg-lib.c
>> @@ -234,6 +234,12 @@ static bool bsg_prepare_job(struct device *dev, struct request *req)
>>  	struct bsg_job *job = blk_mq_rq_to_pdu(req);
>>  	int ret;
>>  
>> +	/* Clear stale SG state since bsg_job is reused as a request PDU */
>> +	job->request_payload.sg_list = NULL;
>> +	job->request_payload.sg_cnt = 0;
>> +	job->reply_payload.sg_list = NULL;
>> +	job->reply_payload.sg_cnt = 0;
>> +
>> 	job->timeout = req->timeout;
>>
>> 	if (req->bio) {
>> -- 
> 
>> Regards,
>> Jonghwi,
> 
> --
> 
> Since there was no reply, I am resending the email as a reminder.
> First, I have confirmed in my environment that, as you suggested,
> memset?as 0 for all 'job' struct elements eventually results an error.
> The reason is, as I mentioned above, that the request/reply gets lost
> before re-using.
> 
> Also, since other elements in the structure are reused, so they are
> not relevant to the current issue.
> 
> If the code execution point is not ideal, there is also the option of
> zeroising after freeing the memory allocation.

Just send it out as a proper patch and we can take a look at it again.

-- 
Jens Axboe

Re: [Patch] bsg: initialize request and reply payloads in bsg_prepare_job

[Hannes Reinecke]

On 2/5/26 14:42, Jens Axboe wrote:
> On 2/4/26 10:32 PM, ??? wrote:
>> bsg: initialize request and reply payloads in bsg_prepare_job
>>
>> struct bsg_job payloads contain fields that are only populated by
>> certain commands, such as sg_list pointers.
>>
>> Because struct bsg_job is allocated with kmalloc(), memory may be
>> reused across requests. If a command does not populate all payload
>> fields, stale state from a previous job may remain and later be
>> misinterpreted during cleanup, potentially leading to use-after-free
>> or double-free issues.
>>
>> Initialize both request and reply payloads at the beginning of job
>> preparation to ensure a clean state for all commands.
>>
>> Signed-off-by: Jonghwi Rha <jonghwi.rha@samsung.com>
>>
>> diff --git a/block/bsg-lib.c b/block/bsg-lib.c
>> index 32da4a4429ce..0fbf8e311c03 100644
>> --- a/block/bsg-lib.c
>> +++ b/block/bsg-lib.c
>> @@ -234,6 +234,12 @@ static bool bsg_prepare_job(struct device *dev, struct request *req)
>>          struct bsg_job *job = blk_mq_rq_to_pdu(req);
>>          int ret;
>>
>> +       /* Clear stale SG state since bsg_job is reused as a request PDU */
>> +       job->request_payload.sg_list = NULL;
>> +       job->request_payload.sg_cnt = 0;
>> +       job->reply_payload.sg_list = NULL;
>> +       job->reply_payload.sg_cnt = 0;
>> +
>>          job->timeout = req->timeout;
>>
>>          if (req->bio) {
> 
> The patch is white-space damaged, tabs are spaces. But I can fix that
> up. Do we just want to do a memset(job, 0, sizeof(*job)) here to avoid
> any oddities like this in the future?
> 

That might indeed be better.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [Patch] bsg: initialize request and reply payloads in bsg_prepare_job

[Jens Axboe]

On 2/4/26 10:32 PM, ??? wrote:
> bsg: initialize request and reply payloads in bsg_prepare_job
> 
> struct bsg_job payloads contain fields that are only populated by
> certain commands, such as sg_list pointers.
> 
> Because struct bsg_job is allocated with kmalloc(), memory may be
> reused across requests. If a command does not populate all payload
> fields, stale state from a previous job may remain and later be
> misinterpreted during cleanup, potentially leading to use-after-free
> or double-free issues.
> 
> Initialize both request and reply payloads at the beginning of job
> preparation to ensure a clean state for all commands.
> 
> Signed-off-by: Jonghwi Rha <jonghwi.rha@samsung.com>
> 
> diff --git a/block/bsg-lib.c b/block/bsg-lib.c
> index 32da4a4429ce..0fbf8e311c03 100644
> --- a/block/bsg-lib.c
> +++ b/block/bsg-lib.c
> @@ -234,6 +234,12 @@ static bool bsg_prepare_job(struct device *dev, struct request *req)
>         struct bsg_job *job = blk_mq_rq_to_pdu(req);
>         int ret;
> 
> +       /* Clear stale SG state since bsg_job is reused as a request PDU */
> +       job->request_payload.sg_list = NULL;
> +       job->request_payload.sg_cnt = 0;
> +       job->reply_payload.sg_list = NULL;
> +       job->reply_payload.sg_cnt = 0;
> +
>         job->timeout = req->timeout;
> 
>         if (req->bio) {

The patch is white-space damaged, tabs are spaces. But I can fix that
up. Do we just want to do a memset(job, 0, sizeof(*job)) here to avoid
any oddities like this in the future?

-- 
Jens Axboe

[Patch] bsg: initialize request and reply payloads in bsg_prepare_job

[라종휘]

Hello,

This is Jonghwi from Samsung. :)
I am sending you a patch via new email as requested.


bsg: initialize request and reply payloads in bsg_prepare_job

struct bsg_job payloads contain fields that are only populated by
certain commands, such as sg_list pointers.

Because struct bsg_job is allocated with kmalloc(), memory may be
reused across requests. If a command does not populate all payload
fields, stale state from a previous job may remain and later be
misinterpreted during cleanup, potentially leading to use-after-free
or double-free issues.

Initialize both request and reply payloads at the beginning of job
preparation to ensure a clean state for all commands.

Signed-off-by: Jonghwi Rha <jonghwi.rha@samsung.com>

diff --git a/block/bsg-lib.c b/block/bsg-lib.c
index 32da4a4429ce..0fbf8e311c03 100644
--- a/block/bsg-lib.c
+++ b/block/bsg-lib.c
@@ -234,6 +234,12 @@ static bool bsg_prepare_job(struct device *dev, struct request *req)
        struct bsg_job *job = blk_mq_rq_to_pdu(req);
        int ret;

+       /* Clear stale SG state since bsg_job is reused as a request PDU */
+       job->request_payload.sg_list = NULL;
+       job->request_payload.sg_cnt = 0;
+       job->reply_payload.sg_list = NULL;
+       job->reply_payload.sg_cnt = 0;
+
        job->timeout = req->timeout;

        if (req->bio) {


BRs,
Jonghwi,