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 - [Medium] scsi: sg: unrelated and broken blktrace CAP_SYS_ADMIN gate in sg_ioctl_common() - [Medium] scsi: st: misattributed GFP_KERNEL_ACCOUNT probe charges in new_tape_buffer() - [Low] scsi: sg: commit message cites non-existent history in sg_build_indirect() 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] --
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 -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
