* [PATCH] mhi: Fix double dma free
@ 2021-02-09 15:53 Loic Poulain
2021-02-09 15:55 ` Loic Poulain
2021-02-09 15:55 ` Jeffrey Hugo
0 siblings, 2 replies; 10+ messages in thread
From: Loic Poulain @ 2021-02-09 15:53 UTC (permalink / raw)
To: manivannan.sadhasivam, hemantk, kvalo; +Cc: linux-arm-msm, Loic Poulain
mhi_deinit_chan_ctxt functionthat takes care of unitializing channel
resources, including unmapping coherent MHI areas, can be called
from different path in case of controller unregistering/removal:
- From a client driver remove callback, via mhi_unprepare_channel
- From mhi_driver_remove that unitialize all channels
mhi_driver_remove()
|-> driver->remove()
| |-> mhi_unprepare_channel()
| |-> mhi_deinit_chan_ctxt()
|...
|-> mhi_deinit_chan_ctxt()
This leads to double dma freeing...
Fix that by preventing deinit for already uninitialized channel.
Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
Reported-by: Kalle Valo <kvalo@codeaurora.org>
---
drivers/bus/mhi/core/init.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index aa575d3..be4eebb 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -557,6 +557,9 @@ void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl,
tre_ring = &mhi_chan->tre_ring;
chan_ctxt = &mhi_cntrl->mhi_ctxt->chan_ctxt[mhi_chan->chan];
+ if (!chan_ctxt->rbase) /* Already uninitialized */
+ return;
+
mhi_free_coherent(mhi_cntrl, tre_ring->alloc_size,
tre_ring->pre_aligned, tre_ring->dma_handle);
vfree(buf_ring->base);
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] mhi: Fix double dma free
2021-02-09 15:53 [PATCH] mhi: Fix double dma free Loic Poulain
@ 2021-02-09 15:55 ` Loic Poulain
2021-02-09 17:02 ` Kalle Valo
2021-02-09 15:55 ` Jeffrey Hugo
1 sibling, 1 reply; 10+ messages in thread
From: Loic Poulain @ 2021-02-09 15:55 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-arm-msm, Hemant Kumar, Manivannan Sadhasivam
Hi Kalle,
On Tue, 9 Feb 2021 at 16:45, Loic Poulain <loic.poulain@linaro.org> wrote:
>
> mhi_deinit_chan_ctxt functionthat takes care of unitializing channel
> resources, including unmapping coherent MHI areas, can be called
> from different path in case of controller unregistering/removal:
> - From a client driver remove callback, via mhi_unprepare_channel
> - From mhi_driver_remove that unitialize all channels
>
> mhi_driver_remove()
> |-> driver->remove()
> | |-> mhi_unprepare_channel()
> | |-> mhi_deinit_chan_ctxt()
> |...
> |-> mhi_deinit_chan_ctxt()
>
> This leads to double dma freeing...
>
> Fix that by preventing deinit for already uninitialized channel.
>
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> Reported-by: Kalle Valo <kvalo@codeaurora.org>
This is a 'blind' fix tentative, can you please check if it resolves
the issue on your side?
Thanks,
Loic
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mhi: Fix double dma free
2021-02-09 15:55 ` Loic Poulain
@ 2021-02-09 17:02 ` Kalle Valo
0 siblings, 0 replies; 10+ messages in thread
From: Kalle Valo @ 2021-02-09 17:02 UTC (permalink / raw)
To: Loic Poulain; +Cc: linux-arm-msm, Hemant Kumar, Manivannan Sadhasivam
Loic Poulain <loic.poulain@linaro.org> writes:
> Hi Kalle,
>
> On Tue, 9 Feb 2021 at 16:45, Loic Poulain <loic.poulain@linaro.org> wrote:
>>
>> mhi_deinit_chan_ctxt functionthat takes care of unitializing channel
>> resources, including unmapping coherent MHI areas, can be called
>> from different path in case of controller unregistering/removal:
>> - From a client driver remove callback, via mhi_unprepare_channel
>> - From mhi_driver_remove that unitialize all channels
>>
>> mhi_driver_remove()
>> |-> driver->remove()
>> | |-> mhi_unprepare_channel()
>> | |-> mhi_deinit_chan_ctxt()
>> |...
>> |-> mhi_deinit_chan_ctxt()
>>
>> This leads to double dma freeing...
>>
>> Fix that by preventing deinit for already uninitialized channel.
>>
>> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
>> Reported-by: Kalle Valo <kvalo@codeaurora.org>
>
> This is a 'blind' fix tentative, can you please check if it resolves
> the issue on your side?
I did a quick test few times and I don't see any crashes anymore, thanks!
Tested-by: Kalle Valo <kvalo@codeaurora.org>
And like Jeffrey said:
Fixes: a7f422f2f89e ("bus: mhi: Fix channel close issue on driver remove")
Is it too late to push this to v5.11? But this should go to v5.12.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mhi: Fix double dma free
2021-02-09 15:53 [PATCH] mhi: Fix double dma free Loic Poulain
2021-02-09 15:55 ` Loic Poulain
@ 2021-02-09 15:55 ` Jeffrey Hugo
2021-02-09 16:06 ` Loic Poulain
1 sibling, 1 reply; 10+ messages in thread
From: Jeffrey Hugo @ 2021-02-09 15:55 UTC (permalink / raw)
To: Loic Poulain, manivannan.sadhasivam, hemantk, kvalo; +Cc: linux-arm-msm
On 2/9/2021 8:53 AM, Loic Poulain wrote:
> mhi_deinit_chan_ctxt functionthat takes care of unitializing channel
> resources, including unmapping coherent MHI areas, can be called
> from different path in case of controller unregistering/removal:
> - From a client driver remove callback, via mhi_unprepare_channel
> - From mhi_driver_remove that unitialize all channels
>
> mhi_driver_remove()
> |-> driver->remove()
> | |-> mhi_unprepare_channel()
> | |-> mhi_deinit_chan_ctxt()
> |...
> |-> mhi_deinit_chan_ctxt()
>
> This leads to double dma freeing...
>
> Fix that by preventing deinit for already uninitialized channel.
>
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> Reported-by: Kalle Valo <kvalo@codeaurora.org>
> ---
Seems like this should have a Fixes: tag, no?
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mhi: Fix double dma free
2021-02-09 15:55 ` Jeffrey Hugo
@ 2021-02-09 16:06 ` Loic Poulain
2021-02-09 17:27 ` Bhaumik Bhatt
0 siblings, 1 reply; 10+ messages in thread
From: Loic Poulain @ 2021-02-09 16:06 UTC (permalink / raw)
To: Jeffrey Hugo
Cc: Manivannan Sadhasivam, Hemant Kumar, Kalle Valo, linux-arm-msm
On Tue, 9 Feb 2021 at 16:55, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>
> On 2/9/2021 8:53 AM, Loic Poulain wrote:
> > mhi_deinit_chan_ctxt functionthat takes care of unitializing channel
> > resources, including unmapping coherent MHI areas, can be called
> > from different path in case of controller unregistering/removal:
> > - From a client driver remove callback, via mhi_unprepare_channel
> > - From mhi_driver_remove that unitialize all channels
> >
> > mhi_driver_remove()
> > |-> driver->remove()
> > | |-> mhi_unprepare_channel()
> > | |-> mhi_deinit_chan_ctxt()
> > |...
> > |-> mhi_deinit_chan_ctxt()
> >
> > This leads to double dma freeing...
> >
> > Fix that by preventing deinit for already uninitialized channel.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > Reported-by: Kalle Valo <kvalo@codeaurora.org>
> > ---
>
> Seems like this should have a Fixes: tag, no?
Right, thanks, i'll add it in V2 once I get feedback.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mhi: Fix double dma free
2021-02-09 16:06 ` Loic Poulain
@ 2021-02-09 17:27 ` Bhaumik Bhatt
2021-02-09 18:17 ` Loic Poulain
0 siblings, 1 reply; 10+ messages in thread
From: Bhaumik Bhatt @ 2021-02-09 17:27 UTC (permalink / raw)
To: Loic Poulain, Manivannan Sadhasivam
Cc: Jeffrey Hugo, Hemant Kumar, Kalle Valo, linux-arm-msm
On 2021-02-09 08:06 AM, Loic Poulain wrote:
> On Tue, 9 Feb 2021 at 16:55, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>>
>> On 2/9/2021 8:53 AM, Loic Poulain wrote:
>> > mhi_deinit_chan_ctxt functionthat takes care of unitializing channel
>> > resources, including unmapping coherent MHI areas, can be called
>> > from different path in case of controller unregistering/removal:
>> > - From a client driver remove callback, via mhi_unprepare_channel
>> > - From mhi_driver_remove that unitialize all channels
>> >
>> > mhi_driver_remove()
>> > |-> driver->remove()
>> > | |-> mhi_unprepare_channel()
>> > | |-> mhi_deinit_chan_ctxt()
>> > |...
>> > |-> mhi_deinit_chan_ctxt()
>> >
>> > This leads to double dma freeing...
>> >
>> > Fix that by preventing deinit for already uninitialized channel.
>> >
>> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
>> > Reported-by: Kalle Valo <kvalo@codeaurora.org>
>> > ---
>>
>> Seems like this should have a Fixes: tag, no?
>
> Right, thanks, i'll add it in V2 once I get feedback.
Hi Loic, Mani,
I saw this same issue a while back but could not collect the logs for
it.
I had already pushed a patch to fix this differently [1] which was
recently reviewed by Hemant.
Although there wasn't a purposeful fixes tag for it. I think the culprit
for this issue is [2]:
As it allows the unprepare to go through on remove(), which was
traditionally not allowed and
ends up uncovering this issue as it fixes another.
Channel updates [3] address that and provide a bunch of other
improvements. Please consider them.
Thanks,
Bhaumik
[1] https://lkml.org/lkml/2021/2/4/1161
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/bus/mhi?h=v5.11-rc7&id=a7f422f2f89e7d48aa66e6488444a4c7f01269d5
[3] https://lkml.org/lkml/2021/2/4/1155
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mhi: Fix double dma free
2021-02-09 17:27 ` Bhaumik Bhatt
@ 2021-02-09 18:17 ` Loic Poulain
2021-02-10 4:37 ` Bhaumik Bhatt
0 siblings, 1 reply; 10+ messages in thread
From: Loic Poulain @ 2021-02-09 18:17 UTC (permalink / raw)
To: Bhaumik Bhatt
Cc: Manivannan Sadhasivam, Jeffrey Hugo, Hemant Kumar, Kalle Valo,
linux-arm-msm
Hi Bhaumik,
On Tue, 9 Feb 2021 at 18:27, Bhaumik Bhatt <bbhatt@codeaurora.org> wrote:
>
> On 2021-02-09 08:06 AM, Loic Poulain wrote:
> > On Tue, 9 Feb 2021 at 16:55, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> >>
> >> On 2/9/2021 8:53 AM, Loic Poulain wrote:
> >> > mhi_deinit_chan_ctxt functionthat takes care of unitializing channel
> >> > resources, including unmapping coherent MHI areas, can be called
> >> > from different path in case of controller unregistering/removal:
> >> > - From a client driver remove callback, via mhi_unprepare_channel
> >> > - From mhi_driver_remove that unitialize all channels
> >> >
> >> > mhi_driver_remove()
> >> > |-> driver->remove()
> >> > | |-> mhi_unprepare_channel()
> >> > | |-> mhi_deinit_chan_ctxt()
> >> > |...
> >> > |-> mhi_deinit_chan_ctxt()
> >> >
> >> > This leads to double dma freeing...
> >> >
> >> > Fix that by preventing deinit for already uninitialized channel.
> >> >
> >> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> >> > Reported-by: Kalle Valo <kvalo@codeaurora.org>
> >> > ---
> >>
> >> Seems like this should have a Fixes: tag, no?
> >
> > Right, thanks, i'll add it in V2 once I get feedback.
>
> Hi Loic, Mani,
>
> I saw this same issue a while back but could not collect the logs for
> it.
>
> I had already pushed a patch to fix this differently [1] which was
> recently reviewed by Hemant.
>
> Although there wasn't a purposeful fixes tag for it. I think the culprit
> for this issue is [2]:
>
> As it allows the unprepare to go through on remove(), which was
> traditionally not allowed and
> ends up uncovering this issue as it fixes another.
>
> Channel updates [3] address that and provide a bunch of other
> improvements. Please consider them.
Yes, patch [2] is the culprit. I would recommend merging this tiny fix
so that it can be easily grab for 5.11 or backported, and keep your
series (rebased on top), for mhi-next (going to review/test it btw).
Regards,
Loic
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mhi: Fix double dma free
2021-02-09 18:17 ` Loic Poulain
@ 2021-02-10 4:37 ` Bhaumik Bhatt
2021-02-10 8:17 ` Manivannan Sadhasivam
0 siblings, 1 reply; 10+ messages in thread
From: Bhaumik Bhatt @ 2021-02-10 4:37 UTC (permalink / raw)
To: Loic Poulain, Manivannan Sadhasivam, Hemant Kumar
Cc: Jeffrey Hugo, Kalle Valo, linux-arm-msm
Hi Loic, Mani, Hemant,
On 2021-02-09 10:17 AM, Loic Poulain wrote:
> Hi Bhaumik,
>
> On Tue, 9 Feb 2021 at 18:27, Bhaumik Bhatt <bbhatt@codeaurora.org>
> wrote:
>>
>> On 2021-02-09 08:06 AM, Loic Poulain wrote:
>> > On Tue, 9 Feb 2021 at 16:55, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>> >>
>> >> On 2/9/2021 8:53 AM, Loic Poulain wrote:
>> >> > mhi_deinit_chan_ctxt functionthat takes care of unitializing channel
>> >> > resources, including unmapping coherent MHI areas, can be called
>> >> > from different path in case of controller unregistering/removal:
>> >> > - From a client driver remove callback, via mhi_unprepare_channel
>> >> > - From mhi_driver_remove that unitialize all channels
>> >> >
>> >> > mhi_driver_remove()
>> >> > |-> driver->remove()
>> >> > | |-> mhi_unprepare_channel()
>> >> > | |-> mhi_deinit_chan_ctxt()
>> >> > |...
>> >> > |-> mhi_deinit_chan_ctxt()
>> >> >
>> >> > This leads to double dma freeing...
>> >> >
>> >> > Fix that by preventing deinit for already uninitialized channel.
>> >> >
>> >> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
>> >> > Reported-by: Kalle Valo <kvalo@codeaurora.org>
>> >> > ---
>> >>
>> >> Seems like this should have a Fixes: tag, no?
>> >
>> > Right, thanks, i'll add it in V2 once I get feedback.
>>
>> Hi Loic, Mani,
>>
>> I saw this same issue a while back but could not collect the logs for
>> it.
>>
>> I had already pushed a patch to fix this differently [1] which was
>> recently reviewed by Hemant.
>>
>> Although there wasn't a purposeful fixes tag for it. I think the
>> culprit
>> for this issue is [2]:
>>
>> As it allows the unprepare to go through on remove(), which was
>> traditionally not allowed and
>> ends up uncovering this issue as it fixes another.
>>
>> Channel updates [3] address that and provide a bunch of other
>> improvements. Please consider them.
>
> Yes, patch [2] is the culprit. I would recommend merging this tiny fix
> so that it can be easily grab for 5.11 or backported, and keep your
> series (rebased on top), for mhi-next (going to review/test it btw).
>
> Regards,
> Loic
If priority is to get this fix in ASAP, your suggestion is OK. I just
see some
typo fixes and a title update to "bus: mhi: core: Fix double dma free()
call"
or something as review comments for your patch.
Another option is that I can send my patch [1] separately and remove it
from my
"channel updates" patch series, if that helps.
I'd like to see what Mani and Hemant on what they prefer. Please advise.
Thanks,
Bhaumik
[1] https://lkml.org/lkml/2021/2/4/1161
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mhi: Fix double dma free
2021-02-10 4:37 ` Bhaumik Bhatt
@ 2021-02-10 8:17 ` Manivannan Sadhasivam
0 siblings, 0 replies; 10+ messages in thread
From: Manivannan Sadhasivam @ 2021-02-10 8:17 UTC (permalink / raw)
To: Bhaumik Bhatt
Cc: Loic Poulain, Hemant Kumar, Jeffrey Hugo, Kalle Valo, linux-arm-msm
On Tue, Feb 09, 2021 at 08:37:21PM -0800, Bhaumik Bhatt wrote:
> Hi Loic, Mani, Hemant,
>
[...]
> If priority is to get this fix in ASAP, your suggestion is OK. I just see
> some
> typo fixes and a title update to "bus: mhi: core: Fix double dma free()
> call"
> or something as review comments for your patch.
>
> Another option is that I can send my patch [1] separately and remove it from
> my
> "channel updates" patch series, if that helps.
>
> I'd like to see what Mani and Hemant on what they prefer. Please advise.
>
Both patches looks good to me although your patch needs to be resubmitted. But
since the fix needs to go in quickly, I'm going to apply Loic's patch.
Thanks,
Mani
> Thanks,
> Bhaumik
>
> [1] https://lkml.org/lkml/2021/2/4/1161
>
> ---
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] mhi: Fix double dma free
@ 2021-02-09 15:52 Loic Poulain
0 siblings, 0 replies; 10+ messages in thread
From: Loic Poulain @ 2021-02-09 15:52 UTC (permalink / raw)
To: manivannan.sadhasivam, hemantk; +Cc: linux-arm-msm, Loic Poulain
mhi_deinit_chan_ctxt functionthat takes care of unitializing channel
resources, including unmapping coherent MHI areas, can be called
from different path in case of controller unregistering/removal:
- From a client driver remove callback, via mhi_unprepare_channel
- From mhi_driver_remove that unitialize all channels
mhi_driver_remove()
|-> driver->remove()
| |-> mhi_unprepare_channel()
| |-> mhi_deinit_chan_ctxt()
|...
|-> mhi_deinit_chan_ctxt()
This leads to double dma freeing...
Fix that by preventing deinit for already uninitialized channel.
Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
Reported-by: Kalle Valo <kvalo@codeaurora.org>
---
drivers/bus/mhi/core/init.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index aa575d3..be4eebb 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -557,6 +557,9 @@ void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl,
tre_ring = &mhi_chan->tre_ring;
chan_ctxt = &mhi_cntrl->mhi_ctxt->chan_ctxt[mhi_chan->chan];
+ if (!chan_ctxt->rbase) /* Already uninitialized */
+ return;
+
mhi_free_coherent(mhi_cntrl, tre_ring->alloc_size,
tre_ring->pre_aligned, tre_ring->dma_handle);
vfree(buf_ring->base);
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-02-10 8:17 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-09 15:53 [PATCH] mhi: Fix double dma free Loic Poulain
2021-02-09 15:55 ` Loic Poulain
2021-02-09 17:02 ` Kalle Valo
2021-02-09 15:55 ` Jeffrey Hugo
2021-02-09 16:06 ` Loic Poulain
2021-02-09 17:27 ` Bhaumik Bhatt
2021-02-09 18:17 ` Loic Poulain
2021-02-10 4:37 ` Bhaumik Bhatt
2021-02-10 8:17 ` Manivannan Sadhasivam
-- strict thread matches above, loose matches on Subject: below --
2021-02-09 15:52 Loic Poulain
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).