Re: [PATCH 4/6] zram: Add QPaCE zcomp backend

[sashiko-bot@kernel.org]

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 <georgi.djakov@oss.qualcomm.com>

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/20260930145215.3380977-1-georgi.djakov@oss.qualcomm.com?part=4