Re: [PATCH] scsi: sg: update local scatter size when clamping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: sg: compiler reload of lockless global scatter_elem_sz bypasses clamp
--

commit c196717f4b3e3f71b450481a3b45198601caf07e
Author: Jiacheng Xu <stitch@zju.edu.cn>

scsi: sg: update local scatter size when clamping

When scatter_elem_sz is set to a value smaller than PAGE_SIZE through
its writable sysfs module parameter, sg_build_indirect() updates the
global scatter element size variables but leaves the local num
variable unchanged.

> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 3f9e08725602c..64ce489128672 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -1863,6 +1863,7 @@ sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
>  	num = scatter_elem_sz;
>  	if (unlikely(num != scatter_elem_sz_prev)) {
>  		if (num < PAGE_SIZE) {
> +			num = PAGE_SIZE;
>  			scatter_elem_sz = PAGE_SIZE;
>  			scatter_elem_sz_prev = PAGE_SIZE;
>  		} else

[Severity: Medium]
Since scatter_elem_sz is a global variable accessed locklessly in
sg_build_indirect(), is there a risk the compiler could reload its value
from memory after the initial read without READ_ONCE()?

If scatter_elem_sz is changed to 0 by a concurrent sysfs write right after
the `num < PAGE_SIZE` check, could a compiler reload in the `else` branch
assign 0 to scatter_elem_sz_prev?

drivers/scsi/sg.c:sg_build_indirect() {
...
		} else
			scatter_elem_sz_prev = num;
...
}

If this happens, on the next execution both variables would be 0, causing
`num != scatter_elem_sz_prev` to evaluate to false.

Would this permanently bypass the clamp and pass 0 to get_order(),
reproducing the exact shift-out-of-bounds bug this patch is attempting to
fix?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012135955.78357-1-stitch@zju.edu.cn?part=1