MIME-Version: 1.0
References: <20230209031159.2337445-1-ouyangweizhao@zeku.com>
 <CACT4Y+Zrz4KOU82jjEperYOM0sEp6TCmgse4XVMPkwAkS+dXrA@mail.gmail.com>
 <93b94f59016145adbb1e01311a1103f8@zeku.com> <CACT4Y+a=BaMNUf=_suQ5or9=ZksX2ht9gX8=XBSDEgHogyy3mg@mail.gmail.com>
 <CA+fCnZf3k-rsaOeti0Q7rqkmvsqDb2XxgxOq6V5Gqp6FGLH7Yg@mail.gmail.com>
 <b058a424e46d4f94a1f2fdc61292606b@zeku.com> <2b57491a9fab4ce9a643bd0922e03e73@zeku.com>
 <CA+fCnZcirNwdA=oaLLiDN+NxBPNcA75agPV1sRsKuZ0Wz6w_hQ@mail.gmail.com> <Y/4nJEHeUAEBsj6y@arm.com>
In-Reply-To: <Y/4nJEHeUAEBsj6y@arm.com>
From:   Andrey Konovalov <andreyknvl@gmail.com>
Date:   Tue, 28 Feb 2023 22:50:46 +0100
Message-ID: <CA+fCnZcFaOAGYic-x7848TMom2Rt5-Bm5SpYd-uxdT3im8PHvg@mail.gmail.com>
Subject: Re: [PATCH v2] kasan: fix deadlock in start_report()
To:     Catalin Marinas <catalin.marinas@arm.com>
Cc:     =?UTF-8?B?6KKB5biFKFNodWFpIFl1YW4p?= <yuanshuai@zeku.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        =?UTF-8?B?5qyn6Ziz54Kc6ZKKKFdlaXpoYW8gT3V5YW5nKQ==?= 
        <ouyangweizhao@zeku.com>, Andrey Ryabinin <ryabinin.a.a@gmail.com>,
        Alexander Potapenko <glider@google.com>,
        Vincenzo Frascino <vincenzo.frascino@arm.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "kasan-dev@googlegroups.com" <kasan-dev@googlegroups.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Weizhao Ouyang <o451686892@gmail.com>,
        =?UTF-8?B?5Lu756uL6bmPKFBlbmcgUmVuKQ==?= <renlipeng@zeku.com>,
        Peter Collingbourne <pcc@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Feb 28, 2023 at 5:09=E2=80=AFPM Catalin Marinas <catalin.marinas@ar=
m.com> wrote:
>
> On Mon, Feb 27, 2023 at 03:13:45AM +0100, Andrey Konovalov wrote:
> > +Catalin, would it be acceptable to implement a routine that disables
> > in-kernel MTE tag checking (until the next
> > mte_enable_kernel_sync/async/asymm call)? In a similar way an MTE
> > fault does this, but without the fault itself. I.e., expose the part
> > of do_tag_recovery functionality without report_tag_fault?
>
> I don't think we ever re-enable MTE after do_tag_recovery(). The
> mte_enable_kernel_*() are called at boot. We do call
> kasan_enable_tagging() explicitly in the kunit tests but that's a
> controlled fault environment.

Right, but here we don't want to re-enable MTE after a fault, we want
to suppress faults when printing an error report.

> IIUC, the problem is that the kernel already got an MTE fault, so at
> that point the error is not really recoverable.

No, the problem is with the following sequence of events:

1. KASAN detects a memory corruption and starts printing a report
_without getting an MTE fault_. This happens when e.g. KASAN sees a
free of an invalid address.

2. During error reporting, an MTE fault is triggered by the error
reporting code. E.g. while collecting information about the accessed
slab object.

3. KASAN tries to print another report while printing a report and
goes into a deadlock.

If we could avoid MTE faults being triggered during error reporting,
this would solve the problem.

> If we want to avoid a
> fault in the first place, we could do something like
> __uaccess_enable_tco() (Vincenzo has some patches to generalise these
> routines)

Ah, this looks exactly like what we need. Adding
__uaccess_en/disable_tco to kasan_report_invalid_free solves the
problem.

Do you think it would be possible to expose these routines to KASAN?

> but if an MTE fault already triggered and MTE is to stay
> disabled after the reporting anyway, I don't think it's worth it.

No MTE fault is triggered yet in the described sequence of events.

> So I wonder whether it's easier to just disable MTE before calling
> report_tag_fault() so that it won't trigger additional faults:

This will only help in case the first error report is caused by an MTE
fault. However, this won't help with the discussed problem: KASAN can
detect a memory corruption and print a report without getting an MTE
fault.

Nevertheless, this change makes sense to avoid a similar scenario
involving 2 MTE faults.