From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C22232260C for ; Mon, 20 Jan 2025 02:39:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737340761; cv=none; b=c1EhL1cU972ZUZLVLRHPXJQSYQxVOMXB+L0MWS1Bb1mFjHajX/taGzYwgzsZWBQI2UYOIqtwfDVCuACnyB2Ub4RHwnapfDNKEU87jYzFLEMWCnYBk1NpvXmdxCOwenamAc0UoWXbLG+HWsP0KIxa9ObZUKGlVvZe7p/gBDABIYU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737340761; c=relaxed/simple; bh=3QxcLWCnQBI11N3j7NP8wfg2oaod7QhyXxSTAZPOZ84=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=VRyQbb4pODucZm44tsqpHn9N4GC/qqn6NaevDdQEYzqdDFqkAfO5T+f0U++wKKzFphSOY02+Pi3bG5KK9CWUcj4vElujb40mODknzifPh0/bhZEvgpRif1iOcdWzGMNt7BgjtYVUoYc6ZQK7DTDALRV/bS6e7vVC8UXpnYkFVrQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=XH55IetP; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="XH55IetP" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1737340757; 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=IOmQpc3a/Z+EiU0zoEFi30PkbyDvn4isXQkxedS9DLQ=; b=XH55IetPALvhglR3b1EpnohCkPOBCB9plMT5fr6pwM8mgOv/weBYQZetsI/Dp3RD8oei4R 5ZCkQml/Z9Bsdhbn6PyFWFVF6uT9k740T34kMB4XL+60zHHzxEKiGHXQXLsiDFPGF/OjY7 RHw22TX9hCUyqPGxq5KLUhzxQaLMcng= Received: from mx-prod-mc-04.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-440-_5G-S1eLMT2iXAj9172JhQ-1; Sun, 19 Jan 2025 21:39:12 -0500 X-MC-Unique: _5G-S1eLMT2iXAj9172JhQ-1 X-Mimecast-MFC-AGG-ID: _5G-S1eLMT2iXAj9172JhQ Received: from mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.40]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 3C3C219560B4; Mon, 20 Jan 2025 02:39:09 +0000 (UTC) Received: from localhost (unknown [10.72.112.227]) by mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id DB10219560BF; Mon, 20 Jan 2025 02:39:06 +0000 (UTC) Date: Mon, 20 Jan 2025 10:39:02 +0800 From: Baoquan He To: Kairui Song Cc: linux-mm@kvack.org, Andrew Morton , Chris Li , Barry Song , Ryan Roberts , Hugh Dickins , Yosry Ahmed , "Huang, Ying" , Nhat Pham , Johannes Weiner , Kalesh Singh , linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 07/13] mm, swap: hold a reference during scan and cleanup flag usage Message-ID: References: <20241230174621.61185-1-ryncsn@gmail.com> <20241230174621.61185-8-ryncsn@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Scanned-By: MIMEDefang 3.0 on 10.30.177.40 On 01/13/25 at 01:34pm, Kairui Song wrote: > On Sat, Jan 4, 2025 at 1:46 PM Baoquan He wrote: > > > > On 12/31/24 at 01:46am, Kairui Song wrote: > > > From: Kairui Song > > > > > > The flag SWP_SCANNING was used as an indicator of whether a device > > > is being scanned for allocation, and prevents swapoff. Combined with > > > SWP_WRITEOK, they work as a set of barriers for a clean swapoff: > > > > > > 1. Swapoff clears SWP_WRITEOK, allocation requests will see > > > ~SWP_WRITEOK and abort as it's serialized by si->lock. > > > 2. Swapoff unuses all allocated entries. > > > 3. Swapoff waits for SWP_SCANNING flag to be cleared, so ongoing > > > allocations will stop, preventing UAF. > > > 4. Now swapoff can free everything safely. > > > > > > This will make the allocation path have a hard dependency on > > > si->lock. Allocation always have to acquire si->lock first for > > > setting SWP_SCANNING and checking SWP_WRITEOK. > > > > > > This commit removes this flag, and just uses the existing per-CPU > > > refcount instead to prevent UAF in step 3, which serves well for > > > such usage without dependency on si->lock, and scales very well too. > > > Just hold a reference during the whole scan and allocation process. > > > Swapoff will kill and wait for the counter. > > > > > > And for preventing any allocation from happening after step 1 so the > > > unuse in step 2 can ensure all slots are free, swapoff will acquire > > > the ci->lock of each cluster one by one to ensure all allocations > > > see ~SWP_WRITEOK and abort. > > > > Changing to use si->users is great, while wondering why we need acquire = > > each ci->lock now. After setup 1, we have cleared SWP_WRITEOK, and take > > the si off swap_avail_heads list. No matter what, we just need wait for > > p->comm's completion and continue, why bothering to loop for the > > ci->lock acquiring? > > > > Hi Baoquan, > > Waiting for p->comm's completion must be done after unuse is called > (unuse will need to take the si->users refcound, so it can't be dead > yet), but unuse must be called after no one will allocate any new > entry. That is guaranteed by the loop ci->lock acquiring. Sorry for late response, Kairui. I went trought the code flow of swap allocation several times, however haven't made clear how loop ci->lock acquiring is needed here. Once si->flags &= ~SWP_WRITEOK is executed in del_from_avail_list() when swaping off, even though the allocation action is still on going, it will be failed in cluster_alloc_range() by the 'if (!(si->flags & SWP_WRITEOK))' checking. Then that allocation requirement will be failed and returned, means no new swap entry|slot allcation will be done. Then unuse won't be impacted at all. In this case, why do we care about it? Please forgive my stupidity, could you elaborate in which case this kind of still ongoging swap allocation will happen during its swap device's off? Could you give an example of the concurrent execution flows? Thanks Baoquan