linux-kernel - Re: [PATCH v2] Revert "swiotlb: panic if nslabs is too small"

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 1 Sep 2022 13:24:18 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Dongli Zhang <dongli.zhang@...cle.com>, Yu Zhao <yuzhao@...gle.com>
Cc:     Christoph Hellwig <hch@...radead.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        iommu@...ts.linux.dev, linux-mips@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        kernel test robot <lkp@...el.com>,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH v2] Revert "swiotlb: panic if nslabs is too small"

On 2022-09-01 03:18, Dongli Zhang wrote:
> Hi Yu,
> 
> On 8/31/22 5:24 PM, Yu Zhao wrote:
>> On Wed, Aug 31, 2022 at 4:20 PM Dongli Zhang <dongli.zhang@...cle.com> wrote:
>>>
>>> Hi Yu,
>>>
>>> As we discussed in the past, the swiotlb panic on purpose
>>
>> We should not panic() at all, especially on a platform that has been
>> working well since at least 4.14.
>>
>> Did you check out this link I previously shared with you?
>> https://urldefense.com/v3/__https://lore.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld*pnerc4J2Ag990WwAA@mail.gmail.com/__;Kw!!ACWV5N9M2RV99hQ!PXzSLurBv7VqxI1451TV4zO3_-BYj4grk-HYBsXzSnA6nZcXaBzdsQ-rF2DAqlICSRPMt-efYv_Uu2A2CQ$
> 
> Thanks for sharing! I used to read that in the past. To be honest I am still
> confused on when to use BUG/panic and when to not, as I still see many usage in
> some patches.
> 
> Just about swiotlb, it may panic in many cases during boot, e.g.,:
> 
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1955655

That's really a different thing, but frankly that panic is also bogus 
anyway - there is no good reason to have different behaviour for failing 
to allocate a buffer slot because the buffer is full vs. failing to 
allocate a buffer slot because there is no buffer. If we can fail 
gracefully some of the time we should fail gracefully all of the time. 
Yes, there's a slight difference in that one case has a chance of 
succeeding if retried in future while the other definitely doesn't, but 
it's not SWIOTLB's place to decide that the entire system is terminally 
unusable just because some device can't make a DMA mapping.

Similarly for the other panics at init time, which seem to have 
originated from a mechanical refactoring of the memblock API with the 
expectation of preserving the existing behaviour at the time. Those have 
then just been moved around without anyone thinking to question why 
they're there, and if memblock *does* now return usable error 
information, why don't we start handling that error properly like we do 
in other init paths?

Really there is no reason to panic anywhere in SWIOTLB. This is old 
code, things have moved on over the last 20 years, and we can and should 
do much better. I'll add this to my list of things to look at for 
cleanup once I find a bit of free time, unless anyone else fancies 
taking it on.

Thanks,
Robin.