From owner-dev-commits-src-main@freebsd.org Wed May 19 21:02:23 2021 Return-Path: Delivered-To: dev-commits-src-main@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id EFA0164BE65 for ; Wed, 19 May 2021 21:02:23 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qk1-x72c.google.com (mail-qk1-x72c.google.com [IPv6:2607:f8b0:4864:20::72c]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4FllgJ66Fcz3wCp for ; Wed, 19 May 2021 21:02:20 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qk1-x72c.google.com with SMTP id o27so14149668qkj.9 for ; Wed, 19 May 2021 14:02:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=bnLDBG9O91+WOP5nPd+HzZaTii7mLDnN+gkiD+kh784=; b=BfzlTQ5EwP+BZm1iTzwJmNWgX7gFH0UX49fp1HskixLfEfiG20+hlH0b1+jyAAMcVV 3mOfceLhpNAylpCDZ6/mOk3/MC34DbIYhpiM6Rsfk3+BOg+HSFhdIkOshoKtkuqW+I+h qCQ8FbiFgEXy68LuVm4KTzqrAKMhWSAJJVwf5+ah3p0GbWhLj50p5+fuWDLm8aa8AOTH AMAdmxMQ5FbWrpEyDnxmns+oNlNTWvvzSGoLJRb3ITXAq/kNl7cyaj35uS7sjn1+Q39q NrS9FDEUzhPL9+GVnUXZNv8WO7u5Omj3mnisq78oNWAZVlP1Cs45jWCiXh8PTu7/6Os1 aB5A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=bnLDBG9O91+WOP5nPd+HzZaTii7mLDnN+gkiD+kh784=; b=jWA6WAi7gNWxMk2aSB7JhBYcobsj+oRJe+iNFrRUroeBpbGCH7gK4AjIwNGQiNFrzE K2dAItOGqf1ClLlkUczBjCo0bMBOGyZgxGblH0dqUIR7ZfpPfuP++6A+59VXV5676mGy Zup+NhSfIKN3/LsVEbSIFFPwVmxo2Hjt+4zi21MPhtvGJj0JvOT/Qmni8rQ5+FEBcdX2 4YsydH4GkzibRCDSKgMbxRtzmrE25m3ys7DIa0MSLwq/f8Gzi6SIV95jAJigEWF5JdWK vJcIxM+WNY5AtWB+0PaivREy60oOam1NOXqz0yWMNMtefgTfusaeT8n8LvCMCny+6EPx 8zKg== X-Gm-Message-State: AOAM5333XfZN1+pMiXtDcEw8n+1auAgsnH7cKlkWq8VAXARmohvYEZPu m8D4qCL/tCvLKv0WVptFFm9w7EbPYweTUMDoZV8wmQ== X-Google-Smtp-Source: ABdhPJyTWCjB4uEINxvcmWH+lUdDgh/0tlp9M9cpJ/uqBMH1ofE9Gxt9w4njzas8XMHnX9ZAmjBeIKEt3ISetF+Hb1s= X-Received: by 2002:a37:30b:: with SMTP id 11mr1408279qkd.206.1621458139291; Wed, 19 May 2021 14:02:19 -0700 (PDT) MIME-Version: 1.0 References: <202105161045.14GAjZIL093217@gitrepo.freebsd.org> In-Reply-To: From: Warner Losh Date: Wed, 19 May 2021 15:02:08 -0600 Message-ID: Subject: Re: git: 0f206cc91279 - main - cam: add missing zeroing of a stack-allocated CCB. To: Konstantin Belousov Cc: Mark Johnston , src-committers , "" , dev-commits-src-main@freebsd.org X-Rspamd-Queue-Id: 4FllgJ66Fcz3wCp X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=bsdimp-com.20150623.gappssmtp.com header.s=20150623 header.b=BfzlTQ5E; dmarc=none; spf=none (mx1.freebsd.org: domain of wlosh@bsdimp.com has no SPF policy when checking 2607:f8b0:4864:20::72c) smtp.mailfrom=wlosh@bsdimp.com X-Spamd-Result: default: False [-2.79 / 15.00]; RCVD_TLS_ALL(0.00)[]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20150623.gappssmtp.com:s=20150623]; NEURAL_HAM_MEDIUM(-0.79)[-0.789]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[dev-commits-src-main@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; RCPT_COUNT_FIVE(0.00)[5]; SPAMHAUS_ZRD(0.00)[2607:f8b0:4864:20::72c:from:127.0.2.255]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DKIM_TRACE(0.00)[bsdimp-com.20150623.gappssmtp.com:+]; NEURAL_HAM_SHORT(-1.00)[-1.000]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::72c:from]; R_SPF_NA(0.00)[no SPF record]; FREEMAIL_TO(0.00)[gmail.com]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; MIME_TRACE(0.00)[0:+,1:+,2:~]; RBL_DBL_DONT_QUERY_IPS(0.00)[2607:f8b0:4864:20::72c:from]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com]; MAILMAN_DEST(0.00)[dev-commits-src-main]; RCVD_COUNT_TWO(0.00)[2] Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.34 X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 May 2021 21:02:24 -0000 On Mon, May 17, 2021 at 12:52 AM Konstantin Belousov 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 > 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 > > > > > AuthorDate: 2021-05-16 09:28:04 +0000 > > > > > Commit: Edward Tomasz Napierala > > > > > 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