From owner-dev-commits-src-all@freebsd.org Wed May 19 21:02:22 2021 Return-Path: Delivered-To: dev-commits-src-all@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 A85C964BE82 for ; Wed, 19 May 2021 21:02:22 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qk1-x732.google.com (mail-qk1-x732.google.com [IPv6:2607:f8b0:4864:20::732]) (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 4FllgJ5tPrz3w3R for ; Wed, 19 May 2021 21:02:20 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qk1-x732.google.com with SMTP id k4so3069004qkd.0 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=XlskK9CV4Ohd/HulujpvLbTQZFm+L1wXmRNRS0mQbZTO3YmxRDIKXcyBAkMgMxUWwI qlgMwGGirb7mmCqFLdbYY8Ex7osZws81oU3U3njxQYI576yyaS1bge51mxaozCC0J6Lk gM4UatlEye1sTEE7sAXe7uPSQCDnjeexTIkHNUgk1yJ3yxfDJs917MkM5EkFaueDdrBi 8HfgNGQ9JxGAfsMaTUZ1v154VfaEOiFP85UVGIsbkL+K+lM4M6De7TpeFKz4XQEiKVM6 uJio9on7AbEmkugsiZUyruLc+inSgLIHybzCjsU3GpmRsdAoPIGniCpnidMgfalNQQjn GWeg== X-Gm-Message-State: AOAM5313FG0tmW5lLtzbUIuUKKtStNiZaGNTZe73eLWcepsG6sr1Zxqz GhFd3jdtUjWRnH4+jOGuUZ9NvfczgnCJP02vSzD+9Q== 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: 4FllgJ5tPrz3w3R 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::732) 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-all@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; RCPT_COUNT_FIVE(0.00)[5]; SPAMHAUS_ZRD(0.00)[2607:f8b0:4864:20::732: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::732: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::732: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-all]; 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-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 May 2021 21:02:23 -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