Received-SPF: pass (google.com: domain of linux-kernel+bounces-91607-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <147b6e99-dc5a-4b40-a1b2-8b957459e76d@redhat.com>
Date: Mon, 4 Mar 2024 22:20:58 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mm/kmemleak: Don't hold kmemleak_lock when calling
 printk()
Content-Language: en-US
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
 linux-kernel@vger.kernel.org, Audra Mitchell <aubaker@redhat.com>
References: <20240228191444.481048-1-longman@redhat.com>
 <ZeCh30o8i-wJVT7N@arm.com> <c5b07970-0523-420b-97ad-c08b50c69db2@redhat.com>
 <ZeHrC56llcicOjLP@arm.com>
From: Waiman Long <longman@redhat.com>
In-Reply-To: <ZeHrC56llcicOjLP@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 3/1/24 09:49, Catalin Marinas wrote:
> On Thu, Feb 29, 2024 at 10:55:38AM -0500, Waiman Long wrote:
>> On 2/29/24 10:25, Catalin Marinas wrote:
>>> On Wed, Feb 28, 2024 at 02:14:44PM -0500, Waiman Long wrote:
>>>> When some error conditions happen (like OOM), some kmemleak functions
>>>> call printk() to dump out some useful debugging information while holding
>>>> the kmemleak_lock. This may cause deadlock as the printk() function
>>>> may need to allocate additional memory leading to a create_object()
>>>> call acquiring kmemleak_lock again.
>>>>
>>>> Fix this deadlock issue by making sure that printk() is only called
>>>> after releasing the kmemleak_lock.
>>> I can't say I'm familiar with the printk() code but I always thought it
>>> uses some ring buffers as it can be called from all kind of contexts and
>>> allocation is not guaranteed.
>>>
>>> If printk() ends up taking kmemleak_lock through the slab allocator, I
>>> wonder whether we have bigger problems. The lock order is always
>>> kmemleak_lock -> object->lock but if printk() triggers a callback into
>>> kmemleak, we can also get object->lock -> kmemleak_lock ordering, so
>>> another potential deadlock.
>> object->lock is per object whereas kmemleak_lock is global. When taking
>> object->lock and doing a data dump leading to a call that takes the
>> kmemlock, it is highly unlikely the it will need to take that particular
>> object->lock again. I do agree that lockdep may still warn about it if that
>> happens as all the object->lock's are likely to be treated to be in the same
>> class.
> Yeah, it's unlikely. I think it can only happen if there's a bug in
> kmemleak (or slab) and the insertion fails because of the same object we
> try to dump. But I suspect lockdep will complain either way.
>
>> I should probably clarify in the change log that the lockdep splat is
>> actually,
>>
>> [ 3991.452558] Chain exists of: [ 3991.452559] console_owner -> &port->lock
>> --> kmemleak_lock
>>
>> So if kmemleak calls printk() acquiring either console_owner or port->lock.
>> It may cause deadlock.
> Could you please share the whole lockdep warning? IIUC, it's not the
> printk() code allocating memory but somewhere down the line in the tty
> layer.
Yes, I will do that in the next version.
>
> Anyway, I had a look again at the kmemleak locking (I've been meaning to
> simplify it for some time, drop the object->lock altogether). The only
> time we nest object->lock within kmemleak_lock is during scan_block().
> If we are unlucky to get some error on another CPU and dump that exact
> object with printk(), it could lead to deadlock.
>
> There's the dump_str_object_info() case as well triggered by a sysfs
> write but luckily this takes the scan_mutex (same as during
> scan_block()), so it solves the nesting problem.
>
> I think in those error cases we can even ignore the object->lock when
> dumping the info. Yeah, it can race, maybe not showing exactly the
> precise data in some rare cases, but in those OOM scenarios it's
> probably the least of our problem.

I was thinking about not taking the object->lock too. You are right that 
under OOM, a little bit of racing doesn't really matter. Will do that in 
the next version.

Cheers,
Longman