Re: [PATCH 1/2] block: add BPF kfuncs to read blkcg io.stat

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] A Time-Of-Check to Time-Of-Use (TOCTOU) race condition in `bpf_blkg_dev` can lead to a NULL pointer dereference when accessing `disk->part0`.

Pre-existing issues:
- [High] Concurrent execution of `blkcg_fill_root_iostats()` lacks mutual exclusion, corrupting the `u64_stats_sync` seqcount and potentially deadlocking readers.
--

--- Patch [1]: [PATCH 1/2] block: add BPF kfuncs to read blkcg io.stat ---
commit 5b3bee962ae05470b0da306e0224ebb6eec2d0ce
Author: Ziyang Men <[email protected]>

block: add BPF kfuncs to read blkcg io.stat

This exposes the block I/O controller's per-device statistics to BPF,
mirroring the memory controller kfuncs. It allows BPF programs to read a
cgroup's io.stat counters and flush the stats through new BPF functions.
The patch also modifies blkcg_fill_root_iostats() to be accessible externally.

> diff --git a/block/bpf_blkcg.c b/block/bpf_blkcg.c
> new file mode 100644
> index 0000000000000..48a86f07e198a
> --- /dev/null
> +++ b/block/bpf_blkcg.c
[ ... ]
> @@ -0,0 +1,315 @@
[ ... ]
> +__bpf_kfunc void bpf_blkcg_flush_stats(struct blkcg *blkcg)
> +{
> +	if (!blkcg->css.parent)
> +		blkcg_fill_root_iostats();

[Severity: High]
This is a pre-existing issue, but can this lead to a corrupted sequence
counter and deadlocked readers? 

Multiple BPF programs can call bpf_blkcg_flush_stats() concurrently, which
calls blkcg_fill_root_iostats(). Inside blkcg_fill_root_iostats(), the
update is performed with:

    flags = u64_stats_update_begin_irqsave(&blkg->iostat.sync);
    blkg_iostat_set(&blkg->iostat.cur, &tmp);
    u64_stats_update_end_irqrestore(&blkg->iostat.sync, flags);

Since u64_stats_update_begin_irqsave() only protects against local interrupts,
multiple CPUs executing this concurrently lack an external lock to serialize
writers. Does this allow concurrent writers to corrupt the underlying
seqcount (e.g., leaving it permanently odd), causing subsequent readers
to spin infinitely in u64_stats_fetch_retry()?

> +	else
> +		css_rstat_flush(&blkcg->css);
> +}
[ ... ]
> +__bpf_kfunc u64 bpf_blkg_dev(struct blkcg_gq *blkg)
> +{
> +	if (!blkg->q || !blkg->q->disk)
> +		return 0;
> +
> +	return blkg->q->disk->part0->bd_dev;

[Severity: High]
Can the compiler double-fetch blkg->q->disk here and cause a NULL pointer
dereference?

If a BPF program calls bpf_blkg_dev() while the block device is concurrently
being removed, disk_release() can set disk->queue->disk = NULL synchronously.

Without READ_ONCE(), the compiler is permitted to fetch blkg->q->disk for
the NULL check, and then fetch it again for the return statement. If the
second fetch sees the concurrently-written NULL, it will dereference
NULL->part0 and crash the kernel. Is a local variable paired with
READ_ONCE() needed here?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1