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:   Mon, 10 Apr 2023 11:00:54 -0400
From:   "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To:     Peng Zhang <perlyzhang@gmail.com>
Cc:     Peng Zhang <zhangpeng.00@bytedance.com>, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        maple-tree@lists.infradead.org, stable@vger.kernel.org
Subject: Re: [PATCH 2/2] maple_tree: Fix a potential memory leak, OOB access,
 or other unpredictable bug
Message-ID: <20230410150054.nfmrlqfjdgqvehcn@revolver>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
        Peng Zhang <perlyzhang@gmail.com>,
        Peng Zhang <zhangpeng.00@bytedance.com>, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        maple-tree@lists.infradead.org, stable@vger.kernel.org
References: <20230407040718.99064-1-zhangpeng.00@bytedance.com>
 <20230407040718.99064-2-zhangpeng.00@bytedance.com>
 <20230410124331.kijufkik2qlxoxjz@revolver>
 <84c50299-5b5b-867e-1e96-2d3a0c6ade2a@gmail.com>
 <20230410131258.txkiqa5eudgsrmht@revolver>
 <c9a5bcce-7e1b-81cd-b85f-0e9128024d6b@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <c9a5bcce-7e1b-81cd-b85f-0e9128024d6b@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?bkJweUNKekNHZlM4N0ZZaDNyQ0hzcGdYeXRJU0tMd1dITmV5SS92N2h4MXJC?=
 =?utf-8?B?eVBFMUxSMHZqTDZ0VjdPcWxtekRESXZwbnh0NTlQM1VRVU9BZGwxY3QxSlNs?=
 =?utf-8?B?RTEvLzk2QzFESkNQT3ZCUUF0NUhUWU93WDZ6R25PTlo1T2xzTW1BVEZNbnFC?=
 =?utf-8?B?U3JZWGhVUFBjeEo3aEloR0tENTREVzBOTHU4bHc1dXpMMlhIclVwY2FOd3NG?=
 =?utf-8?B?NjQwV081Nk9zaXhvLzBVcHZFSFdzaVUyd3dBRzhOTHkvdk5OT1IvODhaQ3Bm?=
 =?utf-8?B?a3pzTjAzcjhVZi81cHdST2RKR2tjUDFqYjQ4aFJIckRaZkxSSFpURm13RnVs?=
 =?utf-8?B?NHdwM1VFODc1N1ljUk9xMEQ0ZEtWZ3Y0VHlCQWJwOFVZSXFXb0xHeU8wNHJY?=
 =?utf-8?B?UEtpa0RzNEVTQmUyclBSeHZxNVF4aWZVTFVzNlkvdzl3MmczeWhtajV1Y2dX?=
 =?utf-8?B?c1A2NEsvQkdlY1VnME0vdm4vWGJPbFVPN3ZtNXZndXJZaDdhNTd0QkdKT1lN?=
 =?utf-8?B?eXZIQWRkTDI4QlZBcGpNQlc2blFwZHlHT0FmUjBCOUFSS2ZrZ2d6U1RwS1pK?=
 =?utf-8?B?bnRmRlcrN0F4QWZVcU1vcFZUUWNQNkNkN1pOTnBPRWZxMXlWemdBVFJVVlZX?=
 =?utf-8?B?Y3Zqbi8zYUU1NGlOdlhFS1pmb2dOV2hrWDBxeFNQTGRkTGhwdGFCL1ZqSnNs?=
 =?utf-8?B?alhJODdqalFOQ2pKb3FFM3dFYm5WYXR4c1ExeUFSVEJNc0t1QWc3UkZqMkx3?=
 =?utf-8?B?RC9TNzAzRWlrRGEyR0N6a0x1Z05pcXE3c3BHaFhhREpyL0tLMmVBbG12UU1S?=
 =?utf-8?B?SmQxdktMckYvcVVFM0x4cXZoNlQ3T081eWJjQkJQZnVmS0VQdjdlVFFnclN4?=
 =?utf-8?B?RStrR3htU0pxSjVoZnhod0NBL0NxQnVNRVlhVVJEUjdGeTJLcDlUQTBxbWx3?=
 =?utf-8?B?dFhmT2ZVMGRDQzgvQkRMZ0xnRkNwRWR4Z1RNZXhuTWJoU2haakg1R1RvU2Jo?=
 =?utf-8?B?aTIramZBR0tUT0h1TW5penZ2RUhXN2JHcXovbFRwandzemlERlZ4UzZmZTI3?=
 =?utf-8?B?MkRvTFZ3ZGVXWTZyQzZZbVZCNGUyM1BTUFRBcDVmUnhWcit4ZnEvWk1pQ2Vv?=
 =?utf-8?B?cm9oTS8xRVA1UGxtU2NyQytzK1Y0YnJWYmZ6ZXMwckpOanlBb0dNMjRPQkts?=
 =?utf-8?B?OTVqTmdSVTlNeXJSbWtDS1BPWU5tanU3RnBJWituSENRS3ZIMjZ1Y3BQVzJG?=
 =?utf-8?B?ekZBQU5tWmRtazlWc3ZaTHlqbmxSRE50WW9DbjNuQ255UncyVGorUTYrZzdJ?=
 =?utf-8?B?dyt3eWRVMG9lT1dHdUpCVFhlRDhDek5KYnpBWjNkSGVDNVY4QUVoLzcwQ3dr?=
 =?utf-8?B?MmtEN2FnVmlSYjd5d0xHNXFtOTdOMEFJMHVlS202MjYyRHZpZHNCZTJzczZ3?=
 =?utf-8?B?ZXpreTlhejJEaGZ5MWxNWEt1VVRSY1BybnU2MDk3aWlGRmRicmhqK2czK1lE?=
 =?utf-8?B?bkFMRForV0s5aG03dStrZXVFQ3FiNVhEa0dGY3BTb1E5NzM1SThaaDN3SUR4?=
 =?utf-8?B?RWk3eDJnOFBSRGUyM0ExTURNUkdiZ1B4MlRKa0xUcFkxNlhEdXF6QmZRR1V0?=
 =?utf-8?B?RExkaTRzL1RFbFU5VjI4L3YzQitDbU9IdFFza2xGeWxqeG9YT3hXWkhpUlBM?=
 =?utf-8?B?enpQaEJPTHhobi9vdXIyV1NKV1QzcW0vclJVbjBsYmJZeUdGdnREUEVSL2ls?=
 =?utf-8?B?ZElPTDJqVGFyNERQRSt6bTM2T2JFY3VKT3ZOMTNqUmRSaUcyQzViOFZKK1I1?=
 =?utf-8?B?NDNqTmg1a1kraGxldmF0OE84b0gyQnQ1Nm5rek1laDFsakZlRXJhU2Uvc3Ir?=
 =?utf-8?B?YlBjSjlBVlZFNGU4eE13TWlHZzI1NU5SVFkvN1kzbU9zRnBVaUtoR0dvc3M1?=
 =?utf-8?B?OGU4NldqVVh4dkY5eTFudEdIYkttMVJXVDdRZG9YN1FDcDNDUjZ3Z0NKMzBW?=
 =?utf-8?B?alk4TEpYbEVLcW5HaWFwZFRJQkNSVTd4THFxcVMxUklDT0VVUExvRUg2MFZJ?=
 =?utf-8?B?NHFLaDZoUVFyaWZPYksxMzRHWWI1Tlc3SW9VZEczZjk1Mm8vZUY0dFRmRDRG?=
 =?utf-8?B?VjZyUWtmdGZDcmVNSThiLzdLNXNSNlVhc29BeThUUmh6MHJOZitnUFh1a0J5?=
 =?utf-8?B?WFE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?UmRnOVhrZjBPRHRUOG5WRDQ1dXdiWklMc0QxcHRKZ2p2NFlJbFVmWndoT25x?=
 =?utf-8?B?MmZaRHZCN25ycy9jSHh0SDhsZWNtMHVRNnJIMURPL1hVWXZJQnRZN2l3cERr?=
 =?utf-8?B?NEtzaHU5UlZxOHJhYkZpRldoZ3oyN2ZqTjVkODllK3VpVTJSTldESlpuZnE1?=
 =?utf-8?B?aEg5RXFKRU9ZNk82bHhBbDJQNE5KeU5wV3VreitBNGdDYzNKYXp3SlR0T0Np?=
 =?utf-8?B?WWJ4Z085b3V3TzRqWWdwZzJycFEzU1Z5dVhJVXBWVXY2am1WeFJ5dDdTVS9h?=
 =?utf-8?B?TDhua3VzV0NFSjRKd29pclJoWk1sK2lrWWJMazhMd1ZXSzJoK29BRnBrQzVR?=
 =?utf-8?B?d3FjOVc0ZWJ4RWwyVEhZM2FHUWtFRWRwejIxbVRlWG1EbFhBYlphUTFseVh6?=
 =?utf-8?B?ZjlYRkgrTzNKNEg1amM0dmY0clpBRFlGemM1QWxicTByUXJkN0FaNkY3TExl?=
 =?utf-8?B?b2lYcU1EbU9zMHNVdXZuQWFmL0d6TGlEWHZwcHZMbXZ2WC9tS0hHZnRDZ1py?=
 =?utf-8?B?STVTNCs0S0w4cGNxNzVlM2xkV25Qdk8xa1FzcmROOWZVOVN0VkxHcmIyS2xy?=
 =?utf-8?B?VVAyTkpRUE5LaWpZNVVZSVAxMkNvdG1mMGVMMXNHV3V5YnZwZGt6Ry9MREhx?=
 =?utf-8?B?NWcwa2RtWnplajcyeFIwN0l2dWNBMExNRC9DNE9sRHcwVEhqRS9ibytpcWRG?=
 =?utf-8?B?UnlxZ0tJOG12SlFyL0R0eVpqeVJQRFU1aklReEJXa3FGL1IrV2tlWENNL1U0?=
 =?utf-8?B?QUFTWTBaZjd1cjIvd1RRNVdxb05WajRhQjY2bmlvYmt1c0ZnTFo4TU5MMng3?=
 =?utf-8?B?MVRYVEJ6anBBQndtMmRWb0l0WDl6N0NGTkU5ZkdNbzdta2xlTEJzQ2poYmxl?=
 =?utf-8?B?MGJYNmVzUnZ1RlIzZVVZL0orMGlFaWlzc1FLWGQ4RU96dzdFNExiN2wzd0NL?=
 =?utf-8?B?RTBXWmMvRlNBa2pQcW1pMjA2RzA5eWVlU0Q4L3ExdFJpdVJwUmdIbGgzTGJX?=
 =?utf-8?B?Rkh0VW9JUXo5SG52ZHdLZmtDQUNERjdwS09tcFlBZXVFSVdVOHgzMU1hbVhy?=
 =?utf-8?B?bC83cytsVFMrTVpUNVRQd040YTREN0lmYXYvWE9ZN3lwdys4L1I3YkE0Mk1R?=
 =?utf-8?B?TDdITEFSOXU5KysrczlFb0NudEVrdDFUS2J4WlJNaEpwOGJ0TlVDdS9uWm11?=
 =?utf-8?B?MXJEVnJjSjRUUlRCTHV0RXdSMllLR1d0NnNLUHBZbStzemZack5RL3FYd0Nm?=
 =?utf-8?B?c3A1Ni8xcmx3bFA3ZHVEZjUzRUptUUZDeGFia0REdzVZMUs3UT09?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: abe5ca41-0781-4355-72bf-08db39d46392
