From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-f72.google.com (mail-lf0-f72.google.com [209.85.215.72]) by kanga.kvack.org (Postfix) with ESMTP id 8937C6B0005 for ; Mon, 4 Jul 2016 05:48:22 -0400 (EDT) Received: by mail-lf0-f72.google.com with SMTP id g18so118809768lfg.2 for ; Mon, 04 Jul 2016 02:48:22 -0700 (PDT) Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-ve1eur01on0122.outbound.protection.outlook.com. [104.47.1.122]) by mx.google.com with ESMTPS id s188si2635083wme.29.2016.07.04.02.48.20 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 04 Jul 2016 02:48:20 -0700 (PDT) Subject: Re: [PATCH v4] kasan/quarantine: fix bugs on qlist_move_cache() References: <1467606714-30231-1-git-send-email-iamjoonsoo.kim@lge.com> From: Andrey Ryabinin Message-ID: <577A3114.9080008@virtuozzo.com> Date: Mon, 4 Jul 2016 12:49:08 +0300 MIME-Version: 1.0 In-Reply-To: <1467606714-30231-1-git-send-email-iamjoonsoo.kim@lge.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: js1304@gmail.com, Andrew Morton Cc: Alexander Potapenko , Dmitry Vyukov , kasan-dev@googlegroups.com, Kuthonuzo Luruo , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Joonsoo Kim On 07/04/2016 07:31 AM, js1304@gmail.com wrote: > From: Joonsoo Kim > > There are two bugs on qlist_move_cache(). One is that qlist's tail > isn't set properly. curr->next can be NULL since it is singly linked > list and NULL value on tail is invalid if there is one item on qlist. > Another one is that if cache is matched, qlist_put() is called and > it will set curr->next to NULL. It would cause to stop the loop > prematurely. > > These problems come from complicated implementation so I'd like to > re-implement it completely. Implementation in this patch is really > simple. Iterate all qlist_nodes and put them to appropriate list. > > Unfortunately, I got this bug sometime ago and lose oops message. > But, the bug looks trivial and no need to attach oops. > > v4: fix cache size bug s/cache->size/obj_cache->size/ > v3: fix build warning > > Signed-off-by: Joonsoo Kim > --- > mm/kasan/quarantine.c | 21 +++++++-------------- > 1 file changed, 7 insertions(+), 14 deletions(-) > > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c > index 4973505..b2e1827 100644 > --- a/mm/kasan/quarantine.c > +++ b/mm/kasan/quarantine.c > @@ -238,30 +238,23 @@ static void qlist_move_cache(struct qlist_head *from, > struct qlist_head *to, > struct kmem_cache *cache) > { > - struct qlist_node *prev = NULL, *curr; > + struct qlist_node *curr; > > if (unlikely(qlist_empty(from))) > return; > > curr = from->head; > + qlist_init(from); > while (curr) { > struct qlist_node *qlink = curr; > struct kmem_cache *obj_cache = qlink_to_cache(qlink); > > - if (obj_cache == cache) { > - if (unlikely(from->head == qlink)) { > - from->head = curr->next; > - prev = curr; > - } else > - prev->next = curr->next; > - if (unlikely(from->tail == qlink)) > - from->tail = curr->next; > - from->bytes -= cache->size; > - qlist_put(to, qlink, cache->size); > - } else { > - prev = curr; > - } > curr = curr->next; Nit: Wouldn't be more appropriate to swap 'curr' and 'qlink' variable names? Because now qlink is acts as a "current" pointer. > + > + if (obj_cache == cache) > + qlist_put(to, qlink, obj_cache->size); > + else > + qlist_put(from, qlink, obj_cache->size); > } > } > > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754092AbcGDOTp (ORCPT ); Mon, 4 Jul 2016 10:19:45 -0400 Received: from mail-db5eur01on0096.outbound.protection.outlook.com ([104.47.2.96]:36088 "EHLO EUR01-DB5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753896AbcGDOTk (ORCPT ); Mon, 4 Jul 2016 10:19:40 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=aryabinin@virtuozzo.com; Subject: Re: [PATCH v4] kasan/quarantine: fix bugs on qlist_move_cache() To: , Andrew Morton References: <1467606714-30231-1-git-send-email-iamjoonsoo.kim@lge.com> CC: Alexander Potapenko , Dmitry Vyukov , , Kuthonuzo Luruo , , , Joonsoo Kim From: Andrey Ryabinin Message-ID: <577A3114.9080008@virtuozzo.com> Date: Mon, 4 Jul 2016 12:49:08 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <1467606714-30231-1-git-send-email-iamjoonsoo.kim@lge.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [195.214.232.10] X-ClientProxiedBy: AM2PR09CA0002.eurprd09.prod.outlook.com (10.161.22.140) To HE1PR0801MB1307.eurprd08.prod.outlook.com (10.167.247.149) X-MS-Office365-Filtering-Correlation-Id: 93a5a086-1a95-4c19-7cd9-08d3a3f04e65 X-Microsoft-Exchange-Diagnostics: 1;HE1PR0801MB1307;2:9u2RaUqHdDVEduoM4tg6EqltK9V0RvSz88q/rF1PAyzKjONTATyiz4oyJfDaZW4BCBu8BG76zUA04L4WByK98dRNJ547Oi1Sjb3ctI8xz1bh7OU5TtPAjnDPY0Y0hvG+7c3+sLrGYZCsOoMrRa6eNZs6IEcvX++Lpgvm0Ve1LgzgpKZt75c9+hSV9we6An8/;3:f667RGry+2e/5iCi4sOTg7RntxE7fn6mECF3FSUBZ0xBNzHS6L9U2Fh6cCD25od9sOoJBkaxbfDSCrGLLrP2tkxAq55QODydKdV+5N3jyAVxh58uo5Y9hGNddzRSodtO X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:HE1PR0801MB1307; X-Microsoft-Exchange-Diagnostics: 1;HE1PR0801MB1307;25:idVH1EqeKoBmWwXQwVSnaLN2/RA3yd77vSVAP9XMaghMDlNgbtnQeZACUW9h4pSjGnVA656bjx560crsiSRtpeei+0ubKuSKVq02Mj3RylUMbkn0+Iz2++jLb5dxCFCORlx0LdnEWm7Vd69yHSpXW5wGSuw4BCCqPghpGju3pJMGK/Go0jiNb5UZ63deCSpu51EoB2IxgKyXGXdLIcgJMw57qxhLUBKpZJmbzJP0wVR1ykOd3XAaJ77HDL/KuBwX93GZdz6z7uk/a0qStIgeE/xqslwI6/YWywQ1WTzuvaBeCyhV9Cn4SvpWv7eFvW/B9OZiVb9dvjN+DaPwY3Mn25rEhhd96ugrIrfqZnVKRxhhbIV/Vg0JSsNXMGvtEyJEBvxoLCfknbogPa4SgwI/uvn/0Ot64U26g7x23O1vBkraI8HQw7QM6oF1TIWj+UzXmPTSw3KAVoC3RoaPP5pFZSZo6X/hUx8bZdWrmVcETskHmHTfm9HOsD/xnzs41UCOkKmqBzzTQ6A0iRl5txbL0BypJlY3+ErJLYlkHqMi5ocX/yQPajLL+K/EmC5JAgqj33iHFukmSV0CCDl5l/OqZxc5bE5rifHDd2kC13e5q5XZ2i7T2uIDqmw1c/PLXYMK/GS1ygsRwJQPuI20u12V/LNlxRbSlNUtBoKQdPk/QzqkJBDnffOOewdBDSZjrvpx3a/Zui6kSV8dHRXnVUxKc71D1GS7NnDle4XM1i5CcEJY3tTR6X/axXOop+cdT+9q X-Microsoft-Exchange-Diagnostics: 1;HE1PR0801MB1307;31:uDAWzLfhTFXZeYmvMoGkGpx7x75lf7fmFhKMGfMwd2lK7i/HZ2OsG9m7+hVpfyvNiaXEZI7HlxMuDQ8BCTaFPjzNhUcV6DOaNV1iVa0gPU0A7nu8rGhTFMZnvMf2njZNqJLPpZh581gPYBnNO8hDJwYGLZNRnEbQRSrpy+eeau4/GqbDXYsh5bvJKeXYIZVXsexQs0UCx+9p7qM0mS1jQw==;4:sEJiKwpmF3E1f5XLTZTnOc+STU/wpq8m8AEkp1Jm5zdlD7+MTW99jZ1nkaNDqNkMEGvRphhtnheC5YaaKz+ZTxRF+2KJyFgxOQPqODL/zGoRURKY3PjtAQeFQ+dF09zV4PTKL/5yxnAEyVEBsjx4FZespgW8juc2fReMIuywno/JurYx0Q3ACTFjFEOwS7RVXD+6Y0NNQT+s6aJKZzWZUu6+aNKtnl1YRYy1BwHOOQb2ulvHhdtZNOx3zcz5yJWhwrVQbHTHariqeYYA2CmKxMVASZnWbCcwWOf5MN5vxGp2IGgV4aEkL+/5XfzSLP36ux2shyzrh8TVRelctOJFSfGGAzG/JOlcqWEL2KiPYNGGe+nkwQc5JJBRYPqkAemItk0NOp4jgr6KUsPED52hs22jlb34KFGfHpE+Xksr3Ms= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040130)(601004)(2401047)(5005006)(8121501046)(10201501046)(3002001)(6041072)(6043046);SRVR:HE1PR0801MB1307;BCL:0;PCL:0;RULEID:;SRVR:HE1PR0801MB1307; X-Forefront-PRVS: 0993689CD1 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6009001)(6049001)(7916002)(377454003)(189002)(199003)(24454002)(92566002)(19580405001)(23746002)(19580395003)(80316001)(66066001)(59896002)(50466002)(65806001)(65956001)(68736007)(230700001)(105586002)(86362001)(83506001)(42186005)(47776003)(106356001)(65816999)(50986999)(87266999)(76176999)(54356999)(64126003)(33656002)(5890100001)(2950100001)(101416001)(305945005)(8676002)(36756003)(77096005)(586003)(81166006)(81156014)(3846002)(7736002)(2906002)(7846002)(97736004)(5001770100001)(4326007)(6116002)(189998001)(4001350100001);DIR:OUT;SFP:1102;SCL:1;SRVR:HE1PR0801MB1307;H:[10.30.19.223];FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?Windows-1252?Q?1;HE1PR0801MB1307;23:qDeuB7k85b5nMStNE6rdjBFredtkxQX5OQK?= =?Windows-1252?Q?HWIXqvhrYinXJgmvBo8Xm/WB8k50MkP5rNsPG390ltcwwQJdDTPmY3/3?= =?Windows-1252?Q?e0Ojvzfs7zFLYDwtC3TuMDg728GslbnC4sXi1ZWp6nw/18+20VAxVvr/?= =?Windows-1252?Q?QpyU8xWQFkhTubM2O35R6D7TaIZ+fccD83/t/m95+KHZC8jUvdHwTv/t?= =?Windows-1252?Q?d6pgrrAmR1yo4fz8m+ox1u9GXIcrtz9rwKTsA1LUrY+01W4ZPtCVOnuF?= =?Windows-1252?Q?Ynzfa2xAaMHb1ivWI6ARvRYfA1YTIvGgXeGjYtVzKa9oTfgdtdg60760?= =?Windows-1252?Q?k9maKiMPwSqJzmM0ycWOz6N9XFVaWYsxVVcPpupBnIsS6RQbGV2orAfk?= =?Windows-1252?Q?IgInVaMppa4KRhkGzQtFSZpGnEDxei5Oocz4W6kUaXMsWY7yDBd8xfWD?= =?Windows-1252?Q?Xo3d2LYv3JcCTWw1x3dLgmO/tDq5DPQn7Udc042A5Rtp+2zrczOpiA2B?= =?Windows-1252?Q?fRB+pzX4DW1TDvexc5Qn9C3RinJvERyX/c7kjRUhfTeQlLETjHFvWdD5?= =?Windows-1252?Q?ZU8BvUklI6ZXLnHDS9DTEssndx/Je59il8agQbMZcwPlk07Pt1qWjlTf?= =?Windows-1252?Q?ir7GGNZyVmK9Xh8JUbfpt7sbILuDLxjcXKmnDUxKvBPoBS96cb+UNT3h?= =?Windows-1252?Q?qyBomNBz8JeEFmrmJOCRNM7SS4McYyyrskcaV9C+kaQuFZ2ui33YpKqx?= =?Windows-1252?Q?rHL5ndM+8FgNDPu/V1cwA//OAYoqlFABwaXzkRjh4bLaa62KCBtrr/At?= =?Windows-1252?Q?JedXIwnRErDtdgeLQppC18bS14UDlLZL7f4/3/If9f9ITXRA+GJU25Ii?= =?Windows-1252?Q?cCN5FRTDb80BrRgkWROCnlE6jSHl6k9OZc6Zm6x2h0nUjGCWgFJOmJQV?= =?Windows-1252?Q?DrAnXCWOS2ghBiCflM2GB2LK6BXUHcZC2UKnze6Z8XUCc0Zwo0rA2UE2?= =?Windows-1252?Q?O9FJQQvq1jMh1rN5J0/6MJFxUSVjdCpAFOI9Gh7eHLlCiNqj0RXxQhO7?= =?Windows-1252?Q?uR12o2gbIMQdkkHaz2wFY6M67xW7fWt+mdSfNtewc7HSMwnLqYWZJZ9V?= =?Windows-1252?Q?oeBgvjkkgepgI1jSJ15CgFJ8p6k9vcPySvb1F7bdWMW+KAzjHpGQFnwo?= =?Windows-1252?Q?qiGxhQJhv+NWtinLxonOZ4osh9xEKPUWRFQ26aBBouOofMpf6vEXQ0FF?= =?Windows-1252?Q?sZKpeIkbkvbMaxTAKA5iEWjDtnp826DPTQQ7/oBljbXaR8DQXge4opCw?= =?Windows-1252?Q?98WHrwBAzQ6QZWCQMj1+fE/quZB8g4X0f2MNh4VjVGIzm8PbeNZkL2hv?= =?Windows-1252?Q?4qx8wIKLhQvbZBrdpouJlzByBYnunVkEZNhyovWCVD/iVpSU5hgv/Xtu?= =?Windows-1252?Q?UbELWXqyZAT8bPwqfm96/?= X-Microsoft-Exchange-Diagnostics: 1;HE1PR0801MB1307;6:Dn0BKz7QLL+ms5U/9WZDLVdIccOwp5hbH9e5/6LUW6zfiQvr3cpFARlj9b2ED7dYM5AdJZJ34DX4vyGbMApZBNKFlOEdDYtJdYLS/0OHOX0AmOGEfGnMt88YSk7HShlJRGTu0jsaXhy4AweC3IOuCDfjq7OuSoDmaF6JR9lUyRyQiv+0ImvnNPcaMZiXnB7fQABWTInC4t6erqq/WTXmPAMW3PLpqtVTkicpUZOoZs0qchfQlTLEatPpMFu9dsjNlPJm2jkHJcRDyXiPy1SfDFr99wpK+90AivOtS6NAQ6pI8XFb3VkJBGLKf6A9+6Z+;5:Wb/UvWI/YT8MI+AbZ+sxJLMX6itZnXZ3Q5Xit0x3sLYhyUa9ZkCF5D3hnxZmdBqcGolrUB89KD5rWLl3LU+beGjgIBIDJBShqpYwDAqv0tZO1qKkW5ljEqNPebvvFYgOpYfplIvyf7kSdbCjE/APqQ==;24:+PFnTxPJFDjR7tr1Xn1QvDmxurcica7bLcIZBL8/4g0+T3Aza9385Nd03jDFzkB/4YZID9cLP26lr/2ErYSK14HC6mA/2gQkwy26QAeUHlI=;7:x/KXlvtXR8ii1atT0A+Lq8M5/YVw4QTXp8Qe/1k/Kagnnoq/NI9hMg2DVZK3HmjyEQh65VWK7DH/5kLK5lWy9xkm1N3s8qC/bK49z/8PRWTPRXcrdM6Dism/DIEf9IBhYIrnMUOUR/XwqkMcPjyErn/a36qkfetNmZrVnftq4cESHbOqA+f5w0vhtJfOxijanlNPPBsY0fC8EEOP0nnr4cYtjv3bxkQ8QGaDi8qCU/xNMm41Tbsdxsao7TPKYwbP SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;HE1PR0801MB1307;20:To2BpUBhY+B2GZHISCtxVVFpNu3IJ3g5AgNBFApFDUSfEL5XK6LXJetDkc7LfkGQBxBEz/aD6S187yRnkMAuwHVlsnsFTc+x1KlzZi/NgfBGO/9AwzvBcXmVp3GIcPHpe8t0Pyv+V997q3BHGfmFX+GNfVZqeD7RYtZPzxa45Gg= X-OriginatorOrg: virtuozzo.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jul 2016 09:48:08.3688 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0801MB1307 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/04/2016 07:31 AM, js1304@gmail.com wrote: > From: Joonsoo Kim > > There are two bugs on qlist_move_cache(). One is that qlist's tail > isn't set properly. curr->next can be NULL since it is singly linked > list and NULL value on tail is invalid if there is one item on qlist. > Another one is that if cache is matched, qlist_put() is called and > it will set curr->next to NULL. It would cause to stop the loop > prematurely. > > These problems come from complicated implementation so I'd like to > re-implement it completely. Implementation in this patch is really > simple. Iterate all qlist_nodes and put them to appropriate list. > > Unfortunately, I got this bug sometime ago and lose oops message. > But, the bug looks trivial and no need to attach oops. > > v4: fix cache size bug s/cache->size/obj_cache->size/ > v3: fix build warning > > Signed-off-by: Joonsoo Kim > --- > mm/kasan/quarantine.c | 21 +++++++-------------- > 1 file changed, 7 insertions(+), 14 deletions(-) > > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c > index 4973505..b2e1827 100644 > --- a/mm/kasan/quarantine.c > +++ b/mm/kasan/quarantine.c > @@ -238,30 +238,23 @@ static void qlist_move_cache(struct qlist_head *from, > struct qlist_head *to, > struct kmem_cache *cache) > { > - struct qlist_node *prev = NULL, *curr; > + struct qlist_node *curr; > > if (unlikely(qlist_empty(from))) > return; > > curr = from->head; > + qlist_init(from); > while (curr) { > struct qlist_node *qlink = curr; > struct kmem_cache *obj_cache = qlink_to_cache(qlink); > > - if (obj_cache == cache) { > - if (unlikely(from->head == qlink)) { > - from->head = curr->next; > - prev = curr; > - } else > - prev->next = curr->next; > - if (unlikely(from->tail == qlink)) > - from->tail = curr->next; > - from->bytes -= cache->size; > - qlist_put(to, qlink, cache->size); > - } else { > - prev = curr; > - } > curr = curr->next; Nit: Wouldn't be more appropriate to swap 'curr' and 'qlink' variable names? Because now qlink is acts as a "current" pointer. > + > + if (obj_cache == cache) > + qlist_put(to, qlink, obj_cache->size); > + else > + qlist_put(from, qlink, obj_cache->size); > } > } > >