Date:   Wed, 22 Feb 2023 21:12:33 -0500
From:   "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To:     Vlastimil Babka <vbabka@suse.cz>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        maple-tree@lists.infradead.org
Subject: Re: [PATCH v4 48/49] mm/mmap: Remove __vma_adjust()
Message-ID: <20230223021233.f2b23zhfpoqzxryy@revolver>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
        Vlastimil Babka <vbabka@suse.cz>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        maple-tree@lists.infradead.org
References: <20230120162650.984577-1-Liam.Howlett@oracle.com>
 <20230120162650.984577-49-Liam.Howlett@oracle.com>
 <ad91d62b-37eb-4b73-707a-3c45c9e16256@suse.cz>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ad91d62b-37eb-4b73-707a-3c45c9e16256@suse.cz>
User-Agent: NeoMutt/20220429
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?VgxrL7VxBB+mL6OY34Dn0/D6TOYJh9VM+5q+17oIOw5DZm5Ka+U4O2K6J763?=
 =?us-ascii?Q?ClGNuNPkjaCYVLt0PeqLJKVSYQ3dcNXfdTVCH+95qfK7JGH4oY6qV8FQGLt0?=
 =?us-ascii?Q?7KxkK+11HhthVnW3t56UklGOB7nRsqGdttC2fGey7C+SVDATNIMMm/lVFLw/?=
 =?us-ascii?Q?6urWBlyU2uan2ujwmk3hAmsuaR/46QHTkHlHs1sOw2SN1L70N1gSIuF41+rb?=
 =?us-ascii?Q?azp4/jtEs10dG3EJIJIK0TsUt1GUtlX/7UoKLIfJYEaLhWD7IW5z7pBVxBgV?=
 =?us-ascii?Q?1R5Ml2aSJQqFZlsD+j6z35cnwEqtFajEwxnkeZZWNmpxkkGTvQU39WW0sJAS?=
 =?us-ascii?Q?y4tXrb9UORHjHcFGM9o2AgUeoiMo8ajOT/PrEaGXRY/ICu0z36/cGQ8H14+r?=
 =?us-ascii?Q?LubMWiAHFgScg0y5kAfDCU2FdpASXhOHzlhBo275KpAu2h9tUhB/1Qs5eTVW?=
 =?us-ascii?Q?30XwWc46mluQq1AWT68aQ6yury3NbylxuOU+cUeWhqZdiaVH0q3p0EeBQPaG?=
 =?us-ascii?Q?dFKRm8f9iKuU0JJcinub688H0pf1+J8qTMN3JmBT6PP6aBRPH6ihc2JeFSNr?=
 =?us-ascii?Q?frZIZ8z69FcUkoH29pXPlgRhwGDK2XsMRxwin+uJ75o7G7orrXBXP2KBD3Ql?=
 =?us-ascii?Q?ppgx7qAOU/XL1l+9mTrfxdI9XbPJinTdJz3hPhu92YlWr/uHe180FdOke1+l?=
 =?us-ascii?Q?8bcWRgDjVDjXlUs1IP+qd670rU3U7pQShTITZTNYZ1yWV9kxfjybrjPauhEh?=
 =?us-ascii?Q?hTTnEYselcBAlazW5F++zKPKawYyTgyTADbQsT93c+2SdXLE8dFy5Zw3MaP1?=
 =?us-ascii?Q?TYS+9pMKYtd+SElZmPKTnAApOs8j1wpxAzDeTdiCdyLXfRvubqXELF+VJA9B?=
 =?us-ascii?Q?Zt5w6IQoNkZFlvNLGveOX2ufXuQbrTc9su1V3fRJLDUci8I2q5mg88MLVh+n?=
 =?us-ascii?Q?GCqOLVHFgVPJCFKVCSS/npLLWacqVGwljmJl5Zo/vbaLrII6VEvpdeUKM5Gg?=
 =?us-ascii?Q?vQugfeTkhyRtxxLeky7qW1y7FJEKrcgxdY/i2oVxMpe1TxXYLd6yUke5nK+R?=
 =?us-ascii?Q?MuB1NGHqpSwuIjp4067R6Hexjedqr0UXO6rN/SIPnP+OqGG6ue49RW1x+NTl?=
 =?us-ascii?Q?1QWb18dj0HjyDu1ZknIUB4rMATsL+84kFEjPIYnWAcgcnMDoUKqaQKM2YTGE?=
 =?us-ascii?Q?lW1203tLf2fmY9XvSJ7rN8ClKgQt2w2chC/RujrK2GUcmrZ+Ya8zOhwzzjp+?=
 =?us-ascii?Q?QpQIDSGkOVwevCBCngLwre3ZD8u5h687ODKqDKx1FvKSF0yfOMTyXhRWtvEq?=
 =?us-ascii?Q?dj4ZyTA+/cVkbb3+qqBuz+kd3Vs0sLddc4qT9YgiFtK8eX1VcocnzGHWVkzg?=
 =?us-ascii?Q?Gu5dVI54akKwRYz5SjY/qZyD61nbMeo3XnqOyBUwizRHuDt5fBppO1Lt2dYv?=
 =?us-ascii?Q?5sD0trG4qxHKTWmGC59wbHdZP2BuL2THPyIXMwl4EpZTxM4gb4Cxz+DzoDqD?=
 =?us-ascii?Q?pBeVGGG4gZ16X30ojICKtkGSwrMI+Gp0J12YtO3hd8TOnnIcebXJq521LWwY?=
 =?us-ascii?Q?AaB/yZDRHgCxAWWSC+ZThQc9EidlJrFyjD755eSEjaybesLHzCIB/UoNh7eH?=
 =?us-ascii?Q?fw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: Ig3AgFDdO0q2ZW8jReFrO5I5oVX1n2tYLO3M/WBnqI4W8qO5oafaY1DBDOExQMJJFykT1VZHO1v7EZwKop4YzngRaK+8qJfIvRLdAjHjvML69KQ1ahWDJQ56TX2qRvJPWnwap+QIlNjiQJc0Q7nz4sY/VDr7mBey7D4cjS1U63/r8GoSPjOs8iPTNmY5lcAD6PobatQhbeawevPnejGeqaYIkaVaeZfjrb8CPx83eknO30rz6mF4kQpUaoKnPLIFh3vC1JukJ855Hvb1caLW/rzJhH+zEy/qN3GmVnRFsyExGEtzX3oyjYzQ79ltXGTArfUsgzFqlF/Zi/uolYBI86EZNean4tftgpGK+PSS/Uf9VXJOli14JTDjUSCxx7rJ9QinVOkw/FYIVsq9G6ehA5C5EDnk7/AphZjvOTco6xb4TvAEaxH+Y6+UUxoYy2dweadL7VPC5VnKL4795w0UWiTxtmzjCc+WxID+R3gg3DJOy5WpZxYDRpddnnEkFBrU7X1t0tNiQjqeMC9qAiTxnp2yi96l/X4SMdCiu1HL318vKKfpIPycT0j1Sikcc/l3wShzsoUYpZcpaorSGyliuCj6EwqYNmpFEO3w6SbwJ6AK6ouofe0SgQ+C2IhzRHk43emr+IeVxUvnU4vp+dTrt8bqyMqqnix7smhgzGvTjCSZ+HfmxVI881xxihoaJIxt6jsueGu+5GnBBgPkM6uJMXbs0H17MVAKOJbXri3lSHJmEe2faFSj6A/v/TsYK5HWn/g1lW67IEk8nx7oz3Al3RpqPax6bt3fHq6m6fFibPsMpX01Eu0rL13yYEaHmXMEw595z5VB15NblDlhuN+Kv+jrnJ0CxJJxCnDPbYpJVmUgz7QqdazcO63oisu0iNdvyh281GasqTj5ZiKskFLSHkQdjQqVnc6s9ojvFryYYbo=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 582d10f4-69aa-4a95-be4c-08db15436eb5
