| 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
| ||
|
Message-ID: <7e390255-7e55-4b59-9a7d-6aa3857c8a42@intel.com>
Date: Tue, 14 Jan 2025 10:00:38 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Davidlohr Bueso <dave@...olabs.net>, dan.j.williams@...el.com
Cc: 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 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?
>
> 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.
> on behalf of the respective wq callback that will never come.
>
> - For the other bg commands, the sleeping thread is kicked and we
> busy-wait until the polling flag is cleared.
>
> In both scenarios, we guarantee that the aborted op's thread is no
> longer around, giving the new bg op full authority to submit the command.
>
> Semantics for devices that do not support such functionality are left
> unchanged, and hence, with this, the driver benefits in both scenarios.
>
> Signed-off-by: Davidlohr Bueso <dave@...olabs.net>
> ---
> drivers/cxl/cxlmem.h | 1 +
> drivers/cxl/pci.c | 81 +++++++++++++++++++++++++++++++++++++++-----
> 2 files changed, 73 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index b933fb73ef8a..e843ffc3c23a 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -516,6 +516,7 @@ to_cxl_memdev_state(struct cxl_dev_state *cxlds)
> enum cxl_opcode {
> CXL_MBOX_OP_INVALID = 0x0000,
> CXL_MBOX_OP_RAW = CXL_MBOX_OP_INVALID,
> + CXL_MBOX_OP_REQUEST_ABORT_BG_OP = 0x0005,
> CXL_MBOX_OP_GET_EVENT_RECORD = 0x0100,
> CXL_MBOX_OP_CLEAR_EVENT_RECORD = 0x0101,
> CXL_MBOX_OP_GET_EVT_INT_POLICY = 0x0102,
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index f2378604669b..5da50e26e4c4 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -115,8 +115,8 @@ static bool cxl_mbox_background_complete(struct cxl_dev_state *cxlds)
> {
> u64 reg;
>
> - reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
> - return FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_PCT_MASK, reg) == 100;
> + reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_STATUS_OFFSET);
> + return FIELD_GET(CXLDEV_MBOX_STATUS_BG_CMD, reg) == 0;
> }
>
> static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
> @@ -241,7 +241,8 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
> * hardware semantics and only allow device health status.
> */
> if (mds->security.poll_tmo_secs > 0) {
> - if (mbox_cmd->opcode != CXL_MBOX_OP_GET_HEALTH_INFO)
> + if (mbox_cmd->opcode != CXL_MBOX_OP_GET_HEALTH_INFO &&
> + mbox_cmd->opcode != CXL_MBOX_OP_REQUEST_ABORT_BG_OP)
> return -EBUSY;
> }
>
> @@ -335,11 +336,64 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
> return 0;
> }
>
> +/*
> + * 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?
DJ
> +
> + dev_dbg(cxlds->dev, "Sanitization operation aborted\n");
> + } else {
> + /*
> + * Kick the poller and wait for it to be done - no one else
> + * can touch mbox regs. rcuwait_wake_up() provides full
> + * barriers such that wake up occurs before waiting on the
> + * bgpoll atomic to be cleared.
> + */
> + rcuwait_wake_up(&cxl_mbox->mbox_wait);
> + atomic_cond_read_acquire(&cxl_mbox->poll_bgop, !VAL);
> + }
> +
> + return true;
> +}
> +
> static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
> struct cxl_mbox_cmd *cmd)
> {
> 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;
>
> mutex_lock_io(&cxl_mbox->mbox_mutex);
> @@ -348,10 +402,18 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
> * each other: no new bg operation can occur in between while polling.
> */
> if (cxl_is_background_cmd(cmd->opcode)) {
> - if (atomic_read_acquire(&cxl_mbox->poll_bgop)) {
> - mutex_unlock(&cxl_mbox->mbox_mutex);
> - return -EBUSY;
> + if (mds->security.sanitize_active ||
> + atomic_read_acquire(&cxl_mbox->poll_bgop)) {
> + if (!cxl_try_to_cancel_background(cxl_mbox)) {
> + mutex_unlock(&cxl_mbox->mbox_mutex);
> + return -EBUSY;
> + }
> }
> + /*
> + * ... at this point we know that the canceled
> + * bgop context is gone, and we are the _only_
> + * background command in town. Proceed to send it.
> + */
> }
>
> rc = __cxl_pci_mbox_send_cmd(cxl_mbox, cmd);
> @@ -394,10 +456,11 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
> CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
> cmd->return_code = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK,
> bg_status_reg);
> - dev_dbg(dev,
> - "Mailbox background operation (0x%04x) completed\n",
> - cmd->opcode);
> +
> + dev_dbg(dev, "Mailbox background operation (0x%04x) %s\n",
> + cmd->opcode, !cmd->return_code ? "completed":"aborted");
> done:
> + /* ensure clearing poll_bop is the last operation */
> atomic_set_release(&cxl_mbox->poll_bgop, 0);
> }
>
Powered by blists - more mailing lists