linux-kernel - Re: [PATCH] PCI/DOE: Remove asynchronous task support

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: <Y3wC4kX6SCr90FGY@iweiny-desk3>
Date:   Mon, 21 Nov 2022 14:59:46 -0800
From:   Ira Weiny <ira.weiny@...el.com>
To:     "Li, Ming" <ming4.li@...el.com>
CC:     Gregory Price <gregory.price@...verge.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        "Lukas Wunner" <lukas@...ner.de>,
        Alison Schofield <alison.schofield@...el.com>,
        <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-pci@...r.kernel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        "Bjorn Helgaas" <bhelgaas@...gle.com>
Subject: Re: [PATCH] PCI/DOE: Remove asynchronous task support

On Mon, Nov 21, 2022 at 10:07:56AM +0800, Li, Ming wrote:
> On 11/21/2022 9:39 AM, Li, Ming wrote:

[snip]

> >> @@ -529,8 +492,18 @@ int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
> >>  		return -EIO;
> >>  
> >>  	task->doe_mb = doe_mb;
> >> -	INIT_WORK(&task->work, doe_statemachine_work);
> >> -	queue_work(doe_mb->work_queue, &task->work);
> >> +
> >> +again:
> >> +	if (!mutex_trylock(&doe_mb->exec_lock)) {
> >> +		if (wait_event_timeout(task->doe_mb->wq,
> >> +				test_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags),
> >> +				PCI_DOE_POLL_INTERVAL))
> >> +			return -EIO;
> > 
> > We already implemented a pci_doe_wait(), I think we can use it to instead of this wait_event_timeout.
> > 
> > Thanks
> > Ming
> > 
> 
> This wait_event_timeout() only check PCI_DOE_FLAG_CANCEL, that means it only detects the signal which the doe_mb has being destroyed.
> If current doe task is done correctly, I think we should wake up next task. Current implementation just waits utill timeout happens and try it again.
> Besides, If two threads are waiting a same doe_mb, thread #1 waited firstly, thread #2 waited secondly, there is a chance that thread #2 is processed before thread #1.
> 

Agreed.

However, the real problem is that the doe_mb is probably free'ed at this point
and all this is going to crash and burn anyway.  The implementation of
PCI_DOE_FLAG_CANCEL was fundamentally flawed even for the current work queue
implementation.

This patch incorrectly tried to use that mechanism but upon looking closer I
see it does not work.

I saw in another thread Jonathan discussing some sort of get/put on the doe_mb.
That is not currently necessary as the creators of doe_mb objects currently
hold references to the PCI device any time they call submit.

:-(

For now all PCI_DOE_FLAG_CANCEL stuff needs to go away,
Ira

> Thanks
> Ming
> 
> >> +		goto again;
> >> +	}
> >> +	exec_task(task);
> >> +	mutex_unlock(&doe_mb->exec_lock);
> >> +
> >>  	return 0;
> >>  }
> >> -EXPORT_SYMBOL_GPL(pci_doe_submit_task);
> >> +EXPORT_SYMBOL_GPL(pci_doe_submit_task_wait);
> >> diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
> >> index ed9b4df792b8..c94122a66221 100644
> >> --- a/include/linux/pci-doe.h
> >> +++ b/include/linux/pci-doe.h
> >> @@ -30,8 +30,6 @@ struct pci_doe_mb;
> >>   * @response_pl_sz: Size of the response payload (bytes)
> >>   * @rv: Return value.  Length of received response or error (bytes)
> >>   * @complete: Called when task is complete
> >> - * @private: Private data for the consumer
> >> - * @work: Used internally by the mailbox
> >>   * @doe_mb: Used internally by the mailbox
> >>   *
> >>   * The payload sizes and rv are specified in bytes with the following
> >> @@ -50,11 +48,6 @@ struct pci_doe_task {
> >>  	u32 *response_pl;
> >>  	size_t response_pl_sz;
> >>  	int rv;
> >> -	void (*complete)(struct pci_doe_task *task);
> >> -	void *private;
> >> -
> >> -	/* No need for the user to initialize these fields */
> >> -	struct work_struct work;
> >>  	struct pci_doe_mb *doe_mb;
> >>  };
> >>  
> >> @@ -72,6 +65,5 @@ struct pci_doe_task {
> >>  
> >>  struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset);
> >>  bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type);
> >> -int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task);
> >> -
> >> +int pci_doe_submit_task_wait(struct pci_doe_mb *doe_mb, struct pci_doe_task *task);
> >>  #endif
> >>
> >> base-commit: b6e7fdfd6f6a8bf88fcdb4a45da52c42ba238c25