[PATCH] mhi: Fix double dma free

[PATCH] mhi: Fix double dma free

[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

Re: [PATCH] mhi: Fix double dma free

[Loic Poulain]

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

Re: [PATCH] mhi: Fix double dma free

[Jeffrey Hugo]

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.

Re: [PATCH] mhi: Fix double dma free

[Loic Poulain]

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.

Re: [PATCH] mhi: Fix double dma free

[Kalle Valo]

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

Re: [PATCH] mhi: Fix double dma free

[Bhaumik Bhatt]

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

Re: [PATCH] mhi: Fix double dma free

[Loic Poulain]

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

Re: [PATCH] mhi: Fix double dma free

[Bhaumik Bhatt]

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

Re: [PATCH] mhi: Fix double dma free

[Manivannan Sadhasivam]

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

[PATCH] mhi: Fix double dma free

[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