Date: Thu, 11 Mar 2021 22:07:11 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: Kyle Evans <kevans@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 1ae20f7c70ea - main - kern: malloc: fix panic on M_WAITOK during THREAD_NO_SLEEPING() Message-ID: <CAGudoHHOa3-D%2Bi7mGwjc4GNQQ2UOaUwny%2BqxpYaTPy5tCAr3mA@mail.gmail.com> In-Reply-To: <8e37c710-bd9d-6fe0-0263-4efeabfd9beb@FreeBSD.org> References: <202103091117.129BHOZa042851@gitrepo.freebsd.org> <CAGudoHFbEt1%2BmSDx04x1-YrxwrxHMdfoRj69EV183ngZ4TkxkA@mail.gmail.com> <8e37c710-bd9d-6fe0-0263-4efeabfd9beb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 3/11/21, John Baldwin <jhb@freebsd.org> wrote:
> On 3/10/21 3:57 AM, Mateusz Guzik wrote:
>> There is something very wrong going on here.
>>
>> Key thing to note is that malloc is ultimately a wrapper around
>> uma_zalloc. Whatever asserts you may want to add to malloc to catch
>> problems sooner, should also present in uma.
>>
>> uma has the following:
>>
>> if (flags & M_WAITOK) {
>> WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL,
>> "uma_zalloc_debug: zone \"%s\"", zone->uz_name);
>> }
>
> This warns about holding locks, not about TD_NO_SLEEPING. Witness' role
> is to check lock orders and warn about invalid operations when holding
> certain locks. It does not currently verify anything about other
> inhibitions
> like TD_NO_SLEEPING. See, e.g. the list of assertions in userret() which
> includes a WITNESS_WARN in addition to several other checks (including
> TD_NO_SLEEPING). Arguably we should just move the TD_NO_SLEEPING check
> from malloc() to here along with the td_intr_nesting_level. Any case
> where you can't use malloc() with M_WAITOK you can't use uma with M_WAITOK
> either.
>
My point is that the witness check is clearly deficient even ignoring
uma -- there are other places which do this and fail to properly check
if going off cpu is allowed. In fact, looks like the TD_NO_SLEEPING
stuff is already not properly checked for when taking sleepable locks.
>> This code used to execute prior to this commit and fail to catch the
>> problems which got reported already.
>>
>> In other words:
>> - WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK is incomplete in terms of
>> internals its internals
>> - the above should be in malloc, perhaps after being abstracted into a
>> an assert-helper
>> - fixing witness deficiency will probably find a slew of new problems
>> - this should remain as a warning (maybe rate-limited) for the foreseable
>> future
>
> I don't know that we've had so many issues that we can't just fix them on
> HEAD right now vs having to make this a warning instead of keeping it as
> a panic.
>
> --
> John Baldwin
>
--
Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHHOa3-D%2Bi7mGwjc4GNQQ2UOaUwny%2BqxpYaTPy5tCAr3mA>