Hi,
在 2026/11/7 21:27, [email protected] 写道:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issues to consider:
> - [High] scsi: sg: GFP_ATOMIC | __GFP_ACCOUNT in sg_build_indirect()
> bypasses memcg max
I'll take another look at the allocation context and whether GFP_ATOMIC
is still required here.
> - [Medium] scsi: sg: unrelated and broken blktrace CAP_SYS_ADMIN gate in
> sg_ioctl_common()
The capability check doesn't belong in this patch. It was accidentally
carried over while splitting the series and should have remained in the
separate blktrace patch. I'll remove it from this patch.
> - [Medium] scsi: st: misattributed GFP_KERNEL_ACCOUNT probe charges in
> new_tape_buffer()
Thanks for pointing this out. v2 drops the st side and keeps the
accounting on the sg side.
> - [Low] scsi: sg: commit message cites non-existent history in
> sg_build_indirect()
The commit message is indeed inaccurate. I'll fix the historical
description and clarify the reserved buffer limit in the next revision.
> It also noticed 1 related pre-existing issue this patch does not
> introduce:
> - [High] scsi: sg: shift-out-of-bounds in sg_build_indirect() [1]
Thanks for pointing that out. It predates the series, and I'll
investigate it separately.
> --
>
> commit ec96b2b0cdd1df3575b4521cd9f81da9824bc2b2
> Author: Tao Cui <[email protected]>
>
> scsi: sg and st: account the command buffer memory to memcg
>
> This commit adds __GFP_ACCOUNT to buffer and descriptor allocations in the
> sg and st drivers to charge them to the calling process's memory cgroup, and
> adds a CAP_SYS_ADMIN check for blktrace ioctls in sg_ioctl_common().
>
>> The sg data buffers are unaccounted: the per-fd reserved buffer, the
>> scatter-gather page array, and the order-0..N pages behind it, plus the
>> per-device tape buffer in st. All of it is user controllable. The
>> reserved size is capped only by the device max_sectors, SG_MAX_QUEUE
>> (16) commands can be in flight per fd, and the fd count is bounded by
>> RLIMIT_NOFILE. Nothing is charged to the memcg of the process driving
>> it. The September 2026 fixes in sg_build_indirect reshaped the element
>> sizing and left the accounting alone.
>
> [Severity: Low]
> Is the commit message accurate about the history of sg_build_indirect() and
> the reserved buffer cap?
>
> In git history, sg_build_indirect() has not been modified since
> aaff5ebaa269 ("scsi: remove the unchecked_isa_dma flag") in 2021, and its
> element sizing has not changed since 10db10d144c0 ("sg: convert the indirect
> IO path to use the block layer") in 2008. In addition, the initial per-fd
> reserved buffer in sg_add_sfp() is capped by min_t(int, def_reserved_size,
> max_sectors_bytes(sdp->device->request_queue)), while only resizing via
> SG_SET_RESERVED_SIZE is capped solely by max_sectors_bytes().
>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index 5408f002e6c01..80d919fa5cf91 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -910,6 +910,19 @@ static int put_compat_request_table(struct
>> compat_sg_req_info __user *o,
>> }
>> #endif
>>
>> +/*
>> + * The sg passthrough of the blktrace ioctls has the same exposure as the
>> + * block device path: holding the node is the only requirement today, and
>> + * a created trace outlives the fd. Must match the gate in
>> + * blk_trace_ioctl().
>> + */
>> +static bool sg_is_blktrace_cmd(unsigned int cmd)
>> +{
>> + return cmd == BLKTRACESETUP || cmd == BLKTRACESETUP2 ||
>> + cmd == BLKTRACESTART || cmd == BLKTRACESTOP ||
>> + cmd == BLKTRACETEARDOWN;
>> +}
>> +
>> static long
>> sg_ioctl_common(struct file *filp, Sg_device *sdp, Sg_fd *sfp,
>> unsigned int cmd_in, void __user *p)
>> @@ -919,6 +932,9 @@ sg_ioctl_common(struct file *filp, Sg_device *sdp, Sg_fd
>> *sfp,
>> Sg_request *srp;
>> unsigned long iflags;
>>
>> + if (sg_is_blktrace_cmd(cmd_in) && !capable(CAP_SYS_ADMIN))
>> + return -EPERM;
>> +
>
> [Severity: Medium]
> Does this capability check belong in a memcg accounting commit, and does it
> cause a UAPI regression for unprivileged users with access to /dev/sg*?
>
> Despite the comment, blk_trace_ioctl() in kernel/trace/blktrace.c does not
> check capable(CAP_SYS_ADMIN); block devices gate these ioctls only on access
> to the device node. Moreover, sg_is_blktrace_cmd() matches BLKTRACESETUP2
> even though sg_ioctl_common() only implements BLKTRACESETUP, BLKTRACESTART,
> BLKTRACESTOP, and BLKTRACETEARDOWN, so unprivileged callers now get -EPERM
> instead of -ENOTTY for BLKTRACESETUP2.
>
>> @@ -1841,7 +1857,7 @@ static int
>> sg_build_sgat(Sg_scatter_hold * schp, const Sg_fd * sfp, int tablesize)
>> {
>> int sg_bufflen = tablesize * sizeof(struct page *);
>> - gfp_t gfp_flags = GFP_ATOMIC | __GFP_NOWARN;
>> + gfp_t gfp_flags = GFP_ATOMIC | __GFP_NOWARN | __GFP_ACCOUNT;
>>
>> schp->pages = kzalloc(sg_bufflen, gfp_flags);
>> if (!schp->pages)
>> @@ -1856,7 +1872,8 @@ sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp,
>> int buff_size)
>> int ret_sz = 0, i, k, rem_sz, num, mx_sc_elems;
>> int sg_tablesize = sfp->parentdp->sg_tablesize;
>> int blk_size = buff_size, order;
>> - gfp_t gfp_mask = GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN | __GFP_ZERO;
>> + gfp_t gfp_mask = GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN |
>> + __GFP_ACCOUNT | __GFP_ZERO;
>>
>> if (blk_size < 0)
>> return -EFAULT;
>
> [Severity: High]
> This problem wasn't introduced by this patch, but can the ret_sz shift in
> sg_build_indirect() trigger a signed shift-out-of-bounds [1]?
>
> drivers/scsi/sg.c:sg_build_indirect() {
> ...
> num = scatter_elem_sz;
> ...
> order = get_order(num);
> retry:
> ret_sz = 1 << (PAGE_SHIFT + order);
> ...
> }
>
> On 4 KB page architectures, setting the scatter_elem_sz module parameter
> near INT_MAX gives order == 19, so 1 << (PAGE_SHIFT + order) evaluates the
> signed 32-bit shift 1 << 31, triggering UBSAN or yielding a negative ret_sz
> that corrupts the allocation loop.
>
>> @@ -2146,7 +2163,7 @@ sg_add_sfp(Sg_device * sdp)
>> unsigned long iflags;
>> int bufflen;
>>
>> - sfp = kzalloc_obj(*sfp, GFP_ATOMIC | __GFP_NOWARN);
>> + sfp = kzalloc_obj(*sfp, GFP_ATOMIC | __GFP_NOWARN | __GFP_ACCOUNT);
>> if (!sfp)
>> return ERR_PTR(-ENOMEM);
>
> [Severity: High]
> Does combining GFP_ATOMIC with __GFP_ACCOUNT in sg_build_sgat(),
> sg_build_indirect(), and sg_add_sfp() allow processes to bypass their memcg
> memory.max limit?
>
> GFP_ATOMIC sets __GFP_HIGH and lacks __GFP_DIRECT_RECLAIM. When a memcg is
> at memory.max, try_charge_memcg() skips reclaim because the allocation
> cannot block and then force-charges past memory.max because __GFP_HIGH is
> set:
>
> mm/memcontrol.c:try_charge_memcg() {
> ...
> nomem:
> if (!(gfp_mask & (__GFP_NOFAIL | __GFP_HIGH)))
> return -ENOMEM;
> force:
> page_counter_charge(&memcg->memory, nr_pages);
> ...
> }
>
> All callers of sg_add_sfp(), sg_build_sgat(), and sg_build_indirect()
> (sg_open(), SG_SET_RESERVED_SIZE in sg_ioctl_common(), and sg_start_req())
> run in sleepable process context holding only mutexes, so using GFP_KERNEL
> would allow memcg reclaim and enforce memory.max.
>
>> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
>> index f1c3c49466379..7083d37622c08 100644
>> --- a/drivers/scsi/st.c
>> +++ b/drivers/scsi/st.c
>> @@ -3973,7 +3973,7 @@ static struct st_buffer *new_tape_buffer(int max_sg)
>> {
>> struct st_buffer *tb;
>>
>> - tb = kzalloc_obj(struct st_buffer);
>> + tb = kzalloc_obj(struct st_buffer, GFP_KERNEL_ACCOUNT);
>> if (!tb) {
>> printk(KERN_NOTICE "st: Can't allocate new tape buffer.\n");
>> return NULL;
>> @@ -3982,7 +3982,8 @@ static struct st_buffer *new_tape_buffer(int max_sg)
>> tb->use_sg = max_sg;
>> tb->buffer_size = 0;
>>
>> - tb->reserved_pages = kzalloc_objs(struct page *, max_sg);
>> + tb->reserved_pages = kzalloc_objs(struct page *, max_sg,
>> + GFP_KERNEL_ACCOUNT);
>> if (!tb->reserved_pages) {
>> kfree(tb);
>> return NULL;
>
> [Severity: Medium]
> Does using GFP_KERNEL_ACCOUNT in new_tape_buffer() charge the device probe
> context instead of the process driving tape I/O?
>
> new_tape_buffer() is only called from st_probe() when a tape device is
> attached, and both tb and tb->reserved_pages live until scsi_tape_release(),
> so they are never charged to the process performing I/O. Meanwhile, the
> per-I/O page array allocated in the driving process's context during direct
> I/O in sgl_map_user_pages() (pages = kmalloc_objs(*pages, max_pages))
> remains unaccounted.
>
> [1] https://sashiko.dev/#/bug/linux-ccac9556-cc9e-4b08-b44f-5f7ffb342827
>
