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,