From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 36B2CC43334 for ; Wed, 20 Jul 2022 14:54:37 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 66F876B0071; Wed, 20 Jul 2022 10:54:36 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 61F4D6B0073; Wed, 20 Jul 2022 10:54:36 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4E7156B0074; Wed, 20 Jul 2022 10:54:36 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 3B6EA6B0071 for ; Wed, 20 Jul 2022 10:54:36 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id EBD23AAEFC for ; Wed, 20 Jul 2022 14:54:35 +0000 (UTC) X-FDA: 79707774510.05.035F81C Received: from mail-wr1-f42.google.com (mail-wr1-f42.google.com [209.85.221.42]) by imf13.hostedemail.com (Postfix) with ESMTP id E4F7120099 for ; Wed, 20 Jul 2022 14:54:32 +0000 (UTC) Received: by mail-wr1-f42.google.com with SMTP id h8so3765818wrw.1 for ; Wed, 20 Jul 2022 07:54:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=yt+ngO8tmWj/KwQcOUTeK5RaJ2ek4CEzrTIRXEZHAOs=; b=RWgbiuU/cvPX+h0opH2QrKUcF1f35fLSeVIBPTODegHvcQF1C1Gac5Xj4lEiH7z+aw aqixvLfNXF0egWiZWeklp84n8XhYxK3u1bUG47zEDPz7woIa6PkxxGm7VEN9iz2JcIea uCHaWLZSI2FPoIHZtT7gnAoomuie9FmTCrnDitK86iEBau1xL2dG8bQ2HdPMuptOn7PG wFJ3oUw0i05d2GQV5Q75ZZxX93wN7rxvn2K0V2KN/COcaPilsQmyUPhAuvJywOBD+wk5 +twk6DST3iNAOVBzNkfGYFO+uabyqoqzSK9j7Fd4eyCy4mkKmgxcEg0foxtN4bI5hBzm CKgA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=yt+ngO8tmWj/KwQcOUTeK5RaJ2ek4CEzrTIRXEZHAOs=; b=k2toHb4tLrVzOOho7H1GjIX+rRe8CBSM3d+a4XRisuU2UGpK98hrxVy7CAw3HdKawl H9pUkbM2MCqSyl9PA3yqfJbWBck8IWw0sJeyS7X3o0v3P014t+LjxpX//rkqKCDNBgvH Yqtvd1ubNBBHlapXcO7sTK/ylMPPU3S1Q6R/Moy/3ZkTM7175r6IBkBg5ohtIjt6PbWU fnEGJI1ZW/AjOdGcuP/ArxPUtq7/LmQRlxhtf2kq4JswZYMUViFpQSEJeaxrX6nMJuP2 ER2N4ngi5s9uQf1bzOEGVOh91wwODc42Dk79Kt1QV5K8jQ4RBJ0sVzrP/vFA1x+x1hIx N/bg== X-Gm-Message-State: AJIora8UkEQFKu0y9suE0AdLVaezJHkSAyhAUbLnl7Qs7M0M/fo7ZCOU Mw4pGa4l4I0r3bgntKRB0TlT6vclgb3tHw4QSto= X-Google-Smtp-Source: AGRyM1u2P5XbvgKRTsW66zBf0bTO0ggXnr+RwjXdB2tcSNL9DspNLx9EDuFXv7g/hzXtCaAHKIODXoPbI4So8lly2TY= X-Received: by 2002:a5d:5f05:0:b0:21d:9ad7:f281 with SMTP id cl5-20020a5d5f05000000b0021d9ad7f281mr32547285wrb.4.1658328871258; Wed, 20 Jul 2022 07:54:31 -0700 (PDT) MIME-Version: 1.0 References: <20220715125013.247085-1-mlombard@redhat.com> <5a469c5a.8b85.1821171d9de.Coremail.chen45464546@163.com> <22bf39a6.8f5e.18211c0898a.Coremail.chen45464546@163.com> <8a7e9cf.1b9.18218925734.Coremail.chen45464546@163.com> In-Reply-To: <8a7e9cf.1b9.18218925734.Coremail.chen45464546@163.com> From: Alexander Duyck Date: Wed, 20 Jul 2022 07:54:18 -0700 Message-ID: Subject: Re: Re: Re: [PATCH V3] mm: prevent page_frag_alloc() from corrupting the memory To: Chen Lin Cc: Maurizio Lombardi , Jakub Kicinski , Andrew Morton , linux-mm , LKML , Netdev Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1658328873; a=rsa-sha256; cv=none; b=f5DxPavhSwQaMBLeyLYykfmB7FW3YhPxqKH6mRhougYjQ02jZExFziDQBVC3myJK5iWQtg Ubr7WennOtDTp733l0gFiy7PLweHmHX6wAGBnjOu8N3YTSFYcAv3doNUYlzrX34Gn0K0iY 02dTB/WNvSBQSyzP+25KCt4DdXLDdCM= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b="RWgbiuU/"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf13.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.221.42 as permitted sender) smtp.mailfrom=alexander.duyck@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1658328873; h=from:from:sender: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:dkim-signature; bh=yt+ngO8tmWj/KwQcOUTeK5RaJ2ek4CEzrTIRXEZHAOs=; b=tjY2qcc8jHSstYTYsGuDONfldUiMx90IS4mWdS5ewAHGT6D3t5GE5wYJDt2nZlWALZs9GM BXpOACDKkW2jwd9IxLgqPvmsQCskeV19cn8WLpaDUOB1JK+hTZxbk4uc3bcBUMPx3M+DRw cPPPz+/lWDPqd/hsVUm/4YeOfTSD0ws= X-Rspam-User: Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b="RWgbiuU/"; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf13.hostedemail.com: domain of alexander.duyck@gmail.com designates 209.85.221.42 as permitted sender) smtp.mailfrom=alexander.duyck@gmail.com X-Stat-Signature: aarfsfwnh9morgjsr1r3agpgjkjjqb71 X-Rspamd-Queue-Id: E4F7120099 X-Rspamd-Server: rspam02 X-HE-Tag: 1658328872-202501 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Jul 19, 2022 at 3:27 PM Chen Lin wrote: > > At 2022-07-18 23:33:42, "Alexander Duyck" wro= te: > >On Mon, Jul 18, 2022 at 8:25 AM Maurizio Lombardi = wrote: > >> > >> po 18. 7. 2022 v 16:40 odes=C3=ADlatel Chen Lin = napsal: > >> > > >> > But the original intention of page frag interface is indeed to alloc= ate memory > >> > less than one page. It's not a good idea to complicate the definiti= on of > >> > "page fragment". > >> > >> I see your point, I just don't think it makes much sense to break > >> drivers here and there > >> when a practically identical 2-lines patch can fix the memory corrupti= on bug > >> without changing a single line of code in the drivers. > >> > >> By the way, I will wait for the maintainers to decide on the matter. > >> > >> Maurizio > > > >I'm good with this smaller approach. If it fails only under memory > >pressure I am good with that. The issue with the stricter checking is > >that it will add additional overhead that doesn't add much value to > >the code. > > > >Thanks, > > > > >- Alex > > One additional question=EF=BC=9A > I don't understand too much about why point >=EF=BC=A1< have more overh= ead than point >B<. > It all looks the same, except for jumping to the refill process, and the = refill is a very long process. > Could you please give more explain=EF=BC=9F > > if (unlikely(offset < 0)) { > -------------->=EF=BC=A1<------------ In order to verify if the fragsz is larger than a page we would have to add a comparison between two values that aren't necessarily related to anything else in this block of code. Adding a comparison at this point should end up adding instructions along the lines of cmp $0x1000,%[some register] jg These two lines would end up applying to everything that takes this path so every time we hit the end of a page we would encounter it, and in almost all cases it shouldn't apply so it is extra instructions. In addition they would be serialized with the earlier subtraction since we can't process it until after the first comparison which is actually using the flags to perform the check since it is checking if offset is signed. > page =3D virt_to_page(nc->va); > > if (!page_ref_sub_and_test(page, nc->pagecnt_bias)) > goto refill; > > if (unlikely(nc->pfmemalloc)) { > free_the_page(page, compound_order(page)); > goto refill; > } > > #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE) > /* if size can vary use size else just use PAGE_SIZE */ > size =3D nc->size; > #endif > /* OK, page count is 0, we can safely set it */ > set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1); > > /* reset page count bias and offset to start of new frag = */ > nc->pagecnt_bias =3D PAGE_FRAG_CACHE_MAX_SIZE + 1; > offset =3D size - fragsz; > -------------->B<------------ At this point we have already excluded the shared and pfmemalloc pages. Here we don't have to add a comparison. The comparison is already handled via the size - fragsz, we just need to make use of the flags following the operation by checking to see if offset is signed. So the added assembler would be something to the effect of: js In addition the assignment operations there should have no effect on the flags so the js can be added to the end of the block without having to do much in terms of forcing any reordering of the instructions.