Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 13 Jun 2023 21:18:14 -0400
From:   "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To:     Jeff Xu <jeffxu@chromium.org>
Cc:     Peter Xu <peterx@redhat.com>, linux-mm@kvack.org,
        linux-hardening@vger.kernel.org, zhangpeng.00@bytedance.com,
        akpm@linux-foundation.org, koct9i@gmail.com, david@redhat.com,
        ak@linux.intel.com, hughd@google.com, emunson@akamai.com,
        rppt@linux.ibm.com, aarcange@redhat.com,
        linux-kernel@vger.kernel.org, Lorenzo Stoakes <lstoakes@gmail.com>
Subject: Re: inconsistence in mprotect_fixup mlock_fixup madvise_update_vma
Message-ID: <20230614011814.sz2l6z6wbaubabk2@revolver>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
        Jeff Xu <jeffxu@chromium.org>, Peter Xu <peterx@redhat.com>,
        linux-mm@kvack.org, linux-hardening@vger.kernel.org,
        zhangpeng.00@bytedance.com, akpm@linux-foundation.org,
        koct9i@gmail.com, david@redhat.com, ak@linux.intel.com,
        hughd@google.com, emunson@akamai.com, rppt@linux.ibm.com,
        aarcange@redhat.com, linux-kernel@vger.kernel.org,
        Lorenzo Stoakes <lstoakes@gmail.com>
References: <CABi2SkWx_BnEHzGqqqbDMJi+vi-5a7XkQUCkyesN5PUtk23SgQ@mail.gmail.com>
 <CABi2SkXw6ZD-M1ZrcXNL7abtM=RzQXv716PPM_k=1Tay=5rUFA@mail.gmail.com>
 <ZIjOlU5EfVNt6NRU@x1n>
 <CABi2SkXE4pUhHucZ_c-_4Ux-VcLKic0+HY_DN2wUEC6DGkDvQQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CABi2SkXE4pUhHucZ_c-_4Ux-VcLKic0+HY_DN2wUEC6DGkDvQQ@mail.gmail.com>
