linux-kernel - Re: [PATCH 2/3] cxl/mbox: support aborting the current background operation

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]

Message-ID: <20250114193344.jswodb44wupxx4a3@offworld>
Date: Tue, 14 Jan 2025 11:33:44 -0800
From: Davidlohr Bueso <dave@...olabs.net>
To: Dave Jiang <dave.jiang@...el.com>
Cc: dan.j.williams@...el.com, jonathan.cameron@...wei.com,
	alison.schofield@...el.com, vishal.l.verma@...el.com,
	ira.weiny@...el.com, fan.ni@...sung.com, a.manzanares@...sung.com,
	sthanneeru.opensrc@...ron.com, emirakhur@...ron.com,
	ajayjoshi@...ron.com, Ravis.OpenSrc@...ron.com,
	sthanneeru@...ron.com, linux-cxl@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] cxl/mbox: support aborting the current background
 operation

On Tue, 14 Jan 2025, Dave Jiang wrote:

>On 10/21/24 8:18 PM, Davidlohr Bueso wrote:
>> CXL 3.1 introduced the ability to request that the current on-going
>> background command be aborted. Add support for this, where the current
>> policy is for the request to occur whenever a new incoming bg command
>> wants to run. As such everything is left to user discretion and it
>> becomes impossible to hog the device/mailbox.
>
>Are you trying to say that the patch is changing the current behavior to where every time a new bg command comes in, it will abort the previous one?

Yes.

>
>>
>> The context of doing the cancellation request is the same as the new
>> incoming command, and will always hold the mbox_mutex, guaranteeing
>> that any successful cancel does not race with a third thread coming
>> in and stealing the effort.
>>
>> - For Sanitize, the thread doing the will cancel the work, and clean
>
>doing the? seems to be missing a word here.

'doing the request', will update.

...

>> +/*
>> + * Return true implies that the request was successful and the on-going
>> + * background operation was in fact aborted. This also guarantees that
>> + * the respective thread is done.
>> + */
>> +static bool cxl_try_to_cancel_background(struct cxl_mailbox *cxl_mbox)
>> +{
>> +	int rc;
>> +	struct cxl_dev_state *cxlds = mbox_to_cxlds(cxl_mbox);
>> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>> +	struct device *dev = cxlds->dev;
>> +	struct cxl_mbox_cmd cmd = {
>> +		.opcode = CXL_MBOX_OP_REQUEST_ABORT_BG_OP
>> +	};
>> +
>> +	lockdep_assert_held(&cxl_mbox->mbox_mutex);
>> +
>> +	rc = __cxl_pci_mbox_send_cmd(cxl_mbox, &cmd);
>> +	if (rc) {
>> +		dev_dbg(dev, "Failed to send abort request : %d\n", rc);
>> +		return false;
>> +	}
>> +
>> +	if (!cxl_mbox_background_complete(cxlds))
>> +		return false;
>> +
>> +	if (mds->security.sanitize_active) {
>> +		/*
>> +		 * Cancel the work and cleanup on its behalf - we hold
>> +		 * the mbox_mutex, cannot race with cxl_mbox_sanitize_work().
>> +		 */
>> +		cancel_delayed_work_sync(&mds->security.poll_dwork);
>> +		mds->security.poll_tmo_secs = 0;
>> +		if (mds->security.sanitize_node)
>> +			sysfs_notify_dirent(mds->security.sanitize_node);
>> +		mds->security.sanitize_active = false;
>
>Should this line happen before the sysfs notification?

It is benign as we hold the lock, but yes. I will also abstract this in a helper,
such that both cxl_mbox_sanitize_work() and cxl_try_to_cancel_background() can
use it.

Thanks,
Davidlohr