X-MS-Exchange-CrossTenant-AuthSource: SN6PR10MB3022.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Apr 2023 15:00:57.1481
 (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: UtDZdeBxYDB7KrKyAvlgRiVL/1uUpA1NMWcGddy8Hvx2/NYg0vTDfwkqVgIhbE8v6RjV6rKkE2jvnpLbttjs1Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR10MB4586
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-04-10_10,2023-04-06_03,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxscore=0
 mlxlogscore=999 spamscore=0 suspectscore=0 bulkscore=0 adultscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2303200000 definitions=main-2304100127
X-Proofpoint-GUID: elhTBlmFNz1Fq07d__AZF759WPJiAMqF
X-Proofpoint-ORIG-GUID: elhTBlmFNz1Fq07d__AZF759WPJiAMqF
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
        DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H2,
        SPF_HELO_NONE,SPF_NONE autolearn=unavailable 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

* Peng Zhang <perlyzhang@gmail.com> [230410 09:28]:
>=20
> =E5=9C=A8 2023/4/10 21:12, Liam R. Howlett =E5=86=99=E9=81=93:
> > * Peng Zhang <perlyzhang@gmail.com> [230410 08:58]:
> > > =E5=9C=A8 2023/4/10 20:43, Liam R. Howlett =E5=86=99=E9=81=93:
> > > > * Peng Zhang <zhangpeng.00@bytedance.com> [230407 00:10]:
> > > > > In mas_alloc_nodes(), there is such a piece of code:
> > > > > while (requested) {
> > > > > 	...
> > > > > 	node->node_count =3D 0;
> > > > > 	...
> > > > > }
> > > > You don't need to quote code in your commit message since it is
> > > > available in the change log or in the file itself.
> > > Ok, I will change it in the next version.
> > > > > "node->node_count =3D 0" means to initialize the node_count field=
 of the
> > > > > new node, but the node may not be a new node. It may be a node th=
at
> > > > > existed before and node_count has a value, setting it to 0 will c=
ause a
> > > > > memory leak. At this time, mas->alloc->total will be greater than=
 the
> > > > > actual number of nodes in the linked list, which may cause many o=
ther
> > > > > errors. For example, out-of-bounds access in mas_pop_node(), and
> > > > > mas_pop_node() may return addresses that should not be used.
> > > > > Fix it by initializing node_count only for new nodes.
> > > > >=20
> > > > > Fixes: 54a611b60590 ("Maple Tree: add new data structure")
> > > > > Signed-off-by: Peng Zhang <zhangpeng.00@bytedance.com>
> > > > > Cc: <stable@vger.kernel.org>
> > > > > ---
> > > > >    lib/maple_tree.c | 16 ++++------------
> > > > >    1 file changed, 4 insertions(+), 12 deletions(-)
> > > > >=20
> > > > > diff --git a/lib/maple_tree.c b/lib/maple_tree.c
> > > > > index 65fd861b30e1..9e25b3215803 100644
> > > > > --- a/lib/maple_tree.c
> > > > > +++ b/lib/maple_tree.c
> > > > > @@ -1249,26 +1249,18 @@ static inline void mas_alloc_nodes(struct=
 ma_state *mas, gfp_t gfp)
> > > > >    	node =3D mas->alloc;
> > > > >    	node->request_count =3D 0;
> > > > >    	while (requested) {
> > > > > -		max_req =3D MAPLE_ALLOC_SLOTS;
> > > > > -		if (node->node_count) {
> > > > > -			unsigned int offset =3D node->node_count;
> > > > > -
> > > > > -			slots =3D (void **)&node->slot[offset];
> > > > > -			max_req -=3D offset;
> > > > > -		} else {
> > > > > -			slots =3D (void **)&node->slot;
> > > > > -		}
> > > > > -
> > > > > +		max_req =3D MAPLE_ALLOC_SLOTS - node->node_count;
> > > > > +		slots =3D (void **)&node->slot[node->node_count];
> > > > Thanks, this is much cleaner.
> > > >=20
> > > > >    		max_req =3D min(requested, max_req);
> > > > >    		count =3D mt_alloc_bulk(gfp, max_req, slots);
> > > > >    		if (!count)
> > > > >    			goto nomem_bulk;
> > > > > +		if (node->node_count =3D=3D 0)
> > > > > +			node->slot[0]->node_count =3D 0;
> > > > >    		node->node_count +=3D count;
> > > > >    		allocated +=3D count;
> > > > >    		node =3D node->slot[0];
> > > > > -		node->node_count =3D 0;
> > > > > -		node->request_count =3D 0;
> > > > Why are we not clearing request_count anymore?
> > > Because the node pointed to by the variable "node"
> > > must not be the head node of the linked list at
> > > this time, we only need to maintain the information
> > > of the head node.
> > Right, at this time it is not the head node, but could it become the
> > head node with invalid data?  I think it can, because we don't
> > explicitly set it in mas_pop_node()?
> 1. Actually in mas_pop_node(), when a node becomes the head node,
> =C2=A0=C2=A0 we initialize its total field and request_count field.

Only if there is a request_count to begin with, right?

>=20
> 2. The total field and request_count field of any non-head node,
> =C2=A0=C2=A0 even if we initialize it, cannot be considered a valid value=
.
> =C2=A0=C2=A0 Imagine if the request_count of the head node is changed, th=
en
> =C2=A0=C2=A0 we don't actually change the request_count of the non-head n=
odes,
> =C2=A0=C2=A0 so it is an invalid value anyway.

When we pop a node, we record the requested value and only initialize it
to the recorded value + 1 if it wasn't zero.  So if there are no
requests, we don't initialize it.

This works because of the zeroing of that request_count that you removed
here.  But it was, as you pointed out, not always using the right node.
I think this needs to be moved to your new 'if' statement.

>=20
> >=20
> > In any case, be sure to mention that you make a change like this in the
> > change log, like "Drop setting the resquest_count as it is unnecessary
> > because.." in a new paragraph, so that it is not missed.
> I thought it was a small change that wasn't written in the changelog.
> In the next version and any future patches, I will write down the
> details of any changes.
>=20
> Thanks.
>=20
> >=20
> >=20
> > > > >    		requested -=3D count;
> > > > >    	}
> > > > >    	mas->alloc->total =3D allocated;
> > > > > --=20
> > > > > 2.20.1
> > > > >=20
>=20