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 E9A84C54EBC for ; Fri, 16 Dec 2022 16:38:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 78A618E0003; Fri, 16 Dec 2022 11:38:38 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 73AB78E0002; Fri, 16 Dec 2022 11:38:38 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 602C68E0003; Fri, 16 Dec 2022 11:38:38 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 503448E0002 for ; Fri, 16 Dec 2022 11:38:38 -0500 (EST) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 2AE36161180 for ; Fri, 16 Dec 2022 16:38:38 +0000 (UTC) X-FDA: 80248727916.17.92C6178 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf16.hostedemail.com (Postfix) with ESMTP id 5A07C18001D for ; Fri, 16 Dec 2022 16:38:36 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=fEOCux3R; spf=pass (imf16.hostedemail.com: domain of longman@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=longman@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1671208716; a=rsa-sha256; cv=none; b=R5DRmDSrcfpOdmEy4tM/WiyL3pbdmF7OJoY2Zo4iLokLRcUBIBgvHLXtN3CJnsztCQux82 1vT/qjiNOqMbF/pVkXjexWLYSlMO0i78GzThXWhZbJUGH24NqGwoOo5C/7HKdJ2eBaTBrz Tw1LLfavRZjBWiZKlCoZFs9Svd43nY4= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=fEOCux3R; spf=pass (imf16.hostedemail.com: domain of longman@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=longman@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1671208716; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc: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=KPGZ8V2aVqu2fvsMtT4AwXNxUpjMCKpVTRjQyLueouQ=; b=spsMRqshm6SJQTta0hZCIkNdpGd6GCZdX3VjBlKHOi/yxINNiSnjg78ODLRdEBHFwTUSe5 uCMcmopN2wKcBoElwzbL43CpJUfCmeWLnr9l7BOrDbulP7JabdDBxgFnm7s4ogDQn4kgcM DQEHdGVF1yUhbWh73c8bD/BqlyrDH+8= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1671208715; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=KPGZ8V2aVqu2fvsMtT4AwXNxUpjMCKpVTRjQyLueouQ=; b=fEOCux3R+ox699nV5cu8azAtXWbSMkJD0y4Wsj7OOVCuIH2U6osl5VTUy5qbL8J+lqPAJ2 RmtHihfAmOREfpek7Ktn++Qx1xXW5fD0mpP096jK3jIvQkHZD4aZvRSh+cJ5HCYpftT2N3 Y4EAohUxHRyJ1xLrYtpzxnfqG8roeDs= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-266-gLKGBV85P-WR8QU7h6LtDg-1; Fri, 16 Dec 2022 11:38:30 -0500 X-MC-Unique: gLKGBV85P-WR8QU7h6LtDg-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 8F37D1C09064; Fri, 16 Dec 2022 16:38:29 +0000 (UTC) Received: from [10.22.10.173] (unknown [10.22.10.173]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1E6722026D68; Fri, 16 Dec 2022 16:38:29 +0000 (UTC) Message-ID: <7ea28f11-34af-c974-94fb-7e2481942e97@redhat.com> Date: Fri, 16 Dec 2022 11:38:28 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Subject: Re: [PATCH 2/2] mm/kmemleak: Fix UAF bug in kmemleak_scan() Content-Language: en-US To: Catalin Marinas Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Muchun Song References: <20221210230048.2841047-1-longman@redhat.com> <20221210230048.2841047-3-longman@redhat.com> From: Waiman Long In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.4 X-Rspam-User: X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 5A07C18001D X-Stat-Signature: cc3yuioo6bp41qsdhd7xbdajjqdhgejj X-HE-Tag: 1671208716-350499 X-HE-Meta: U2FsdGVkX1+AnPb1onZlV0YTk/fH074GqMq4iSqmJoIkJcBJbyiO58rw+6sBFshQv6Q7/pPBwFYd64NHydMG5MhPDiXCRa1fQ15VeKt++iiNoQqj+d2M5ZroUGs48D8YyKi+Fxqc9XblsJrd34UkB8yrTwc2zUMuPvit+gavQNXICDB9JZI5y4lyY8sw4YRhMflTafSeylgIh4pPUa7grbJ/0WuYAo997bGq5/hFJQOBjFqimAM0V5X0zX8ITEBDphC9SPweT4xenMKhcA8qlPYIukqSuSdZH4S1fVhFTqsWytLjhl9RsWjuCX5pITo4jLtupNg+7Hh/mxkoCON5wvfrliUJcQMj0LF8vSXkwwb1Fn3k6D56wcUj8limlVopXCP/Xv2DoCu5kvr7cQ99TKiUK9cgrmPOA6YIJP+kCd6IjhJN/wrSugYJjjs5wSZ/+m52uImCdCODVROKqirus0fWlF/OEc4aJ9brWYFjm5TQuuHjy5ciUBor6gECFBCc/vT7+537x0qK+PrfKuCCKz6iit8gS/owI1esddzIhtLqgFk2LTAcVfAoRMCX02vHsT6ct4+HQkQS+gc6ryGVcodRTo3l5/y9t2sCfRP9MWun3XjhiVMqeqEm5Lo7W3vh2mT2xkExEVtp9hsmBX2J+iky9CxzF4GkidM51xfS54LsyTqgfX1m7g03IMsmV5H2Dwvt3Z+MMyNCQybSSpvBfUNy9KjYdHuMjB+8rRS5Cbpkp2DYznDT4X6X5uyT76AIq6/OgtB3K+LshGyOy2upOqpMEuQmgUsP8CGxOcAgeF+4RkgqXcmTs5GW1Z4mCzPsSgyFlyA4JwyWCEgdZi4G68OuP4XeHm6u/QWxubB1wWI9148+KaON1U1rnCk0ACB+OHIXyiDaUNaFKvJR/Rqi4tieC1U3UmxBa6fmaV0Qw2w= 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: On 12/16/22 05:32, Catalin Marinas wrote: > On Wed, Dec 14, 2022 at 10:54:28AM -0500, Waiman Long wrote: >> On 12/14/22 06:16, Catalin Marinas wrote: >>> On Sat, Dec 10, 2022 at 06:00:48PM -0500, Waiman Long wrote: >>>> Commit 6edda04ccc7c ("mm/kmemleak: prevent soft lockup in first >>>> object iteration loop of kmemleak_scan()") fixes soft lockup problem >>>> in kmemleak_scan() by periodically doing a cond_resched(). It does >>>> take a reference of the current object before doing it. Unfortunately, >>>> if the object has been deleted from the object_list, the next object >>>> pointed to by its next pointer may no longer be valid after coming >>>> back from cond_resched(). This can result in use-after-free and other >>>> nasty problem. >>> Ah, kmemleak_cond_resched() releases the rcu lock, so using >>> list_for_each_entry_rcu() doesn't help. > [...] >>> Another potential issue with re-scanning is that the loop may never >>> complete if it always goes from the beginning. Yet another problem with >>> restarting is that we may count references to an object multiple times >>> and get more false negatives. >>> >>> I'd keep the OBJECT_ALLOCATED logic in the main kmemleak_scan() loop and >>> retake the object->lock if cond_resched() was called >>> (kmemleak_need_resched() returning true), check if it was freed and >>> restart the loop. We could add a new OBJECT_SCANNED flag so that we >>> skip such objects if we restarted the loop. The flag is reset during >>> list preparation. > [...] >> Thanks for the review. Another alternative way to handle that is to add an >> OBJECT_ANCHORED flag to indicate that this object shouldn't be deleted from >> the object list yet. Maybe also an OBJECT_DELETE_PENDING flag so that >> kmemleak_cond_resched() will delete it after returning from cond_resched() >> when set by another function that want to delete this object. All these >> checks and flag setting will be done with object lock held. How do you >> think? > I think we are over-complicating this. The problems I see with deleting > an object are that (1) only the object being deleted is locked (so that > the corresponding memory block is not freed while scanning) and (2) > kmemleak_cond_resched() releases the RCU lock briefly. A list_del_rcu() > on the object next to the one being scanned (and locked) will leave the > current object->next pointer dangling. Yes, I believe that is the cause of the UAF error that I saw from KASAN. > If we get rid of object->lock and just use kmemleak_lock instead, we can > have a big lock around the scanning, released briefly in > kmemleak_cond_resched(). A standard list_del() (not _rcu) could be run > during the resched but it also updates the current object->next. Once > the lock is re-acquired, the list traversal can continue safely. The > current object cannot be freed due to get_object(). No need for > restarting the loop. The problem with a big lock (kmemleak_lock) is that we will be disabing interrupt for an extended period of time which is not ideal. I have posted a v2 patch that drop the idea of restarting the loop. Instead, I just block the current object from being removed from the object_list to make sure its next pointer will point to a valid object. > > I don't think we'd miss much in terms of scalability for a debug > feature. Object freeing already takes the kmemleak_lock, it's just that > during scanning it will have to wait for the scanning loop to release > it. We might as well release it within the loop on each iteration. > > So my proposal is to replace the rcu list traversal with the classic > one and kmemleak_lock held (some functions like __find_and_get_object() > will have to skip the lock). With this in place, we can subsequently > remove all object->lock instances, just rely on the big lock (we do need > to run lockdep if we do the latter separately, some nesting is a bit > weird; my preference would be to remove the object->lock at the same > time). We still need the rcu freeing in put_object() but for a > completely different reason: the sl*b allocators don't like being called > recursively, so we just use the RCU mechanism to free the kmemleak > structures in a separate thread. That was what I thought about when you said you wanted to use a big lock instead of object->lock in the last email. As I said above, we can't hold the kmemleak_lock with interrupt disabled for an extended period of time especially if RT tasks are running. So we may need to release the lock frequently like per dozen objects or so. I believe we still need rcu_read_lock() just to be safe. Cheers, Longman >