Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider: - [High] soc: qcom: qpace: concurrent MMIO access from CPU aliasing on >8 cores - [High] soc: qcom: qpace: UAF of global qpace_priv after driver unbind - [High] soc: qcom: qpace: 100ms busy wait with preemption disabled causes latency spikes - [High] soc: qcom: qpace: interrupt storm due to uncleared level-triggered IRQ - [High] soc: qcom: qpace: stray DMA writes on urgent command timeout - [High] soc: qcom: qpace: hardware stuck in STOP mode after suspend/resume - [Medium] soc: qcom: qpace: silent truncation of unaligned output_addr causes DMA corruption - [Medium] soc: qcom: qpace: inline stp assembly breaks big-endian architectures --
commit 5edbc123f3ebd977977e7d874e4ecebc5ed95e6a Author: Georgi Djakov <[email protected]> soc: qcom: qpace: Add Qualcomm Page Compression Engine driver This commit adds a platform driver for the Qualcomm Page Compression Engine (QPaCE). It provides the urgent command path for synchronous single-page compression and decompression, supporting low-latency operations for compressed-memory users like zram. > diff --git a/drivers/soc/qcom/qpace.c b/drivers/soc/qcom/qpace.c > index 0000000000000..69aa14b7c4772 > --- /dev/null > +++ b/drivers/soc/qcom/qpace.c [ ... ] > +static inline int qpace_urgent_command_trigger(dma_addr_t input_addr, > + dma_addr_t output_addr, > + int urg_reg_num, > + enum urg_reg_cxts command) > +{ > + void __iomem *td_dst_src_reg = qpace_priv->urg_regs + > + (urg_reg_num * QPACE_REG_PAGE_SIZE) + > + > QPACE_URG_CMD_0_TD_DST_ADDR_L_CFG_CNTXT_OFFSET; > + u64 urg_addr_field_lower, urg_addr_field_upper; > + u32 stat_reg; > + unsigned long ret; > + > + urg_addr_field_lower = > FIELD_PREP(URG_CMD_0_TD_DST_ADDR_L__CMD_CFG_CNTXT, > + command); > + urg_addr_field_lower |= GENMASK(63, 8) & output_addr; [Severity: Medium] Will this silently truncate the output_addr if the caller passes an address that is not 256-byte aligned? In qpace_urgent_command_trigger(), this drops the lowest 8 bits, meaning the hardware will write to the incorrectly truncated address and overwrite up to 255 bytes of memory before the intended buffer. > + > + urg_addr_field_upper = input_addr; > + > + /* Clear a stale request-on-active error before ringing the doorbell. */ > + qpace_write_urg_cmd(qpace_priv, urg_reg_num, > QPACE_URG_CMD_0_STAT_CLR_OFFSET, > + URG_CMD_0_STAT_CLR_REQ_ON_ACTIVE_ERR); > + > + /* Ensure that preceding stores that QPaCE will depend on are done > executing */ > + dma_wmb(); > + > + /* > + * Ring the doorbell with a single 128-bit atomic store. QPaCE > + * triggers processing when it observes the (dst_addr, src_addr) > + * pair land together. > + */ > + asm volatile("stp %0, %1, [%2]\n" : > + : "r" (urg_addr_field_lower), "r" (urg_addr_field_upper), > "r" (td_dst_src_reg) > + : "memory"); [Severity: Medium] Does this inline assembly break big-endian architectures? By bypassing the standard writeq() accessors, this stp instruction skips implicit cpu_to_le64 conversions, causing the hardware to receive byte-swapped, garbage memory addresses and context configurations if the kernel is compiled with CONFIG_CPU_BIG_ENDIAN. > + > + ret = readl_poll_timeout_atomic(qpace_priv->urg_regs + > + urg_reg_num * QPACE_REG_PAGE_SIZE + > + QPACE_URG_CMD_0_ED_STAT_OFFSET, > + stat_reg, > + FIELD_GET(URG_CMD_0_ED_STAT_COMP_CODE, > + stat_reg) != OP_URG_ONGOING, > + 1, 100 * USEC_PER_MSEC); [Severity: High] Does this cause severe scheduling latency spikes or RCU stalls? The caller, such as qpace_urgent_compress(), disables preemption via get_cpu() before initiating this hardware poll. Executing a 100ms busy-wait loop in qpace_urgent_command_trigger() while preemption is completely disabled can freeze CPU cores for unacceptable durations under load. > + if (ret) { > + dev_err(qpace_priv->dev, "QPaCE urgent command timed out\n"); > + return -ETIMEDOUT; > + } [Severity: High] Are we returning a timeout error here without stopping the hardware from continuing the DMA operation? If the hardware takes longer than 100ms in qpace_urgent_command_trigger(), this returns -ETIMEDOUT while the device continues. If the caller (like zram) frees the output page, the hardware could later finish and cause delayed asynchronous memory corruption. > + > + return stat_reg; > +} > + > +int qpace_urgent_compress(dma_addr_t input_addr, > + dma_addr_t output_addr, > + struct qpace_algorithm *algo) > +{ > + int urg_reg_num; > + int stat_reg; > + u32 stat_reg_val; > + int ret; > + > + ret = qpace_get(); > + if (ret) > + return ret; > + > + urg_reg_num = get_cpu() % NUM_TRS_ERS_URG_CMD_REGS; > + stat_reg = qpace_urgent_command_trigger(input_addr, output_addr, > urg_reg_num, > + algo->urg_comp_cntxt); [Severity: High] Can this assign identical hardware registers to different CPUs running concurrently when NR_CPUS is greater than 8 without cross-CPU mutual exclusion? In qpace_urgent_compress(), get_cpu() disables preemption but provides no mutual exclusion across different cores. If two threads initiate compression on CPU 0 and CPU 8 simultaneously, they will both be assigned urg_reg_num = 0 and race on the same MMIO block, leading to data corruption and hardware timeouts. > + put_cpu(); > + > + qpace_put(); [ ... ] > +int qpace_urgent_decompress(dma_addr_t input_addr, > + dma_addr_t output_addr, > + size_t input_size, > + struct qpace_algorithm *algo) > +{ > + int urg_reg_num; > + int stat_reg; > + u32 stat_reg_val; > + int ret; > + > + ret = qpace_get(); > + if (ret) > + goto out; > + > + urg_reg_num = get_cpu() % NUM_TRS_ERS_URG_CMD_REGS; > + qpace_write_urg_cmd_ctx(qpace_priv, > QPACE_URG_CMD_0_CFG_CNTXT_SIZE_n_OFFSET, > + urg_reg_num, algo->urg_decomp_cntxt, > + FIELD_PREP(URG_CMD_0_CFG_CNTXT_SIZE_SIZE, > input_size)); [Severity: High] Does this dynamically overwrite the shared hardware context size register concurrently across aliasing CPUs? In qpace_urgent_decompress(), with more than 8 cores, aliasing CPUs will race on this MMIO block leading to data corruption. > + stat_reg = qpace_urgent_command_trigger(input_addr, output_addr, > urg_reg_num, > + algo->urg_decomp_cntxt); > + put_cpu(); [ ... ] > +int qpace_get(void) > +{ > + int ret = 0; > + > + mutex_lock(&qpace_ref_lock); > + if (qpace_priv->suspended || READ_ONCE(qpace_priv->broken)) > + ret = -EBUSY; [Severity: High] Can this exported API blindly dereference the global pointer after the driver has been unbound? If the device is unbound via sysfs, qpace_priv is freed, but the global pointer remains set. A subsequent call from a client like zram will call qpace_get() which dereferences qpace_priv->suspended, resulting in a use-after-free. > + else > + _get_qpace(); > + mutex_unlock(&qpace_ref_lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(qpace_get); [ ... ] > +static irqreturn_t urgent_interrupt_handler(int irq, void *unused) > +{ > + pr_debug("Urgent interrupt handled\n"); > + return IRQ_HANDLED; > +} [Severity: High] Will this cause a catastrophic interrupt storm? The urgent_interrupt_handler() fails to acknowledge or clear the interrupt at the device level. Because the GIC interrupt is level-triggered, returning IRQ_HANDLED without clearing it on the hardware will cause it to immediately re-fire forever, resulting in a hard CPU lockup if the hardware ever asserts this line. [ ... ] > +static void qpace_remove(struct platform_device *pdev) > +{ > + mutex_lock(&qpace_ref_lock); > + qpace_priv->suspended = true; > + mutex_unlock(&qpace_ref_lock); > + > + static_branch_disable(&qpace_drv_probed); > + > + wait_for_completion(&qpace_priv->no_active_refs); > + > + /* No callers remain; tear down the hardware. */ > + cancel_work_sync(&qpace_priv->disable_work); > + qpace_power_off(&pdev->dev); > +} [Severity: High] Does this leave the global qpace_priv pointer set? In qpace_remove(), the device is torn down and devres will free the memory backing qpace_priv, but the global pointer is never cleared, enabling the use-after-free mentioned above. [ ... ] > +static int qpace_resume(struct device *dev) > +{ > + int ret; > + > + mutex_lock(&qpace_ref_lock); > + > + if (qpace_priv->active_rings) > + dev_err(dev, "active_rings not 0, unexpected case\n"); > + > + ret = icc_set_bw(qpace_priv->interconnect, 0, 1); > + if (ret) > + goto out_unlock; > + > + program_urg_command_contexts_v2(); > + program_decomp_core_cfg(); > + > + qpace_priv->suspended = false; [Severity: High] Will the hardware be stuck in STOP mode after suspend/resume? In qpace_resume(), we restore contexts but fail to wake the hardware engine up to RUN mode (omitting the QPACE_RUN write) and fail to reprogram essential bulk mode and cache index settings from qpace_hw_init(). This means all hardware operations will indefinitely time out after the system resumes. > + > +out_unlock: > + if (ret) > + dev_err(dev, "failed to resume QPaCE: %d\n", ret); > + mutex_unlock(&qpace_ref_lock); > + return ret; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
