| 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: <839aa95e-c3d1-1f60-1b36-20d29445e61f@intel.com>
Date: Fri, 10 Sep 2021 16:38:35 -0700
From: Russ Weight <russell.h.weight@...el.com>
To: Xu Yilun <yilun.xu@...el.com>
CC: <mdf@...nel.org>, <linux-fpga@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <trix@...hat.com>,
<lgoncalv@...hat.com>, <hao.wu@...el.com>,
<matthew.gerlach@...el.com>
Subject: Re: [PATCH v15 6/6] fpga: image-load: enable cancel of image upload
On 9/10/21 7:55 AM, Xu Yilun wrote:
> On Wed, Sep 08, 2021 at 07:18:46PM -0700, Russ Weight wrote:
>> Extend the FPGA Image Load class driver to include a cancel IOCTL that
>> can be used to request that an image upload be canceled. The IOCTL may
>> return EBUSY if it cannot be canceled by software or ENODEV if there
>> is no update in progress.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@...el.com>
>> ---
>> v15:
>> - Compare to previous patch:
>> [PATCH v14 6/6] fpga: sec-mgr: enable cancel of secure update
>> - Changed file, symbol, and config names to reflect the new driver name
>> - Cancel is now initiated by IOCT instead of sysfs
>> - Removed signed-off/reviewed-by tags
>> v14:
>> - Updated ABI documentation date and kernel version
>> v13:
>> - No change
>> v12:
>> - Updated Date and KernelVersion fields in ABI documentation
>> v11:
>> - No change
>> v10:
>> - Rebased to 5.12-rc2 next
>> - Updated Date and KernelVersion in ABI documentation
>> v9:
>> - Updated Date and KernelVersion in ABI documentation
>> v8:
>> - No change
>> v7:
>> - Changed Date in documentation file to December 2020
>> v6:
>> - No change
>> v5:
>> - No change
>> v4:
>> - Changed from "Intel FPGA Security Manager" to FPGA Security Manager"
>> and removed unnecessary references to "Intel".
>> - Changed: iops -> sops, imgr -> smgr, IFPGA_ -> FPGA_, ifpga_ to fpga_
>> v3:
>> - No change
>> v2:
>> - Bumped documentation date and version
>> - Minor code cleanup per review comments
>> ---
>> ---
>> Documentation/fpga/fpga-image-load.rst | 6 ++++
>> drivers/fpga/fpga-image-load.c | 45 +++++++++++++++++++++++---
>> include/linux/fpga/fpga-image-load.h | 1 +
>> include/uapi/linux/fpga-image-load.h | 1 +
>> 4 files changed, 49 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
>> index 3d5eb51223e3..763e7833a6ea 100644
>> --- a/Documentation/fpga/fpga-image-load.rst
>> +++ b/Documentation/fpga/fpga-image-load.rst
>> @@ -37,3 +37,9 @@ FPGA_IMAGE_LOAD_STATUS:
>> Collect status for an on-going image upload. The status returned includes
>> how much data remains to be transferred, the progress of the image load,
>> and error information in the case of a failure.
>> +
>> +FPGA_IMAGE_LOAD_CANCEL:
>> +
>> +Request that a on-going image upload be cancelled. This IOCTL may return
>> +EBUSY if it cannot be cancelled by software or ENODEV if there is no update
>> +in progress.
>> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
>> index 6ec0a39f07b3..c32e4b1ea35a 100644
>> --- a/drivers/fpga/fpga-image-load.c
>> +++ b/drivers/fpga/fpga-image-load.c
>> @@ -46,6 +46,24 @@ static void fpga_image_dev_error(struct fpga_image_load *imgld,
>> imgld->lops->cancel(imgld);
>> }
>>
>> +static int fpga_image_prog_transition(struct fpga_image_load *imgld,
>> + enum fpga_image_prog new_progress)
>> +{
>> + int ret = 0;
>> +
>> + mutex_lock(&imgld->lock);
>> + if (imgld->request_cancel) {
>> + imgld->err_progress = imgld->progress;
>> + imgld->err_code = FPGA_IMAGE_ERR_CANCELED;
>> + imgld->lops->cancel(imgld);
>> + ret = -ECANCELED;
>> + } else {
>> + imgld->progress = new_progress;
>> + }
>> + mutex_unlock(&imgld->lock);
>> + return ret;
>> +}
>> +
>> static void fpga_image_prog_complete(struct fpga_image_load *imgld)
>> {
>> mutex_lock(&imgld->lock);
>> @@ -77,8 +95,10 @@ static void fpga_image_do_load(struct work_struct *work)
>> goto modput_exit;
>> }
>>
>> - fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_WRITING);
>> - while (imgld->remaining_size) {
>> + if (fpga_image_prog_transition(imgld, FPGA_IMAGE_PROG_WRITING))
>> + goto done;
>> +
>> + while (imgld->remaining_size && !imgld->request_cancel) {
>> ret = imgld->lops->write_blk(imgld, offset);
>> if (ret != FPGA_IMAGE_ERR_NONE) {
>> fpga_image_dev_error(imgld, ret);
>> @@ -88,7 +108,9 @@ static void fpga_image_do_load(struct work_struct *work)
>> offset = size - imgld->remaining_size;
>> }
>>
>> - fpga_image_update_progress(imgld, FPGA_IMAGE_PROG_PROGRAMMING);
>> + if (fpga_image_prog_transition(imgld, FPGA_IMAGE_PROG_PROGRAMMING))
>> + goto done;
>> +
>> ret = imgld->lops->poll_complete(imgld);
>> if (ret != FPGA_IMAGE_ERR_NONE)
>> fpga_image_dev_error(imgld, ret);
>> @@ -159,6 +181,7 @@ static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
>> imgld->remaining_size = wb.size;
>> imgld->err_code = FPGA_IMAGE_ERR_NONE;
>> imgld->progress = FPGA_IMAGE_PROG_STARTING;
>> + imgld->request_cancel = false;
>> reinit_completion(&imgld->update_done);
>> schedule_work(&imgld->work);
>> return 0;
>> @@ -189,7 +212,7 @@ static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd,
>> unsigned long arg)
>> {
>> struct fpga_image_load *imgld = filp->private_data;
>> - int ret = -ENOTTY;
>> + int ret = 0;
>>
>> mutex_lock(&imgld->lock);
>>
>> @@ -200,6 +223,17 @@ static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd,
>> case FPGA_IMAGE_LOAD_STATUS:
>> ret = fpga_image_load_ioctl_status(imgld, arg);
>> break;
>> + case FPGA_IMAGE_LOAD_CANCEL:
>> + if (imgld->progress == FPGA_IMAGE_PROG_PROGRAMMING)
>> + ret = -EBUSY;
>> + else if (imgld->progress == FPGA_IMAGE_PROG_IDLE)
>> + ret = -ENODEV;
>> + else
>> + imgld->request_cancel = true;
>> + break;
>> + default:
>> + ret = -ENOTTY;
>> + break;
>> }
>>
>> mutex_unlock(&imgld->lock);
>> @@ -374,6 +408,9 @@ void fpga_image_load_unregister(struct fpga_image_load *imgld)
>> goto unregister;
>> }
>>
>> + if (imgld->progress != FPGA_IMAGE_PROG_PROGRAMMING)
>> + imgld->request_cancel = true;
>> +
> Why we cancel the programing rather than waiting for programing done?
This isn't new - it is the way the security manager was implemented.
Updates can take up to 40 minutes for the N3000. If a person tries
to unload the driver modules, should we hang for 40 minutes? Or should
we try to cancel the update and allow the module to be unloaded?
I think it is reasonable to cancel the update (if possible) under these
circumstances. What do you think?
- Russ
>
> Thanks,
> Yilun
>
>> mutex_unlock(&imgld->lock);
>> wait_for_completion(&imgld->update_done);
>>
>> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
>> index 68f3105b51d2..4e51b9fd1724 100644
>> --- a/include/linux/fpga/fpga-image-load.h
>> +++ b/include/linux/fpga/fpga-image-load.h
>> @@ -52,6 +52,7 @@ struct fpga_image_load {
>> enum fpga_image_prog progress;
>> enum fpga_image_prog err_progress; /* progress at time of failure */
>> enum fpga_image_err err_code; /* image load error code */
>> + bool request_cancel;
>> bool driver_unload;
>> struct eventfd_ctx *finished;
>> void *priv;
>> diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h
>> index 6a995bcc0fb7..8d0dfa1f9b77 100644
>> --- a/include/uapi/linux/fpga-image-load.h
>> +++ b/include/uapi/linux/fpga-image-load.h
>> @@ -39,6 +39,7 @@ enum fpga_image_err {
>>
>> #define FPGA_IMAGE_LOAD_WRITE _IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write)
>> #define FPGA_IMAGE_LOAD_STATUS _IOR(FPGA_IMAGE_LOAD_MAGIC, 1, struct fpga_image_status)
>> +#define FPGA_IMAGE_LOAD_CANCEL _IO(FPGA_IMAGE_LOAD_MAGIC, 2)
>>
>> /**
>> * FPGA_IMAGE_LOAD_WRITE - _IOW(FPGA_IMAGE_LOAD_MAGIC, 0,
>> --
>> 2.25.1
Powered by blists - more mailing lists