User-Agent: NeoMutt/20220429
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QXF5czdXWVY3NzRydStwUTl0VStTNTFsQXNRamtnay9RdDJCL3V1dkxDUWQ2?=
 =?utf-8?B?MTZOWkdydElTK1VobFRsdzZsMklDVUE1MXFrRTE4cS9NUzdiaEdPTFZBMXk0?=
 =?utf-8?B?ZmlVV2FmMXhJUG1uY0p5TUtDZXNEbndqWFN1ODR0Q2J6aWxKNytNRXV1L29h?=
 =?utf-8?B?Z1J0Y294VEMvaGVSRG5WampTTFZ4YWRGUXJkU1B6YnRZL2R2WWZONi9oZ0lL?=
 =?utf-8?B?alladGxZUmN5MUdhbjVlT09QZHhJOUpJSlhUaHh5TmVJbFFBYStaaFNSQjRB?=
 =?utf-8?B?bDUvSUF5RG1vWGNuTC9uczRuaUF0NDJMVUp2NzZLUisvVGZWL2ptMS8vMnlB?=
 =?utf-8?B?bDRaK0hmNDR0dlhtSWx3TGJNanRDR21xYmhiRi9pUm1kTWY3MTd1U2VZM2M3?=
 =?utf-8?B?cmNiN1FoWCtrSXFUNVhlTWJyeVNYaWpEd1hQTERpZE5iQ01MQ3Zhb0psYkhx?=
 =?utf-8?B?QXIweStVdUZueDd3cGp6M3dnKzFKUm4vSTZ2L1Nuemh2ME8ySUNHeFdrR2Fz?=
 =?utf-8?B?c29mUlEyVy92Qlc5V1V4dFFzZ25Nam0zSHRHcy9JdE81b25SMEdESUFyNDYw?=
 =?utf-8?B?VjlpVFppMU50VmRuelAxd253SEh4ZCtHYXlHQ0ZBU1A0ZnkyRVRlV3FmVlZ5?=
 =?utf-8?B?U21GNnhUd3M4TTQrbkF4aHpCMWc5NFBzbVRrallGSVVaYmZFS0FDSkxtN3Bj?=
 =?utf-8?B?YVIyUmdhRTFmU0JHVGI1SjY3Mk1jRnUwOGZ3SW5GaFVieEF1Y0I5MGpvbkd5?=
 =?utf-8?B?Ty92aDJ5cW9UMWQ4bHFFaEhvZktrdXJ6OE5zdHJ6N2FqUGFNeTRnS0d5d1hW?=
 =?utf-8?B?MVVVSnRFb3l2VU9rdnRmRHE4V0FXY3B5alMyN1Fsakc3ZU8wZ2V4VW9pS3dt?=
 =?utf-8?B?SGZQWG42cWgyODYxazZFb0V6MW9ZdFdWb3RpaHlXcGY2ZUY5eWgzcXNIVjJS?=
 =?utf-8?B?L2xrNzZxcTUyckFhWU5BMzgyOE9ONGQxMTliZ0plczBiSnJvZ3pCZ3BtNjBW?=
 =?utf-8?B?UnZqbEJsMmJMRlBlbFArOUlkQ2kzclpQbkJjQ3lsbzRQRTluOHZnU0g1cUF2?=
 =?utf-8?B?aFFNNlFtM2IwbGlzM2kwQjJscGtwWXpnUDd3ZDB2K0xOd2Jld2htNEpFSlV3?=
 =?utf-8?B?bVlDdmVmeVp5eUxMbHhlbHZCc1JET3BGYWZiZ0taak1RVWpDeWJMaE1FSFZV?=
 =?utf-8?B?SDAxdzNvK3hEa2JmaUpjbGdONUEzQ2c2b0tjRUFjcHBxYUphZVNyR1lHdXZ5?=
 =?utf-8?B?UnZzYkh5NU1SZG8rNitTQS9BTEk2TmhWM29wYS9WR0Qxb3RNVEY0bGNwc05X?=
 =?utf-8?B?THJCL09KWWpnK0ZPMDY2ZVdHOEZFT09EaE9OR05JcEt2RncyRFNwY1d5cDJB?=
 =?utf-8?B?VnZpdDRqcmgvUU9KdFlUSUVueVNEVERZeVR3OVBtQXZKU1JQeHVEaURlZEdt?=
 =?utf-8?B?SjFKY3g4TVV4c2NScmt4RmFGMktIejM2QXVieUlQYVBWSVhBNllOL3NDbkhX?=
 =?utf-8?B?dzNEV1VvdHovNDlrK1NhRFZtbW5zdFdHd2FXNmxBak8rSXpSeHphcnU2bWk3?=
 =?utf-8?B?UXdMeVNDL2NadnZ5L1ltTmpBWG85aVR5eE41azRuZlBZNENtMHIzWUw5N1dn?=
 =?utf-8?B?YU9EUVA1bjNHb1Yxd0dPd0ZyZ2c5eWREVkhTcTQ3NGZKR1luemg0ZUk0aVo5?=
 =?utf-8?B?QVhFMXkxc050TmFTb09Vc0ZjRG9mbjF4TGRhUWgvM1BlZGxwbXRweW5kd0ND?=
 =?utf-8?B?bzBaN09wMW1TZHp3RUhkNkt3ZGhkbC9QengvVnNyRGRkRFMzT1ZVSVhUdXM2?=
 =?utf-8?B?QzVENHRIY2xISTBwSE5Ibk5vajF6Q2V3NkxRbktHVXJEUEV5aERSdi9CRDY3?=
 =?utf-8?B?TEVXTldFM3VuL0lQQnFKemZ6TkpUcWxaM3FYeHYwcjVja0ZOclZTZDA5elVP?=
 =?utf-8?B?VitNckUwTlBFT1NRS1RTRWNueXhaZlZYYTdNdzFtVlBXRXRHbEVSaktOSE9I?=
 =?utf-8?B?R2kvSmdPemJSaldERFlLUGd2bjBOcXNDQmJzTzB4VXovVUsyYlhBVG9KYW90?=
 =?utf-8?B?NCt3Qi9CSStlV0srUmFBMWNEL2hRZjllQm5TS2d0S09UMkNxVkJFd21EODdR?=
 =?utf-8?B?ZUFlUUJnb1MxK1ZZemJiWkF0b3cwSExuZ1IwTTR5Q3k4eTQvbUxvVllVclRV?=
 =?utf-8?B?Ync9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?RG9pZEE2SFN4TUU5Vy9jTWw5TUxtU0tSVHFhQ0M2QktvR2M0T3pFdEtkeGdX?=
 =?utf-8?B?MmQrNTdEdmNRNCtnRGNlWXpLaUdweGY0SzAwWEpBQmIwVW5qRllXazdTZWMz?=
 =?utf-8?B?WEQ5OTVkNHd5QzNSUlZSN09NZjRBSUVBNllqTFRSbFNZSXlMU2Y4MHQvSE1n?=
 =?utf-8?B?OFNEeWNTNzF5a1p5RXFSV2NNK3F0OVdrZWM4RlBDYTZHQzJYcGZQZURhNVl2?=
 =?utf-8?B?V1F0a0tLdnE0Q0tJVENkVDI1UC9qUDJNbXM4bTFwTTFIeVVySTdxOE9tS0M5?=
 =?utf-8?B?VUMxSEowaTc3WEIyZDdJSTZwU1FYNEF5d3BxNTVGUzFwSEpsQmpwbGg0RVNE?=
 =?utf-8?B?aWVRdXN3L25BclkxZDRNTnlVZzZVbGoyMEFrc29JdnE2YWwva2dNVTVFdUYz?=
 =?utf-8?B?dlVFRDdKWU9wTXpDdUVHZEFrZzBBQlZGTFNPcG4rWGMyTnQ4cThlTW56UStJ?=
 =?utf-8?B?OGVoN0llZDJ5Nk5VRlZnMDd6MitiZDdoRlpaL2piN0ZrZlQ0bVp6aDIvcmt3?=
 =?utf-8?B?MWZjQVlTZVdEV3UwWU1abHJNRDMyTnBRRm50Q2N3MHBEUUVucjNGeTdPZVdL?=
 =?utf-8?B?LzkzakIvTWorb1p3ajgvODY5SHdCQjRMSlJvbmwxemgvTXhkS0grN2FzdEty?=
 =?utf-8?B?T3JCSDFHc2ZEK2FwSEJvNnNOM0swTTJGYWdKU0xPRSsyeVNsSkF1NHNKNjhR?=
 =?utf-8?B?a3pOd2o1S0t5cHdHSmFxWVBZY1g0Ny8rcmdETDZOZWo4MlQzNkV0b1ZWYld5?=
 =?utf-8?B?WDRuUlNKNFBxZFdDeGdQV0svZmlpM2Z5UHpzWmJiN0p4andQSzR6NkhtbWZ6?=
 =?utf-8?B?WFV5aEVtZ0o2Z1hLb2JWS2Z4SmpTRnVQS2p3K0JTNklVNG5CdlRtZzE4RzVS?=
 =?utf-8?B?WFd5Yi9QTWFhL1paNEYyOXBTUmFZdWhJK1lxZUdYblE2Yy9rYW1SVmpqOERC?=
 =?utf-8?B?WWMvNnhwU291V2tIQXVOdHpBWjhuQ05nU3gzdm5SQ2o4TStMSS9iclUvb3Zp?=
 =?utf-8?B?a28wQkhEOEZVZFdqMUZOL0xBYkVwMTI1RkxtM0pmMCtiWDJQZUdKanh1SjlC?=
 =?utf-8?B?cG93VVVoeFd5eWo3Y3BiMVA5cDYvL3ZDQjU2Skg3aEpmaVU2SVlLSGtLZUR1?=
 =?utf-8?B?T0xSU1dUSmJhNnZ4NDE1WitxMitUT0xDZjhoUGlwazBHNS9aeFlsYmQ2OXNy?=
 =?utf-8?B?cHpseTVZRE5nRWpwRmx1R0FJVmRSNkl5M1oxdTA0ZUFnSmhRQytnNUwzeVV6?=
 =?utf-8?B?WGpmdXRjVzkzSjZQZ1VKMkxENkpPejFpZzZZbnhOSk1BbXpDS0U4NTEzUGpu?=
 =?utf-8?B?eElWdmhtTzg5bGU4dCsxL25abGFzbm4zT1ljOGRvREZTWW1IcWVUaUhTUDJG?=
 =?utf-8?B?dDRVak14WU5tMUpNSkg4aE56S2x5S1JON1dZK2szcFh2T01LSHJrdzNqK1h1?=
 =?utf-8?B?cXMrWDd1ZVNyaTZYM0htRmlVOEMyVU9XNVphTHRobmU1NXNyS2xXenV2N2N0?=
 =?utf-8?Q?YWek44=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 13e500ff-af9c-47d6-2d87-08db6c753c18
