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 732F2C77B61 for ; Mon, 10 Apr 2023 15:23:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9CE6628002B; Mon, 10 Apr 2023 11:23:44 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9304528001E; Mon, 10 Apr 2023 11:23:44 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7D0D428002B; Mon, 10 Apr 2023 11:23:44 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 681F928001E for ; Mon, 10 Apr 2023 11:23:44 -0400 (EDT) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 8FF3AC0A59 for ; Mon, 10 Apr 2023 15:23:43 +0000 (UTC) X-FDA: 80665851126.20.860A871 Received: from mail-pj1-f66.google.com (mail-pj1-f66.google.com [209.85.216.66]) by imf01.hostedemail.com (Postfix) with ESMTP id A92D94000B for ; Mon, 10 Apr 2023 15:23:40 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=cn89rkUS; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf01.hostedemail.com: domain of perlyzhang@gmail.com designates 209.85.216.66 as permitted sender) smtp.mailfrom=perlyzhang@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1681140220; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to: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=gAAiwiCB/IlxZ+tW1NN/jAR3nnAR8DHWWAB+OfEwJvg=; b=dV1C4zszsBl21zjr6tPM3rx56N+y96FM+a5qyTEOy3SZYPuSy65LdYCGL7Q4o5MMv+N1V9 ma2pHWBxRfrRzmrvIYXrJSZeaQhPZH2FRnJhZm4bN3ru7UtIldCxVRBlqzlKQlMTvH8lmg 1W0Zfsn+OfdOucNIlqH/CkPF8OVG/bM= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=cn89rkUS; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf01.hostedemail.com: domain of perlyzhang@gmail.com designates 209.85.216.66 as permitted sender) smtp.mailfrom=perlyzhang@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1681140220; a=rsa-sha256; cv=none; b=5agaamW4KDKSMBz4MBIMZAoRfxBJ0f7Z8Az3l8T1h0N4UgKSCtJ9aydCEwBAurln8D32UU yKgEZ+jgpFIONvErErpAyuUjGANB0NTGkWCWegBhLADRoDUpxPFSzml4364jDKCaWrwSkI Tr2rkEVe3K42A9PA8M+k+VetN3dzwDs= Received: by mail-pj1-f66.google.com with SMTP id nh20-20020a17090b365400b0024496d637e1so10158638pjb.5 for ; Mon, 10 Apr 2023 08:23:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1681140219; h=content-transfer-encoding:in-reply-to:from:references:to:subject :user-agent:mime-version:date:message-id:from:to:cc:subject:date :message-id:reply-to; bh=gAAiwiCB/IlxZ+tW1NN/jAR3nnAR8DHWWAB+OfEwJvg=; b=cn89rkUSvkBY31gPcK0r5LJ4lI1IntvjKgLNJrWjW3NrJEYqnRJpRqjjHNivrSHdNR 1rZxnELiSfFupOW/KUrbHI8OJOHs+IPhW/bCnV5M7uTD3JysegGmLsSu86AdTjnYN3ZO Dxr/dTtNsaEprPkNGyAWEYuQW//lXNyQwuBeNfXp62c8EL3LiExAXW03Nmp1t5oMpMxe tiezxkNK/RK5+L+GfXw6Dd9T+7fTzGD+ETVND+9G74kfWIRba4R2It0AGh1dPyhNbij7 YKhQItjAWnuJLGyv6D4fZ88X8x5Y37Yc31qPqxptXOY81Qky+Gn3IA07XECppV2l2RGD 0+qQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1681140219; h=content-transfer-encoding:in-reply-to:from:references:to:subject :user-agent:mime-version:date:message-id:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=gAAiwiCB/IlxZ+tW1NN/jAR3nnAR8DHWWAB+OfEwJvg=; b=OQKKRqfiGj2scfzk6ah6mhu5f6iqw1Hc/g/8kz/tSg9RyMx7WHWOYTVAbZvn6y+v0r iiVqfXMpAxqiEgpNTlZjeHedZaTVbJ6KN4O5bUqWrv6ebd1U+B5lSQjAgMmzdBH8aAyI Ea4kO/UGLg59lihDqxdguKR5QwyIyet2T9ufwgfz0YCvWVj8f6ayMhvCOqQmnxIR52fj Wz+kF1tBRstD21mDmVPkshP6s6COCNrPVaotjiGSOt0yPKM6PoR3eWdXyYDUpfNwI2vl jOnSMBzfkFpQx6+rgAHvPmGETLehd43/EDGaXgd+UQzZwQ6kHBv1eGs6UX0L/FrHhZoA X2Qw== X-Gm-Message-State: AAQBX9duoB1VF2a6cXS2XwL5MrCHusuQ51xu6Gmm5I+A3xJ36B0+YL2O ZkbVpITKtpSqf/RgwtSYBeg= X-Google-Smtp-Source: AKy350ZeXaTDh0K7PEBWGBjrhAE6TpuqOt7vs7YwXQMRhRZKwopQRczZw7AEZXwUNqx5n6jU5Kkc4w== X-Received: by 2002:a17:90a:840a:b0:23f:7176:df32 with SMTP id j10-20020a17090a840a00b0023f7176df32mr13169233pjn.40.1681140219268; Mon, 10 Apr 2023 08:23:39 -0700 (PDT) Received: from [10.200.10.217] ([139.177.225.248]) by smtp.gmail.com with ESMTPSA id j7-20020a17090a318700b002468e374d28sm3109841pjb.7.2023.04.10.08.23.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 10 Apr 2023 08:23:38 -0700 (PDT) Message-ID: <16b7b4e1-f6aa-5ee4-a70f-9c07febaee89@gmail.com> Date: Mon, 10 Apr 2023 23:23:32 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Subject: Re: [PATCH 2/2] maple_tree: Fix a potential memory leak, OOB access, or other unpredictable bug To: "Liam R. Howlett" , Peng Zhang , 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> <20230410150054.nfmrlqfjdgqvehcn@revolver> From: Peng Zhang In-Reply-To: <20230410150054.nfmrlqfjdgqvehcn@revolver> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: A92D94000B X-Rspamd-Server: rspam09 X-Rspam-User: X-Stat-Signature: h4patwhdjmodyb461w6hwctsmha6nixo X-HE-Tag: 1681140220-585493 X-HE-Meta: U2FsdGVkX18IVaaKAz0y9q1P53nJO9MM9I4yBtYzFS1ysgjlB87mbcwew7/nDcYex5jM362wfsRObYMRaABi2V7VTnF6Y/ITZETrzodUSptq8I64aEHcN0ud5QZtXASVJtSrmgLV8U9PpN+Hpw9tL6QM02riqbLgt2xzZC6WfIx2lzVKZuV7NGQrwn1wk12gZ1be2Z9TE8lhpNnogZ5RjJa8IqOBgodjzdwgeu6XcHPhBWutOoRaTN1FKJ6Ivh2lLnvdF952+mORmpver7U4AISm4MiEvUjhVS1skObNEAPNqk+m/USvEEWW/dqdYSrY5Y51vzTi151HEoyfUg4MvPBJs+9/ixCDQRAGHToM3WV+B/KehhGogFsLAIsB+4dcbDzVrCRNC3lvj1/5IjLs03D0vOYlGIK717yMoruL1aXaxV6B1Ta/7UkyLq4GHdFbvgCmxutq89szLvmbDk1lH/bMq2yCoKtq6sP+R+ld3DIYVBqOV+nXJCeZOYjkweoIWLtOBv3JU36E49RGh2iGLaZ8136xfkhX8FXgyeB+62Dc6Z21d6adIm4oh1+XMt3Nz/Wz9aQ1WLBg3QXF7mt+VbElawtX3ukVnA6Tk5wWYtdsfT1GfSWIGrsinYZz7IvIo5VAznKLZz/QtiKjivL/sJZtr0RgklaIhRI9oXD7BQYlNkO51iMLB9+hSeNWHz0w4x9WnQ30SAbjAOeddnld5tWLO57V8eWlovdFBbZ7SOln0SoVeQ0icgr8WQMAeCiY0YWFqpp+EIUTNGO39CXIKGjzftgexZBkpEoq+Ft80R5T6UlkCJY1lBJ5o4/uZbe9kkf4mX13QMVnCyBW1w7IZ4pcOiNpaY4+iy8mniNDVOuHzlNh79horVXTDHjYo+/+5uLQbdx1g+tViFG61iGiC5LlvZnPnJi1yCmfRmHR3S9gFQsQqoXSW13XkFnO7jRZC0L6KvTlB6KvNBBOjWR VQbMu3dt j4nlLl4oUeKCS/adAYMbDGdVXL4g0lmI8xcMrznGoqth+Ba1vuUU95ajDPbB/hAoDxNS8Zb76EQfMUP/hiKRfedyS1Q+yit0SPe6YPVr3fbEjHVumg8STMk98T7lVnBOPyqq1nYXPtC0rT4xE26VEp3RJpaVFd9qnSWC8vfILEdjkxPY/fGibyhb1y0k+XsG/Iopglxpu4AQSLnxSMD8Z8luy+EAfNR9qzIsENSt20H5c57DJTk5JW+hPn3CYHICqx0roKp6PrFBwaukxctD+6dX+ZYYSRI20dJpkTZjvgO+OY84uJ2qUDEApwj0a+QZsVnYueHXwc1uIib5llPDJ4FZdwsSMyQBVNnHPp4ks+S3qsLurnMOtRORSu73jrp71niiYn8H7+FQDx2jccIrpymiX1No5s/6iYuuu3z8pXxqBAh6TbbaFiVFyyUcNP8YzFpJgbikr+JIA6oTWT236BcP+z/YVVOl2iiFhJo/KjHZmyoOd5BjwtDXjeDFZAnlYkF9+sO9QHJ3uDx50+4d8dJB0htcvzxmh0x8KDBaFWOhW6qvJI8zdGhnzzulFXBWw/SkawQsJwhfDy2G9ArMKc5hjemA5yii5GGv9uFNXLOKxi2ustRviclJGmzgvW75v8eqp 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: 在 2023/4/10 23:00, Liam R. Howlett 写道: > * Peng Zhang [230410 09:28]: >> 在 2023/4/10 21:12, Liam R. Howlett 写道: >>> * Peng Zhang [230410 08:58]: >>>> 在 2023/4/10 20:43, Liam R. Howlett 写道: >>>>> * Peng Zhang [230407 00:10]: >>>>>> In mas_alloc_nodes(), there is such a piece of code: >>>>>> while (requested) { >>>>>> ... >>>>>> node->node_count = 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 = 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 that >>>>>> existed before and node_count has a value, setting it to 0 will cause 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 other >>>>>> 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. >>>>>> >>>>>> Fixes: 54a611b60590 ("Maple Tree: add new data structure") >>>>>> Signed-off-by: Peng Zhang >>>>>> Cc: >>>>>> --- >>>>>> lib/maple_tree.c | 16 ++++------------ >>>>>> 1 file changed, 4 insertions(+), 12 deletions(-) >>>>>> >>>>>> 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 = mas->alloc; >>>>>> node->request_count = 0; >>>>>> while (requested) { >>>>>> - max_req = MAPLE_ALLOC_SLOTS; >>>>>> - if (node->node_count) { >>>>>> - unsigned int offset = node->node_count; >>>>>> - >>>>>> - slots = (void **)&node->slot[offset]; >>>>>> - max_req -= offset; >>>>>> - } else { >>>>>> - slots = (void **)&node->slot; >>>>>> - } >>>>>> - >>>>>> + max_req = MAPLE_ALLOC_SLOTS - node->node_count; >>>>>> + slots = (void **)&node->slot[node->node_count]; >>>>> Thanks, this is much cleaner. >>>>> >>>>>> max_req = min(requested, max_req); >>>>>> count = mt_alloc_bulk(gfp, max_req, slots); >>>>>> if (!count) >>>>>> goto nomem_bulk; >>>>>> + if (node->node_count == 0) >>>>>> + node->slot[0]->node_count = 0; >>>>>> node->node_count += count; >>>>>> allocated += count; >>>>>> node = node->slot[0]; >>>>>> - node->node_count = 0; >>>>>> - node->request_count = 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, >>    we initialize its total field and request_count field. > Only if there is a request_count to begin with, right? > >> 2. The total field and request_count field of any non-head node, >>    even if we initialize it, cannot be considered a valid value. >>    Imagine if the request_count of the head node is changed, then >>    we don't actually change the request_count of the non-head nodes, >>    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. Yes, you are right. I neglected that if request_count is equal to 0, the request_count field of the new head node will not be set. There are many implementation details of maple_tree, which is quite error-prone. I will modify it in the next version. Thanks. > > 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. > >>> 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. >> >> Thanks. >> >>> >>>>>> requested -= count; >>>>>> } >>>>>> mas->alloc->total = allocated; >>>>>> -- >>>>>> 2.20.1 >>>>>>