Re: [PATCH] scsi: sg: clean up request on error paths in sg_read()

[John Garry <john.garry@linux.dev>]

On 9/19/26 21:48, Hui Peng wrote:
> In sg_read(), once sg_get_rq_mark() locates a completed Sg_request and
> transitions srp->done from 1 to 2, an allocation failure in kzalloc() or
> a user-copy fault (-EFAULT) in copy_to_user() / sg_read_oxfer() returns
> or jumps to free_old_hdr without calling sg_finish_rem_req(srp) and
> sg_remove_request(sfp, srp).
> 
> Because srp->done is already 2, subsequent sg_read() calls skip the
> request, permanently leaking srp->bio and keeping sfp->res_in_use = 1
> (blocking SG_SET_RESERVED_SIZE with -EBUSY and wedging non-SG_FLAG_NO_DXFER
> writes with -EDOM) until the file descriptor is closed.
> 
> Always call sg_finish_rem_req(srp) and sg_remove_request(sfp, srp) under
> the free_old_hdr cleanup label, matching sg_new_read().
> 
> Fixes: cb59e8408381 ("[PATCH] sg.c: update")
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>

How was this tested?

> ---
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -480,8 +480,10 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
>   
>   	hp = &srp->header;
>   	old_hdr = kzalloc(SZ_SG_HEADER, GFP_KERNEL);
> -	if (!old_hdr)
> -		return -ENOMEM;
> +	if (!old_hdr) {
> +		retval = -ENOMEM;
> +		goto free_old_hdr;
> +	}
>   
>   	old_hdr->reply_len = (int) hp->timeout;
>   	old_hdr->pack_len = old_hdr->reply_len; /* old, strange behaviour */
> @@ -543,10 +545,10 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
>   		}
>   	} else
>   		count = (old_hdr->result == 0) ? 0 : -EIO;
> -	sg_finish_rem_req(srp);
> -	sg_remove_request(sfp, srp);
>   	retval = count;
>   free_old_hdr:

This is no longer a fully appropriate label name

> +	sg_finish_rem_req(srp);
> +	sg_remove_request(sfp, srp);
>   	kfree(old_hdr);
>   	return retval;
>   }