| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFEp6-0iuJNDM9hdU3rWns=Vst6Ev1iyNim1ngRH3Z44CHwTAg@mail.gmail.com>
Date: Wed, 17 Dec 2025 21:55:01 +0100
From: Loic Poulain <loic.poulain@....qualcomm.com>
To: manivannan.sadhasivam@....qualcomm.com
Cc: Jeff Hugo <jeff.hugo@....qualcomm.com>,
Carl Vanderlip <carl.vanderlip@....qualcomm.com>,
Oded Gabbay <ogabbay@...nel.org>,
Manivannan Sadhasivam <mani@...nel.org>,
Jeff Johnson <jjohnson@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Maxim Kochetkov <fido_max@...ox.ru>, linux-arm-msm@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
mhi@...ts.linux.dev, linux-wireless@...r.kernel.org,
ath11k@...ts.infradead.org, ath12k@...ts.infradead.org,
netdev@...r.kernel.org, Bjorn Andersson <andersson@...nel.org>,
Johan Hovold <johan@...nel.org>, Chris Lew <quic_clew@...cinc.com>,
stable@...r.kernel.org
Subject: Re: [PATCH 1/2] net: qrtr: Drop the MHI auto_queue feature for IPCR
DL channels
Hi Mani,
On Wed, Dec 17, 2025 at 6:17 PM Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.oss.qualcomm.com@...nel.org> wrote:
>
> From: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
>
> MHI stack offers the 'auto_queue' feature, which allows the MHI stack to
> auto queue the buffers for the RX path (DL channel). Though this feature
> simplifies the client driver design, it introduces race between the client
> drivers and the MHI stack. For instance, with auto_queue, the 'dl_callback'
> for the DL channel may get called before the client driver is fully probed.
> This means, by the time the dl_callback gets called, the client driver's
> structures might not be initialized, leading to NULL ptr dereference.
>
> Currently, the drivers have to workaround this issue by initializing the
> internal structures before calling mhi_prepare_for_transfer_autoqueue().
> But even so, there is a chance that the client driver's internal code path
> may call the MHI queue APIs before mhi_prepare_for_transfer_autoqueue() is
> called, leading to similar NULL ptr dereference. This issue has been
> reported on the Qcom X1E80100 CRD machines affecting boot.
>
> So to properly fix all these races, drop the MHI 'auto_queue' feature
> altogether and let the client driver (QRTR) manage the RX buffers manually.
> In the QRTR driver, queue the RX buffers based on the ring length during
> probe and recycle the buffers in 'dl_callback' once they are consumed. This
> also warrants removing the setting of 'auto_queue' flag from controller
> drivers.
>
> Currently, this 'auto_queue' feature is only enabled for IPCR DL channel.
> So only the QRTR client driver requires the modification.
>
> Cc: stable@...r.kernel.org
> Fixes: 227fee5fc99e ("bus: mhi: core: Add an API for auto queueing buffers for DL channel")
> Fixes: 68a838b84eff ("net: qrtr: start MHI channel after endpoit creation")
> Reported-by: Johan Hovold <johan@...nel.org>
> Closes: https://lore.kernel.org/linux-arm-msm/ZyTtVdkCCES0lkl4@hovoldconsulting.com
> Suggested-by: Chris Lew <quic_clew@...cinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> ---
> drivers/accel/qaic/mhi_controller.c | 44 -----------------------
> drivers/bus/mhi/host/pci_generic.c | 20 ++---------
> drivers/net/wireless/ath/ath11k/mhi.c | 4 ---
> drivers/net/wireless/ath/ath12k/mhi.c | 4 ---
> net/qrtr/mhi.c | 67 +++++++++++++++++++++++++++++------
> 5 files changed, 58 insertions(+), 81 deletions(-)
[...]
> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
> index 69f53625a049..0b4d181ea747 100644
> --- a/net/qrtr/mhi.c
> +++ b/net/qrtr/mhi.c
> @@ -24,13 +24,25 @@ static void qcom_mhi_qrtr_dl_callback(struct mhi_device *mhi_dev,
> struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
> int rc;
>
> - if (!qdev || mhi_res->transaction_status)
> + if (!qdev || (mhi_res->transaction_status && mhi_res->transaction_status != -ENOTCONN))
> return;
>
> + /* Channel got reset. So just free the buffer */
> + if (mhi_res->transaction_status == -ENOTCONN) {
> + devm_kfree(&mhi_dev->dev, mhi_res->buf_addr);
> + return;
> + }
> +
> rc = qrtr_endpoint_post(&qdev->ep, mhi_res->buf_addr,
> mhi_res->bytes_xferd);
> if (rc == -EINVAL)
> dev_err(qdev->dev, "invalid ipcrouter packet\n");
> +
> + /* Done with the buffer, now recycle it for future use */
> + rc = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, mhi_res->buf_addr,
> + mhi_dev->mhi_cntrl->buffer_len, MHI_EOT);
> + if (rc)
> + dev_err(&mhi_dev->dev, "Failed to recycle the buffer: %d\n", rc);
> }
>
> /* From QRTR to MHI */
> @@ -72,6 +84,27 @@ static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
> return rc;
> }
>
> +static int qcom_mhi_qrtr_queue_dl_buffers(struct mhi_device *mhi_dev)
> +{
> + void *buf;
> + int ret;
> +
> + while (!mhi_queue_is_full(mhi_dev, DMA_FROM_DEVICE)) {
This approach might be a bit racy, since a buffer could complete
before the alloc+queue loop finishes. That could e.g lead to recycle
error in a concurrent DL callback. It might be simpler to just queue
the number of descriptors returned by mhi_get_free_desc_count().
> + buf = devm_kmalloc(&mhi_dev->dev, mhi_dev->mhi_cntrl->buffer_len, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = mhi_queue_buf(mhi_dev, DMA_FROM_DEVICE, buf, mhi_dev->mhi_cntrl->buffer_len,
> + MHI_EOT);
> + if (ret) {
> + dev_err(&mhi_dev->dev, "Failed to queue buffer: %d\n", ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
> const struct mhi_device_id *id)
> {
> @@ -87,20 +120,30 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
> qdev->ep.xmit = qcom_mhi_qrtr_send;
>
> dev_set_drvdata(&mhi_dev->dev, qdev);
> - rc = qrtr_endpoint_register(&qdev->ep, QRTR_EP_NID_AUTO);
> - if (rc)
> - return rc;
>
> /* start channels */
> - rc = mhi_prepare_for_transfer_autoqueue(mhi_dev);
> - if (rc) {
> - qrtr_endpoint_unregister(&qdev->ep);
> + rc = mhi_prepare_for_transfer(mhi_dev);
> + if (rc)
> return rc;
> - }
> +
> + rc = qrtr_endpoint_register(&qdev->ep, QRTR_EP_NID_AUTO);
> + if (rc)
> + goto err_unprepare;
> +
> + rc = qcom_mhi_qrtr_queue_dl_buffers(mhi_dev);
> + if (rc)
> + goto err_unregister;
>
> dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n");
>
> return 0;
> +
> +err_unregister:
> + qrtr_endpoint_unregister(&qdev->ep);
> +err_unprepare:
> + mhi_unprepare_from_transfer(mhi_dev);
> +
> + return rc;
> }
Regards,
Loic
Powered by blists - more mailing lists