Re: [PATCH 4/6] zram: Add QPaCE zcomp backend
flat view
From: sashiko-bot@kernel.org
Date: 2026-09-30 15:08:33
Also in:
linux-devicetree
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ 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? [ ... ]
quoted hunk ↗ jump to hunk
+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? [ ... ]
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ + 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?
quoted hunk ↗ jump to hunk
+ req->dst_len = PAGE_SIZE; + return 0; + }
[ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/20260930145215.3380977-1-georgi.djakov@oss.qualcomm.com?part=4