X-MS-Exchange-CrossTenant-AuthSource: SN6PR10MB3022.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Feb 2023 02:12:37.8078
 (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: 2XPZzO+wGeTfsl5yAYSkXZ4a6VCNm29rHKS44/qMjlhRRWUVUgVXPPeamtBDMfP+NfJeUQNq+/1fKweOM8YBJw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4791
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.170.22
 definitions=2023-02-22_12,2023-02-22_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 phishscore=0 suspectscore=0
 mlxlogscore=758 adultscore=0 mlxscore=0 bulkscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2302230017
X-Proofpoint-GUID: 5_v_HuNxfGrN4xBCA8TqUIWTLS5bmG8l
X-Proofpoint-ORIG-GUID: 5_v_HuNxfGrN4xBCA8TqUIWTLS5bmG8l
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

* Vlastimil Babka <vbabka@suse.cz> [230222 11:17]:
> On 1/20/23 17:26, Liam R. Howlett wrote:
> > From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
> > 
> > Inline the work of __vma_adjust() into vma_merge().  This reduces code
> > size and has the added benefits of the comments for the cases being
> > located with the code.
> > 
> > Change the comments referencing vma_adjust() accordingly.
> > 
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> 
> ...
> 
> > @@ -1054,32 +945,85 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> >  					     vm_userfaultfd_ctx, anon_name)) {
> >  		merge_next = true;
> >  	}
> > +
> > +	remove = remove2 = adjust = NULL;
> >  	/* Can we merge both the predecessor and the successor? */
> >  	if (merge_prev && merge_next &&
> > -			is_mergeable_anon_vma(prev->anon_vma,
> > -				next->anon_vma, NULL)) {	 /* cases 1, 6 */
> > -		err = __vma_adjust(vmi, prev, prev->vm_start,
> > -					next->vm_end, prev->vm_pgoff, prev);
> > -		res = prev;
> > -	} else if (merge_prev) {			/* cases 2, 5, 7 */
> > -		err = __vma_adjust(vmi, prev, prev->vm_start,
> > -					end, prev->vm_pgoff, prev);
> > -		res = prev;
> > +	    is_mergeable_anon_vma(prev->anon_vma, next->anon_vma, NULL)) {
> > +		remove = mid;				/* case 1 */
> > +		vma_end = next->vm_end;
> > +		err = dup_anon_vma(res, remove);
> > +		if (mid != next) {			/* case 6 */
> > +			remove2 = next;
> > +			if (!remove->anon_vma)
> > +				err = dup_anon_vma(res, remove2);
> > +		}
> > +	} else if (merge_prev) {
> > +		err = 0;				/* case 2 */
> > +		if (mid && end > mid->vm_start) {
> > +			err = dup_anon_vma(res, mid);
> > +			if (end == mid->vm_end) {	/* case 7 */
> > +				remove = mid;
> > +			} else {			/* case 5 */
> > +				adjust = mid;
> > +				adj_next = (end - mid->vm_start);
> > +			}
> > +		}
> >  	} else if (merge_next) {
> > -		if (prev && addr < prev->vm_end)	/* case 4 */
> > -			err = __vma_adjust(vmi, prev, prev->vm_start,
> > -					addr, prev->vm_pgoff, next);
> > -		else					/* cases 3, 8 */
> > -			err = __vma_adjust(vmi, mid, addr, next->vm_end,
> > -					next->vm_pgoff - pglen, next);
> >  		res = next;
> > +		if (prev && addr < prev->vm_end) {	/* case 4 */
> > +			vma_end = addr;
> > +			adjust = mid;
> > +			adj_next = -(vma->vm_end - addr);
> > +			err = dup_anon_vma(res, adjust);
> 
> I think this one is wrong, and should be fixed as below. I'm not
> exactly sure about user visible effects, but shouldn't matter if
> we fix before rc1? I guess what can happen is we end up with pages
> becoming part of 'prev' that have anon_vma originally from 'mid'
> which is not connected to 'prev', so eventually some rmap operation
> will fail to do the right thing etc. Or 'mid' is unmapped, its
> anon_vma freed and we have a use-after free. Probably rare to happen,
> but nasty enough.

Yes, you are correct.  Thanks for the closer look here.


> 
> ----8<----
> From 854f4cef0fecde9a0a89ff1a5beb0a1e2115363f Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Wed, 22 Feb 2023 16:51:46 +0100
> Subject: [PATCH urgent for 6.3-rc1] mm/mremap: fix dup_anon_vma() in vma_merge() case 4
> 
> In case 4, we are shrinking 'prev' (PPPP in the comment) and expanding
> 'mid' (NNNN). So we need to make sure 'mid' clones the anon_vma from
> 'prev', if it doesn't have any. After commit 0503ea8f5ba7 ("mm/mmap:
> remove __vma_adjust()") we can fail to do that due to wrong parameters
> for dup_anon_vma(). The call is a no-op because res == next, adjust ==
> mid and mid == next. Fix it.
> 
> Fixes: 0503ea8f5ba7 ("mm/mmap: remove __vma_adjust()")
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/mmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 20f21f0949dd..740b54be3ed4 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -973,7 +973,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  			vma_end = addr;
>  			adjust = mid;
>  			adj_next = -(vma->vm_end - addr);
> -			err = dup_anon_vma(res, adjust);
> +			err = dup_anon_vma(adjust, prev);

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

>  		} else {
>  			vma = next;			/* case 3 */
>  			vma_start = addr;
> -- 
> 2.39.2
> 
> 
>