Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] block: zram: signed/unsigned promotion masks hardware errors in qpace_compress - [High] block: zram: missing qpace availability check causes NULL pointer dereference - [High] block: zram: unbinding qpace driver causes Use-After-Free in backend - [Medium] block: zram: qpace backend uses bounce buffers instead of claimed zero-copy DMA mapping --
commit e4fba83b2c0230490d177ce709e7ea22807d84d4 Author: Georgi Djakov <[email protected]> zram: Add QPaCE zcomp backend Add a zcomp backend for the Qualcomm Page Compression Engine (QPaCE) so zram can expose qpace-lz4 as a selectable compression algorithm when the QPaCE driver is available. > Compress and decompress operations are handled via the QPaCE urgent > synchronous path: each request DMA-maps the source and destination > buffers, issues a blocking hardware command, and returns the result size. [Severity: Medium] Does the implementation contradict the commit message's claim about DMA mapping the request buffers? The code allocates static bounce buffers and performs synchronous memory copies instead of mapping the actual request memory directly. > diff --git a/drivers/block/zram/backend_qpace.c > b/drivers/block/zram/backend_qpace.c > new file mode 100644 > index 0000000000000..3af13727adefc > --- /dev/null > +++ b/drivers/block/zram/backend_qpace.c [ ... ] > +static int qpace_create_ctx(struct zcomp_params *params, struct zcomp_ctx > *ctx) > +{ > + struct device *dev = qpace_get_dma_dev(); [Severity: High] Is it safe to unconditionally call qpace_get_dma_dev() here without verifying if the hardware actually probed via qpace_is_dev_available()? If the QPaCE driver hasn't probed successfully or is missing, the global qpace_priv pointer remains NULL, and qpace_get_dma_dev() dereferences it: drivers/soc/qcom/qpace.c:qpace_get_dma_dev() { return qpace_priv->dev; } Could this cause a kernel panic if a user writes 'qpace-lz4' to /sys/block/zram0/comp_algorithm on an unsupported system? [Severity: High] Additionally, does this cache the device pointer without acquiring a robust reference via get_device() or taking a module reference? If the QPaCE driver is unbound via sysfs, qpace_priv is destroyed. Subsequent compress operations or teardown might access the freed device pointer or qpace_priv structure, leading to a use-after-free. > + struct qpace_ctx *cc; > + > + if (!dev) > + return -ENODEV; > + > + cc = kzalloc_obj(*cc, GFP_KERNEL); > + if (!cc) > + return -ENOMEM; > + > + cc->dev = dev; > + cc->in_buf = dma_alloc_coherent(dev, PAGE_SIZE, &cc->in_dma, > GFP_KERNEL); [Severity: Medium] This statically allocates a bounce buffer rather than preparing to map the dynamic request buffers. Does this introduce CPU overhead by breaking the zero-copy DMA design claimed in the commit message? [ ... ] > +static void qpace_destroy_ctx(struct zcomp_ctx *ctx) > +{ > + struct qpace_ctx *cc = ctx->context; > + > + if (!cc) > + return; > + > + dma_free_coherent(cc->dev, PAGE_SIZE, cc->out_buf, cc->out_dma); [Severity: High] If the device was unbound and qpace_priv was freed, could accessing cc->dev here trigger a use-after-free during cleanup? [ ... ] > +static int qpace_compress(struct zcomp_params *params, struct zcomp_ctx *ctx, > + struct zcomp_req *req) > +{ > + struct qpace_algorithm *algo = params->drv_data; > + struct qpace_ctx *cc = ctx->context; > + int ret; > + > + memcpy(cc->in_buf, req->src, req->src_len); [Severity: Medium] Does copying data to the bounce buffer here confirm the data path is not zero-copy? > + > + ret = qpace_urgent_compress(cc->in_dma, cc->out_dma, algo); > + > + if (ret == -E2BIG || ret >= PAGE_SIZE) { [Severity: High] Are hardware errors from the QPaCE compressor masked here? If qpace_urgent_compress() returns a negative error code (e.g., -EINVAL), comparing the signed integer 'ret' to PAGE_SIZE (typically unsigned long) promotes 'ret' to a large unsigned value. Would this cause the check to incorrectly pass, silently treating broken or failed compressions as incompressible data instead of propagating the failure? > + req->dst_len = PAGE_SIZE; > + return 0; > + } [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
