From nobody Wed Sep 25 15:14:26 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4XDKyW55KJz5XLcF; Wed, 25 Sep 2024 15:14:27 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R10" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4XDKyW4Gvwz4NFq; Wed, 25 Sep 2024 15:14:27 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1727277267; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=mjZWp98Gqkbai8fj9xjwOEWnGRBDEoMSB2VpLoytxwk=; b=xs0fwnuZtQTmuEqORe/QO2Fadj8eVRs473w2na6gy9nXQ/vrvnSF/3XgB+ScFshmsbQqky tLGLWg2BSn2+q8aNogzQoQW9uZjYVKN/fehT9OrB/NOWLmnl8cyJeRBoa6BMSSPaSrIsoD dUsM1r5iDBgIj/58cjDSotYhtW4MwLkJjI5oHQYHzVFjRDzHGRx4kGJAO8LCqQBuT089gl jLqGc2cpZih6bOvtr+ev8+kIcDIQlUIXvHpbvfGuCbpspaJJkylviRafu590h/qLIPW+YJ fz/wLUAZ9E4UwjMAz2pBwCInkCkJipyMZqsoeEi1nA/ttkRPhJzKB3E6JuqL6w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1727277267; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=mjZWp98Gqkbai8fj9xjwOEWnGRBDEoMSB2VpLoytxwk=; b=w5EFpjdzpmgPO5xKNf7roxab7TWXMwGI+jDGwAFlGoXa39c3x6H3m7KCCBdcXtHdRR62Zv ui8MrDQ90KF/dqvq41slxw7Ml3PsDqfQcv270TIiRz9av1ovMyHA9OSVXkDFiW9woFPo1t PsQu5RW5mucjQc4yVTJWqMEnWcfqDSsyai4Z9SaSVKDsZd31/tNHgDsi4DkXdqUtkBnxhL emV/MougfmwN3yL239OSzablRNSyvR3p8Vxyt7GF7OoiBY6qAr9MiGvRT38vaFqV1pBqA1 gs8KMR35eO/FTr/Ik1xzjlrVEgFMjqbSSfigDRbWNOINJvZEUHBbIt6GemXVNA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1727277267; a=rsa-sha256; cv=none; b=OFgoPqI4vP4/9grPeppWhrjEHoJpR46S3SaJi0Nptuj19emxjnvo+yzDJR9umfB9m0jx/P hw5mpsMvnI4Ait9VfWkY5TmVJfgdydetueS51qjhKVa8OTtDozuFxmYi1MOllemNwZS+Dz 7B0T7S+fNgvALzX7Sdbjp6kwRWf4VgctUQi1O7FmoiO6WYIa/xryzdCfc7hnXbs26x4VCA uRd1O5fju2hb35HnDDujYdVDbXvbHE8eQZfSQ9dUXDXppPbyH6MFdw3xO/oeWm3o1sXpy4 2nba2/fDepPahB080VYiWHsZXKIlOdRc16trqAlRmBHMu7KS+gyFB6Fso+yfDg== Received: from [IPV6:2601:5c0:4200:b830:7d5c:2819:6552:855e] (unknown [IPv6:2601:5c0:4200:b830:7d5c:2819:6552:855e]) (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 did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4XDKyW1rwfz14XW; Wed, 25 Sep 2024 15:14:27 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: <84f27dd0-27a3-4fa9-83c2-df2d7ca0ccbd@FreeBSD.org> Date: Wed, 25 Sep 2024 11:14:26 -0400 List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: git: f08247fd888e - main - Assert that mbufs are writable if we write to them Content-Language: en-US From: John Baldwin To: Drew Gallatin , Kristof Provost , Mark Johnston Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org References: <202409111118.48BBIQ2h065089@gitrepo.freebsd.org> <2b1955e2-fbf1-41cb-b256-a9a257b16a83@FreeBSD.org> <1f61b6de-0fe2-4343-b4ad-f0866785a4bc@FreeBSD.org> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 9/25/24 09:58, John Baldwin wrote: > On 9/13/24 11:45, Drew Gallatin wrote: >> I think you also need to remove M_EXTPG from M_WRITABLE(). Attached a trivial, untested patch. > > Yes, I came back to testing this yesterday and ran into that as well. However, as part of this > I also tried to audit all the calls to M_WRITABLE and most of them are assuming they can use > mtod() to get a pointer. I think what might be a better approach is to add a new > M_WRITABLE_EXTPG() variant that doesn't check M_EXTPG and leave M_WRITABLE as-is. Places > like m_copyback that are M_EXTPG aware would use the new macro. This still requires the > patch to not set M_RDONLY in all M_EXTPG mbufs. The other thing we might want to do though > is set M_RDONLY on encrypted data after KTLS has encrypted it as there is no good reason to > modify encrypted data. I've uploaded a series of reviews starting with https://reviews.freebsd.org/D46783 >> Drew >> >> On Thu, Sep 12, 2024, at 5:40 AM, John Baldwin wrote: >>> On 9/12/24 05:03, John Baldwin wrote: >>>> I think part of the motivation for marking M_EXTPG as read-only is that you can't "write" >>>> to m_data via mtod() or the like. That said, M_EXTPG aren't really read-only. It >>>> depends on the backing store. M_EXTPG were first merged into FreeBSD prior to KTLS to >>>> support sendfile, and in that case, they should be M_RDONLY because they alias pages >>>> from the file's VM object. However, M_EXTPG mbufs allocated via functions like >>>> m_uiotombuf_nomap should not be M_RDONLY. I think this originated in the original >>>> import of KTLS which doesn't push setting M_RDONLY out to the callers of mb_alloc_extpgs, >>>> and a few other places that hardcode M_RDONLY with M_EXTPG (_mb_unmapped_to_ext should >>>> preserve M_RDONLY from the original mbuf instead of forcing M_RDONLY). >>>> >>>> I can take a stab at a patch but won't have time to really test it until after Euro. >>> >>> Patch available below. Compile tested but not run-tested: >>> >>> https://github.com/freebsd/freebsd-src/compare/main...bsdjhb:freebsd:m_extpg_rdonly >>> >>>> On 9/11/24 16:56, Drew Gallatin wrote: >>>>> M_EXTPGS mbufs are marked read-only because they refer to external data. The original crypto code, (before kTLS was converted to OCF), used to just build an iovec using PHYS_TO_DMAP() on the page array. I think this case was missed during the conversion to OCF. >>>>> >>>>> I'm not sure what the best thing to do is, as they should be read only, except this one specific case.... I'd be tempted to just nerf the KASSERT for EXTPGS. >>>>> >>>>> On Wed, Sep 11, 2024, at 11:02 AM, Kristof Provost wrote: >>>>>> On 11 Sep 2024, at 16:45, Mark Johnston wrote: >>>>>>> On Wed, Sep 11, 2024 at 11:18:26AM +0000, Kristof Provost wrote: >>>>>>>> The branch main has been updated by kp: >>>>>>>> >>>>>>>> URL: https://cgit.FreeBSD.org/src/commit/?id=f08247fd888e6f7db0ecf2aaa39377144ac40b4c >>>>>>>> >>>>>>>> commit f08247fd888e6f7db0ecf2aaa39377144ac40b4c >>>>>>>> Author: Kristof Provost >>>>>>>> AuthorDate: 2024-09-10 20:15:31 +0000 >>>>>>>> Commit: Kristof Provost >>>>>>>> CommitDate: 2024-09-11 11:17:48 +0000 >>>>>>>> >>>>>>>> Assert that mbufs are writable if we write to them >>>>>>>> >>>>>>>> m_copyback() modifies the mbuf, so it must be a writable mbuf. >>>>>>> >>>>>>> This change still triggers a panic for me when running KTLS tests. I >>>>>>> note that EXTPG mbufs always have M_RDONLY set, but I'm not quite sure >>>>>>> why. I suspect such mbufs need special handling with respect to the new >>>>>>> assertion. >>>>>>> >>>>>>> syzbot also triggered this panic: >>>>>>> https://syzkaller.appspot.com/bug?extid=58c918369f9dc323409d >>>>>>> >>>>>> Yeah, I saw that one before I went out for a bike ride. >>>>>> >>>>>> Clearly something is wrong. Either ktls is using read-only buffers or the M_WRITABLE() macro isn’t quite smart enough to spot this specific case. >>>>>> >>>>>> I’m not familiar enough with ktls to easily tell which. >>>>>> >>>>>> I’ll back this assertion change out for now, so we’re not panicing test machines while we figure this out. >>>>>> >>>>>> Best regards, >>>>>> Kristof >>>>>> >>>>> >>>> >>> >>> -- >>> John Baldwin >>> >>> >> > -- John Baldwin