X-MS-Exchange-CrossTenant-AuthSource: SN6PR10MB3022.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2023 01:18:17.9649
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: LEu7qlXRxH9K0Z6UZcK43pNjbx6bCoZcQ8+9lV7rD5NEXWQ1y80rM+pc48XBizgnnlFqX1PlGymLEsd5E7XfKw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB4849
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.176.26
 definitions=2023-06-13_24,2023-06-12_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=811 phishscore=0
 malwarescore=0 mlxscore=0 adultscore=0 bulkscore=0 spamscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2305260000 definitions=main-2306140008
X-Proofpoint-ORIG-GUID: OMDotedKp__7L2s05aihElVp_7_qJDlH
X-Proofpoint-GUID: OMDotedKp__7L2s05aihElVp_7_qJDlH
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

* Jeff Xu <jeffxu@chromium.org> [230613 17:29]:
> Hello Peter,
>=20
> Thanks for responding.
>=20
> On Tue, Jun 13, 2023 at 1:16=E2=80=AFPM Peter Xu <peterx@redhat.com> wrot=
e:
> >
> > Hi, Jeff,
> >
> > On Tue, Jun 13, 2023 at 08:26:26AM -0700, Jeff Xu wrote:
> > > + more ppl to the list.
> > >
> > > On Mon, Jun 12, 2023 at 6:04=E2=80=AFPM Jeff Xu <jeffxu@chromium.org>=
 wrote:
