linux-kernel - Re: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 16 Dec 2020 11:52:15 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Bhaskara Budiredla <bbudiredla@...vell.com>
Cc:     Kees Cook <keescook@...omium.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>,
        Sunil Kovvuri Goutham <sgoutham@...vell.com>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Christoph Hellwig <hch@....de>
Subject: Re: [EXT] Re: [PATCH 1/2] mmc: Support kmsg dumper based on pstore/blk

[...]

> >>
> >> Agree. Seems I need to create an alternate path to forcefully gain
> >> access to the host for the case of panic write. As you pointed out
> >> mmc_claim_host(), mmc_release_host() and runtime PM can create issues.
> >>
> >> >The question is, can/should we rely on mmc_claim_host() to succeed in
> >> >this path? Maybe it will work, in cases when there is no ongoing
> >> >request, as it means the host should be available to be immediately
> >> >claimed. Although, then the problem ends up with runtime PM, as if
> >> >the host is available for claiming, it's likely that the host is runtime
> >suspended...
> >> >
> >>
> >> An extra check can be added to see if host was runtime suspended ahead
> >> of panic write attempt.
> >
> >What if that is the case, should we just return an error?
> >
> Yes.
>
> >Moreover, even the device belonging to the mmc card can be runtime
> >suspended too. So if that is the case, we should return an error too?
> >
>
> Yes, same here.
>
> Assuming ->req_cleanup_pending() properly terminates the ongoing DMA transfers,
> mmc_claim_host() and mmc_release_host() can be dropped in panic write case
> as it has then exclusive access from then on.

Again, I think you have misunderstood how it works from the core point of view.

->req_cleanup_pending() will never be able to terminate a request in a
proper way, without involving the mmc core's knowledge about the
eMMC/SD protocol.

Yes, it could clean up from an ongoing DMA transfer, but that doesn't
bring the eMMC/SD card back into a state where it can accept new
requests.

>
>
> >[...]
> >
> >> >> >[...]
> >> >> >
> >> >> >Having said the above, I am not entirely convinced that it makes
> >> >> >sense to support this, at all.
> >> >> >
> >> >> >Not only, will the support be highly fragile from the mmc core
> >> >> >point of view, but there is also a significant complexity for an
> >> >> >mmc host driver to support this (at least in general).
> >> >> >
> >> >>
> >> >> I am not sure if the comments on host driver complexity is true.
> >> >> Terminating ongoing requests and introducing polling functions on
> >> >> host drivers should be straight forward. None those would disturb
> >> >> the core functionality. They are completely independent.
> >> >
> >> >I think you are underestimating the part with terminating ongoing
> >> >requests. It sounds to me that you really haven't been terminating
> >> >any requests at all, but rather just doing a reset of the mmc
> >> >controller (which is what I observed in patch2).
> >> >
> >>
> >> No, it's not true. I am not doing any reset. Please point me to
> >> specific code snippet where you have observed this.
> >
> >I was looking at patch2 and the ->req_cleanup_pending() callback that you
> >have assigned to cvm_req_cleanup_pending().
> >
> >In there you clear a potentially running DMA job, which is *kind* of a reset of
> >the controller. More importantly, it's definitely *not* terminating an ongoing
> >request, in a way that you can expect the eMMC/SD card to be ready for new
> >communications afterwards. This is my main point.
> >
>
> I am not sure that clearing an ongoing DMA will reset the controller. These are host
> controller specific. The idea is: To drop ongoing transfers, whatever a host software
> has to do it will does through this cleanup function. We may not generalize this,
> providing a hook and letting each host controller handling it seems better.

I fully understand the approach, but again, it doesn't work. Sorry.

As I said, the only thing that *could* work is to call
mmc_claim_host() in the panic write hook -  and hope for the best. If
it succeeds, there is nothing to cleanup or terminate.

Kind regards
Uffe