| Message ID | 20220811094840.1654088-1-fido_max@inbox.ru (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2,1/1] net: qrtr: start MHI channel after endpoit creation | expand |
On Thu, 11 Aug 2022 12:48:40 +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> You must CC the author of the patch under Fixes, they are usually the best person to review the fix. Adding Loic now. > diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c > index 18196e1c8c2f..9ced13c0627a 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,13 @@ 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) { > + qrtr_endpoint_unregister(&qdev->ep); > + return rc; > + } > + > dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n"); > > return 0;
On Sat, 13 Aug 2022 at 02:09, Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 11 Aug 2022 12:48:40 +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> Reviewed-by: Loic Poulain <loic.poulain@linaro.org> > > You must CC the author of the patch under Fixes, they are usually > the best person to review the fix. Adding Loic now. > > > diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c > > index 18196e1c8c2f..9ced13c0627a 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,13 @@ 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) { > > + qrtr_endpoint_unregister(&qdev->ep); > > + return rc; > > + } > > + > > dev_dbg(qdev->dev, "Qualcomm MHI QRTR driver probed\n"); > > > > return 0; >
Hello: This patch was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: On Thu, 11 Aug 2022 12:48:40 +0300 you 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; > > [...] Here is the summary with links: - [v2,1/1] net: qrtr: start MHI channel after endpoit creation https://git.kernel.org/netdev/net/c/68a838b84eff You are awesome, thank you!
diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c index 18196e1c8c2f..9ced13c0627a 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,13 @@ 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) { + qrtr_endpoint_unregister(&qdev->ep); + 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 | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)