> > > >
> > > > Hello,
> > > >
> > > > There seems to be inconsistency in different VMA fixup
> > > > implementations, for example:
> > > > mlock_fixup will skip VMA that is hugettlb, etc, but those checks d=
o
> > > > not exist in mprotect_fixup and madvise_update_vma. Wouldn't this b=
e a
> > > > problem? the merge/split skipped by mlock_fixup, might get acted on=
 in
> > > > the madvice/mprotect case.
> > > >
> > > > mlock_fixup currently check for
> > > > if (newflags =3D=3D oldflags ||

newflags =3D=3D oldflags, then we don't need to do anything here, it's
already at the desired mlock.  mprotect does this, madvise does this..
probably.. it's ugly.

> > > > (oldflags & VM_SPECIAL) ||

It's special, merging will fail always.  I don't know about splitting,
but I guess we don't want to alter the mlock state on special mappings.

> > > > is_vm_hugetlb_page(vma) || vma =3D=3D get_gate_vma(current->mm) ||
> > > > vma_is_dax(vma) || vma_is_secretmem(vma))
> >
> > The special handling you mentioned in mlock_fixup mostly makes sense to=
 me.
> >
> > E.g., I think we can just ignore mlock a hugetlb page if it won't be
> > swapped anyway.
> >
> > Do you encounter any issue with above?
> >
> > > > Should there be a common function to handle VMA merge/split ?
> >
> > IMHO vma_merge() and split_vma() are the "common functions".  Copy Lore=
nzo
> > as I think he has plan to look into the interface to make it even easie=
r to
> > use.
> >
> The mprotect_fixup doesn't have the same check as mlock_fixup. When
> userspace calls mlock(), two VMAs might not merge or split because of
> vma_is_secretmem check, However, when user space calls mprotect() with
> the same address range, it will merge/split.  If mlock() is doing the
> right thing to merge/split the VMAs, then mprotect() is not ?

It looks like secretmem is mlock'ed to begin with so they don't want it
to be touched.  So, I think they will be treated differently and I think
it is correct.

Although, it would have been nice to have the comment above the function
kept up to date on why certain VMAs are filtered out.

>=20
> Also skipping merge of VMA might be OK, but skipping split doesn't,
> wouldn't that cause inconsistent between vma->vm_flags and what is
> provisioned in the page ?

I don't quite follow what you mean.  It seems like the mlock_fixup() is
skipped when we don't want the flag to be altered on a particular VMA.
Where do they get out of sync?