| Message ID | 20220811092145.1648008-1-fido_max@inbox.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/1] net: qrtr: start MHI channel after endpoit creation | expand |
On Thu, Aug 11, 2022 at 12:21:45PM +0300, Maxim Kochetkov wrote: > MHI channel may generates event/interrupt right after enabling. > It may leads to 2 race conditions issues. > > 1) > Such event may be dropped by qcom_mhi_qrtr_dl_callback() at check: > > if (!qdev || mhi_res->transaction_status) > return; > > Because dev_set_drvdata(&mhi_dev->dev, qdev) may be not performed at > this moment. In this situation qrtr-ns will be unable to enumerate > services in device. > --------------------------------------------------------------- > > 2) > Such event may come at the moment after dev_set_drvdata() and > before qrtr_endpoint_register(). In this case kernel will panic with > accessing wrong pointer at qcom_mhi_qrtr_dl_callback(): > > rc = qrtr_endpoint_post(&qdev->ep, mhi_res->buf_addr, > mhi_res->bytes_xferd); > > Because endpoint is not created yet. > -------------------------------------------------------------- > So move mhi_prepare_for_transfer_autoqueue after endpoint creation > to fix it. > > Fixes: a2e2cc0dbb11 ("net: qrtr: Start MHI channels during init") > Signed-off-by: Maxim Kochetkov <fido_max@inbox.ru> > Reviewed-by: Hemant Kumar <quic_hemantk@quicinc.com> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> > --- > net/qrtr/mhi.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c > index 18196e1c8c2f..17520d9e7a51 100644 > --- a/net/qrtr/mhi.c > +++ b/net/qrtr/mhi.c > @@ -78,11 +78,6 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, > struct qrtr_mhi_dev *qdev; > int rc; > > - /* start channels */ > - rc = mhi_prepare_for_transfer_autoqueue(mhi_dev); > - if (rc) > - return rc; > - > qdev = devm_kzalloc(&mhi_dev->dev, sizeof(*qdev), GFP_KERNEL); > if (!qdev) > return -ENOMEM; > @@ -96,6 +91,11 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, > if (rc) > return rc; > > + /* start channels */ > + rc = mhi_prepare_for_transfer_autoqueue(mhi_dev); > + if (rc) > + return rc; I missed the fact that you need to call qrtr_endpoint_unregister() in the error path. Please fix it. Thanks, Mani > + > dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n"); > > return 0; > -- > 2.34.1 >
diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c index 18196e1c8c2f..17520d9e7a51 100644 --- a/net/qrtr/mhi.c +++ b/net/qrtr/mhi.c @@ -78,11 +78,6 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, struct qrtr_mhi_dev *qdev; int rc; - /* start channels */ - rc = mhi_prepare_for_transfer_autoqueue(mhi_dev); - if (rc) - return rc; - qdev = devm_kzalloc(&mhi_dev->dev, sizeof(*qdev), GFP_KERNEL); if (!qdev) return -ENOMEM; @@ -96,6 +91,11 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, if (rc) return rc; + /* start channels */ + rc = mhi_prepare_for_transfer_autoqueue(mhi_dev); + if (rc) + return rc; + dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n"); return 0;
MHI channel may generates event/interrupt right after enabling. It may leads to 2 race conditions issues. 1) Such event may be dropped by qcom_mhi_qrtr_dl_callback() at check: if (!qdev || mhi_res->transaction_status) return; Because dev_set_drvdata(&mhi_dev->dev, qdev) may be not performed at this moment. In this situation qrtr-ns will be unable to enumerate services in device. --------------------------------------------------------------- 2) Such event may come at the moment after dev_set_drvdata() and before qrtr_endpoint_register(). In this case kernel will panic with accessing wrong pointer at qcom_mhi_qrtr_dl_callback(): rc = qrtr_endpoint_post(&qdev->ep, mhi_res->buf_addr, mhi_res->bytes_xferd); Because endpoint is not created yet. -------------------------------------------------------------- So move mhi_prepare_for_transfer_autoqueue after endpoint creation to fix it. Fixes: a2e2cc0dbb11 ("net: qrtr: Start MHI channels during init") Signed-off-by: Maxim Kochetkov <fido_max@inbox.ru> Reviewed-by: Hemant Kumar <quic_hemantk@quicinc.com> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> --- net/qrtr/mhi.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)