Date: Wed, 19 May 2021 15:02:08 -0600 From: Warner Losh <imp@bsdimp.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Mark Johnston <markj@freebsd.org>, src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@freebsd.org>" <dev-commits-src-all@freebsd.org>, dev-commits-src-main@freebsd.org Subject: Re: git: 0f206cc91279 - main - cam: add missing zeroing of a stack-allocated CCB. Message-ID: <CANCZdfo3PO45tTn_Z8uXzN6g5F%2BBDDxet9agGM7zvUScjdDn3Q@mail.gmail.com> In-Reply-To: <YKISvdE3jVAAVXUF@kib.kiev.ua> References: <202105161045.14GAjZIL093217@gitrepo.freebsd.org> <YKFOeDlJhm8sJHJX@nuc> <CANCZdfrPuWJeH4xK6Z0E1c2s6fvn5_tcwd0wN%2Bs7R-aKD61zqQ@mail.gmail.com> <YKFebPTnf1jCZtkD@brick> <YKISvdE3jVAAVXUF@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, May 17, 2021 at 12:52 AM Konstantin Belousov <kostikbel@gmail.com> wrote: > On Sun, May 16, 2021 at 07:03:24PM +0100, Edward Tomasz Napierala wrote: > > On 0516T1227, Warner Losh wrote: > > > On Sun, May 16, 2021, 11:55 AM Mark Johnston <markj@freebsd.org> > wrote: > > > > > > > On Sun, May 16, 2021 at 10:45:35AM +0000, Edward Tomasz Napierala > wrote: > > > > > The branch main has been updated by trasz: > > > > > > > > > > URL: > > > > > https://cgit.FreeBSD.org/src/commit/?id=0f206cc91279e630ad9e733eb6e330b7dbe6c70e > > > > > > > > > > commit 0f206cc91279e630ad9e733eb6e330b7dbe6c70e > > > > > Author: Edward Tomasz Napierala <trasz@FreeBSD.org> > > > > > AuthorDate: 2021-05-16 09:28:04 +0000 > > > > > Commit: Edward Tomasz Napierala <trasz@FreeBSD.org> > > > > > CommitDate: 2021-05-16 10:38:26 +0000 > > > > > > > > > > cam: add missing zeroing of a stack-allocated CCB. > > > > > > > > > > This could cause a panic at boot. > > > > > > > > There are other instances of this, for example syzbot is currently > > > > hitting an assertion, seemingly because the alloc_flags field of a > > > > stack-allocated CCB was not zeroed: > > > > https://syzkaller.appspot.com/bug?extid=2e9ce63919709feb3d1c > > > > > > > > I think the patch below will fix it, but I did not audit other > callers. > > > > It feels a bit strange to require all callers of xpt_setup_ccb() to > > > > manually zero the structure first, can we provide a single routine to > > > > initialize stack-allocated CCBs? > > > > We definitely could, although in some cases it's a bit more > > complicated than that - a function that gets passed a CCB and then > > calls xpt_setup_ccb() to fill it shouldn't zero it, as that would > > be making assumption on how the CCB passed to it was allocated. > > > > Now that I look at the code, I can definitely see that I've missed > > a couple of places. Perhaps I should replace those two KASSERTs with > > diagnostic printfs for now, until I get that sorted out? > > > > > If we did, we could set a flag we could assert on, and/or do static > > > analysis to find any others... > > > > That sounds promising, except I've never done anything like that; > > I don't even know where to start. > > Are stack-allocated ccbs passed around? In particular, can the thread > that allocated the ccb, put to sleep while ccb is processed elsewere? > This should break under swapping. > I don't think so, but we should check. Most of the ones I've spot checked are immediate operations that don't block. I've not checked every single one, though. This is another reason to use xpt_stack_ccb_inint() or similar. That way, we can flag this as 'stack allocated' and assert in xpt_setup_ccb when it is stack allocated and trying to do a queued operation. Sadly, traz reports that there's some tricky uses/reuses of ccbs that make this difficult to do at this time. Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfo3PO45tTn_Z8uXzN6g5F%2BBDDxet9agGM7zvUScjdDn3Q>