lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 16 Feb 2024 13:55:10 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Haitao Huang <haitao.huang@...ux.intel.com>, jarkko@...nel.org,
 dave.hansen@...ux.intel.com, tj@...nel.org, mkoutny@...e.com,
 linux-kernel@...r.kernel.org, linux-sgx@...r.kernel.org, x86@...nel.org,
 cgroups@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 hpa@...or.com, sohil.mehta@...el.com, tim.c.chen@...ux.intel.com
Cc: zhiquan1.li@...el.com, kristen@...ux.intel.com, seanjc@...gle.com,
 zhanb@...rosoft.com, anakrish@...rosoft.com, mikko.ylinen@...ux.intel.com,
 yangjie@...rosoft.com, chrisyan@...rosoft.com
Subject: Re: [PATCH v9 09/15] x86/sgx: Charge mem_cgroup for per-cgroup
 reclamation

On 2/16/24 13:38, Haitao Huang wrote:
> On Fri, 16 Feb 2024 09:15:59 -0600, Dave Hansen <dave.hansen@...el.com>
> wrote:
..
>> Does this 'indirect' change any behavior other than whether it does a
>> search for an mm to find a place to charge the backing storage?
> 
> No.
> 
>> Instead of passing a flag around, why not just pass the mm?
>>
> There is no need to pass in mm. We could just check if current->mm ==
> NULL for the need of doing the search in the enclave mm list.
> 
> But you had a concern [1] that the purpose was not clear hence suggested
> current_is_ksgxd().

Right, because there was only one possible way that mm could be NULL but
it wasn't obvious from the code what that way was.

> Would it be OK if we replace current_is_ksgxd() with (current->flags &
> PF_KTHREAD)? That would express the real intent of checking if calling
> context is not in a user context.

No, I think that focuses on the symptom and not on the fundamental problem.

The fundamental problem is that you need an mm in order to charge your
allocations to the right group.  Indirect reclaim means you are not in a
context which is connected to the mm that should be charged while direct
reclaim is.

>> This refactoring out of 'indirect' or passing the mm around really wants
>> to be in its own patch anyway.
>>
> Looks like I could do:
> 1) refactoring of 'indirect' value/enum suggested above. This seems the
> most straightforward without depending on any assumptions of other
> kernel code.
> 2) replaceĀ  current_is_ksgxd() with current->mm == NULL. This assumes
> kthreads has no mm.
> 3) replace current_is_ksgxd() with current->flags & PF_KTHREAD. This is
> direct use of the flag PF_KTHREAD, so it should be better than #2?
> 
> Any preference or further thoughts?

Pass around a:

	struct mm_struct *charge_mm

Then, at the bottom do:

	/*
	 * Backing RAM allocations need to be charged to some mm and
	 * associated cgroup.  If this context does not have an mm to
	 * charge, search the enclave's mm_list to find some mm
	 * associated with this enclave.
	 */
	if (!charge_mm)
		... do slow mm lookup
	else
		return mm_to_cgroup_whatever(charge_mm);

Then just comment the call sites where the initial charge_mm comes in:

	
	/* Indirect SGX reclaim, no mm to charge, so NULL: */
	foo(..., NULL);


	/* Direct SGX reclaim, charge current mm for allocations: */
	foo(..., current->mm);


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