Re: [PATCH 0/3] scsi: arcmsr: Add driver parameter cmd_timeout for scsi command timeout setting
On Tue, 2018-05-08 at 14:32 +0800, Ching Huang wrote: > On Tue, 2018-05-08 at 01:41 -0400, Martin K. Petersen wrote: > > Hello Ching, > > > > > 1. Add driver parameter cmd_timeout, default value is > > > ARCMSR_DEFAULT_TIMEOUT. > > > 2. Add slave_configure callback function to set device command timeout > > > value. > > > 3. Update driver version to v1.40.00.06-20180504. > > > > I am not so keen on arcmsr overriding the timeout set by the admin or > > application. > > > > Also, instead of introducing this module parameter, why not simply ask > > the user to change rq_timeout? > > > This timeout setting only after device has been inquiry successfully. > Of course, user can set timeout value to /sys/block/sdX/device/timeout. > But user does not like to set this value once command timeout occurred. > They rather like timeout never happen. > This timeout setting apply to all devices, its better than user has to set one bye one for each device.
Re: [PATCH 0/3] scsi: arcmsr: Add driver parameter cmd_timeout for scsi command timeout setting
On Tue, 2018-05-08 at 01:41 -0400, Martin K. Petersen wrote: > Hello Ching, > > > 1. Add driver parameter cmd_timeout, default value is > > ARCMSR_DEFAULT_TIMEOUT. > > 2. Add slave_configure callback function to set device command timeout > > value. > > 3. Update driver version to v1.40.00.06-20180504. > > I am not so keen on arcmsr overriding the timeout set by the admin or > application. > > Also, instead of introducing this module parameter, why not simply ask > the user to change rq_timeout? > This timeout setting only after device has been inquiry successfully. Of course, user can set timeout value to /sys/block/sdX/device/timeout. But user does not like to set this value once command timeout occurred. They rather like timeout never happen.
Re: [RESEND V2 1/6] tcmu: add new netlink events helpers
Mike, > Thanks. Patchset looks ok to me. > > Acked-by: Mike Christie Applied this to 4.18/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v2] target/file: add support of direct and async I/O
Bryant, > Can you review this patch and pull it into scsi since Nicholas has > been out for awhile? > > I have tested this patch and really like the performance enhancements > as a result of it. No problem queuing it up if I get an ACK from Christoph or Mike. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 0/3] scsi: arcmsr: Add driver parameter cmd_timeout for scsi command timeout setting
Hello Ching, > 1. Add driver parameter cmd_timeout, default value is ARCMSR_DEFAULT_TIMEOUT. > 2. Add slave_configure callback function to set device command timeout value. > 3. Update driver version to v1.40.00.06-20180504. I am not so keen on arcmsr overriding the timeout set by the admin or application. Also, instead of introducing this module parameter, why not simply ask the user to change rq_timeout? -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v2] scsi: 3w-9xxx: fix a missing-check bug
Wenwen, > In twa_chrdev_ioctl(), the ioctl driver command is firstly copied from the > userspace pointer 'argp' and saved to the kernel object 'driver_command'. > Then a security check is performed on the data buffer size indicated by > 'driver_command', which is 'driver_command.buffer_length'. If the security > check is passed, the entire ioctl command is copied again from the 'argp' > pointer and saved to the kernel object 'tw_ioctl'. Then, various operations > are performed on 'tw_ioctl' according to the 'cmd'. Given that the 'argp' > pointer resides in userspace, a malicious userspace process can race to > change the buffer size between the two copies. This way, the user can > bypass the security check and inject invalid data buffer size. This can > cause potential security issues in the following execution. > > This patch checks for capable(CAP_SYS_ADMIN) in twa_chrdev_open()t o avoid > the above issues. Applied patch 1 + 2 to 4.18/scsi-queue. Thank you. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 1/2] scsi: mpt3sas: remove obsolete path "drivers/scsi/mpt2sas/" from MAINTAINERS
Tomohiro,
> drivers/scsi/mpt2sas/ no longer exists after
> c84b06a48c ("mpt3sas: Single driver module which supports both
> SAS 2.0 & SAS 3.0 HBAs") merged/removed it.
Applied patches 1 + 2 to 4.18/scsi-queue. Thanks!
--
Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: megaraid: silence a static checker bug
Dan, > If we had more than 32 megaraid cards then it would cause memory > corruption. That's not likely, of course, but it's handy to enforce it > and make the static checker happy. Applied to 4.18/scsi-queue. Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH][V2] scsi: lpfc: fix spelling mistakes: "mabilbox" and "maibox"
Colin, > Trivial fix to spelling mistakes in lpfc_printf_log log message > > "mabilbox" -> "mailbox" > "maibox" -> "mailbox" Applied to 4.18/scsi-queue. Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: mptsas: fix spelling mistake: "matchs" -> "matches"
Colin, > Trivial fix to spelling mistake in warning message Applied to 4.18/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: qla2xxx: remove the unused tcm_qla2xxx_cmd_wq
Andrei, Applied to 4.18/scsi-queue. thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: mptfusion: fix spelling mistake: "initators" -> "initiators"
Colin, > Trivial fix to spelling mistake in text string Applied to 4.18/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] mptlan: fix mpt_lan_sdu_send()'s return type
Luc, > The method ndo_start_xmit() is defined as returning an 'netdev_tx_t', > which is a typedef for an enum type, but the implementation in this > driver returns an 'int'. You forgot to update the function prototype accordingly. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 06/10] lpfc: Check if SCSI scanning is complete, before allowing I/O commands
James,
Not a fan of these:
> +static int
> +lpfc_is_scan_cmd(uint8_t opcode)
> +{
> + switch (opcode & 0x1f) {
> + case LPFC_INQUIRY_CMD_CODE:
> + case LPFC_LOG_SELECT_CMD_CODE:
> + case LPFC_LOG_SENSE_CMD_CODE:
Especially since they are masked off twice:
> +#define LPFC_INQUIRY_CMD_CODE(INQUIRY & 0x1f)
> +#define LPFC_LOG_SELECT_CMD_CODE (LOG_SELECT & 0x1f)
> +#define LPFC_LOG_SENSE_CMD_CODE (LOG_SENSE &
> 0x1f)
--
Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 00/10] lpfc updates for 12.0.0.3
James, > This patch contains lpfc bug fixes and a few message updates and > cleanups. Applied to 4.18/scsi-queue except for patch #6. -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 00/11] hisi_sas: some misc patches
John, > This patchset introduces some misc changes for the > driver. These include: > - fixes for potential problems related to SCSI EH > and device removal races > - fix protection info size for all hw versions > - some SoC bug workarounds > - minor optimisations > - other more minor things Applied to 4.18/scsi-queue. Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 00/12] qla2xxx: Updates for driver
Himanshu, > Please apply this to 4.18 scsi-misc branch at your earliest > convenience. Applied patches 2 through 12 to 4.18/scsi-queue. Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 00/21] qedf: Update driver to 8.33.16.20.
Chad, > Please add these patches to your tree for the next kernel merge window > at your convenience. Applied to 4.18/scsi-queue with the zeroday tweak rolled in. Thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v3 00/14] mpt3sas: Enhancements and Defect fixes.
Sreekanth, > Any update on this patch set. I applied it to 4.18/scsi-queue earlier today. -- Martin K. Petersen Oracle Linux Engineering
RE: [PATCH v3 00/14] mpt3sas: Enhancements and Defect fixes.
Hi, Any update on this patch set. Thanks, Sreekanth -Original Message- From: Chaitra P B [mailto:chaitra.basa...@broadcom.com] Sent: Tuesday, April 24, 2018 2:58 PM To: linux-scsi@vger.kernel.org Cc: sathya.prak...@broadcom.com; sreekanth.re...@broadcom.com; suganath-prabu.subram...@broadcom.com; Chaitra P B Subject: [PATCH v3 00/14] mpt3sas: Enhancements and Defect fixes. Chaitra P B (14): mpt3sas: Bug fix for big endian systems. mpt3sas: Pre-allocate RDPQ Array at driver boot time. mpt3sas: Lockless access for chain buffers. mpt3sas: Optimize I/O memory consumption in driver. mpt3sas: Enhanced handling of Sense Buffer. mpt3sas: Added support for SAS Device Discovery Error Event. mpt3sas: Increase event log buffer to support 24 port HBA's. mpt3sas: Allow processing of events during driver unload. mpt3sas: Cache enclosure pages during enclosure add. mpt3sas: Report Firmware Package Version from HBA Driver. mpt3sas: Update MPI Headers mpt3sas: For NVME device, issue a protocol level reset instead of hot reset and use TM timeout value exposed in PCIe Device Page 2. mpt3sas: fix possible memory leak. mpt3sas: Update driver version "25.100.00.00" drivers/scsi/mpt3sas/mpi/mpi2.h | 9 +- drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h | 30 +- drivers/scsi/mpt3sas/mpi/mpi2_init.h | 2 +- drivers/scsi/mpt3sas/mpi/mpi2_ioc.h | 7 +- drivers/scsi/mpt3sas/mpt3sas_base.c | 475 +++--- drivers/scsi/mpt3sas/mpt3sas_base.h | 60 +++- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 33 ++- drivers/scsi/mpt3sas/mpt3sas_ctl.h | 2 +- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 491 +-- drivers/scsi/mpt3sas/mpt3sas_warpdrive.c | 3 +- 10 files changed, 816 insertions(+), 296 deletions(-) -- 1.8.3.1
Re: [PATCH] zfcp: fix infinite iteration on ERP ready list
Steffen, > zfcp_erp_adapter_reopen() schedules blocking of all of the adapter's > rports via zfcp_scsi_schedule_rports_block() and enqueues a reopen > adapter ERP action via zfcp_erp_action_enqueue(). Both are separately > processed asynchronously and concurrently. Applied to 4.17/scsi-fixes. Thank you! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v2 2/2] scsi: ufs: Use freq table with devfreq
Hi,
On 2018년 05월 05일 07:44, Bjorn Andersson wrote:
> devfreq requires that the client operates on actual frequencies, not
> only 0 and UMAX_INT and as such UFS brok with the introduction of
> f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency").
>
> This patch registers the frequencies of the first clock as opp levels
> and use these to determine if we're trying to step up or down.
>
> Signed-off-by: Bjorn Andersson
> ---
>
> Chances since v1:
> - Register min_freq and max_freq as opp levels.
> - Unregister opp levels on removal, to make e.g. probe defer working
>
> drivers/scsi/ufs/ufshcd.c | 47 +--
> 1 file changed, 40 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 2253f24309ec..257614b889bd 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1168,16 +1168,13 @@ static int ufshcd_devfreq_target(struct device *dev,
> struct ufs_hba *hba = dev_get_drvdata(dev);
> ktime_t start;
> bool scale_up, sched_clk_scaling_suspend_work = false;
> + struct list_head *clk_list = &hba->clk_list_head;
> + struct ufs_clk_info *clki;
> unsigned long irq_flags;
>
> if (!ufshcd_is_clkscaling_supported(hba))
> return -EINVAL;
>
> - if ((*freq > 0) && (*freq < UINT_MAX)) {
> - dev_err(hba->dev, "%s: invalid freq = %lu\n", __func__, *freq);
> - return -EINVAL;
> - }
> -
> spin_lock_irqsave(hba->host->host_lock, irq_flags);
> if (ufshcd_eh_in_progress(hba)) {
> spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
> @@ -1187,7 +1184,13 @@ static int ufshcd_devfreq_target(struct device *dev,
> if (!hba->clk_scaling.active_reqs)
> sched_clk_scaling_suspend_work = true;
>
> - scale_up = (*freq == UINT_MAX) ? true : false;
> + if (list_empty(clk_list)) {
> + spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
> + goto out;
> + }
> +
> + clki = list_first_entry(clk_list, struct ufs_clk_info, list);
> + scale_up = (*freq == clki->max_freq) ? true : false;
> if (!ufshcd_is_devfreq_scaling_required(hba, scale_up)) {
> spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
> ret = 0;
> @@ -1257,16 +1260,29 @@ static struct devfreq_dev_profile ufs_devfreq_profile
> = {
>
> static int ufshcd_devfreq_init(struct ufs_hba *hba)
> {
> + struct list_head *clk_list = &hba->clk_list_head;
> + struct ufs_clk_info *clki;
> struct devfreq *devfreq;
> int ret;
>
> - devfreq = devm_devfreq_add_device(hba->dev,
> + /* Skip devfreq if we don't have any clocks in the list */
> + if (list_empty(clk_list))
> + return 0;
> +
> + clki = list_first_entry(clk_list, struct ufs_clk_info, list);
> + dev_pm_opp_add(hba->dev, clki->min_freq, 0);
> + dev_pm_opp_add(hba->dev, clki->max_freq, 0);
> +
> + devfreq = devfreq_add_device(hba->dev,
> &ufs_devfreq_profile,
> "simple_ondemand",
> NULL);
> if (IS_ERR(devfreq)) {
> ret = PTR_ERR(devfreq);
> dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
> +
> + dev_pm_opp_remove(hba->dev, clki->min_freq);
> + dev_pm_opp_remove(hba->dev, clki->max_freq);
> return ret;
> }
>
> @@ -1275,6 +1291,22 @@ static int ufshcd_devfreq_init(struct ufs_hba *hba)
> return 0;
> }
>
> +static void ufshcd_devfreq_remove(struct ufs_hba *hba)
> +{
> + struct list_head *clk_list = &hba->clk_list_head;
> + struct ufs_clk_info *clki;
> +
> + if (!hba->devfreq)
> + return;
> +
> + devfreq_remove_device(hba->devfreq);
> + hba->devfreq = NULL;
> +
> + clki = list_first_entry(clk_list, struct ufs_clk_info, list);
> + dev_pm_opp_remove(hba->dev, clki->min_freq);
> + dev_pm_opp_remove(hba->dev, clki->max_freq);
> +}
> +
> static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
> {
> unsigned long flags;
> @@ -6966,6 +6998,7 @@ static void ufshcd_hba_exit(struct ufs_hba *hba)
> if (hba->devfreq)
> ufshcd_suspend_clkscaling(hba);
> destroy_workqueue(hba->clk_scaling.workq);
> + ufshcd_devfreq_remove(hba);
> }
> ufshcd_setup_clocks(hba, false);
> ufshcd_setup_hba_vreg(hba, false);
>
For using OPP entries for devfreq:
Reviewed-by: Chanwoo Choi
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Re: [PATCH v2 1/2] scsi: ufs: Extract devfreq registration
Hi,
On 2018년 05월 05일 07:44, Bjorn Andersson wrote:
> Failing to register with devfreq leaves hba->devfreq assigned, which
> causes the error path to dereference the ERR_PTR(). Rather than bolting
> on more conditionals, move the call of devm_devfreq_add_device() into
> it's own function and only update hba->devfreq once it's successfully
> registered.
>
> The subsequent patch builds upon this to make UFS actually work again,
> as it's been broken since f1d981eaecf8 ("PM / devfreq: Use the available
> min/max frequency")
>
> Reviewed-by: Subhash Jadavani
> Signed-off-by: Bjorn Andersson
> ---
>
> Changes since v1:
> - None
>
> drivers/scsi/ufs/ufshcd.c | 31 ++-
> 1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 8f22a980b1a7..2253f24309ec 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1255,6 +1255,26 @@ static struct devfreq_dev_profile ufs_devfreq_profile
> = {
> .get_dev_status = ufshcd_devfreq_get_dev_status,
> };
>
> +static int ufshcd_devfreq_init(struct ufs_hba *hba)
> +{
> + struct devfreq *devfreq;
> + int ret;
> +
> + devfreq = devm_devfreq_add_device(hba->dev,
> + &ufs_devfreq_profile,
> + "simple_ondemand",
You better to use 'DEVFREQ_GOV_SIMPLE_ONDEMAND' definition
in include/linux/devfreq.h in order to prevent the some typo.
- aa7c352f9841 ("PM / devfreq: Define the constant governor name")
> + NULL);
> + if (IS_ERR(devfreq)) {
> + ret = PTR_ERR(devfreq);
> + dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
> + return ret;
> + }
> +
> + hba->devfreq = devfreq;
> +
> + return 0;
> +}
> +
> static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
> {
> unsigned long flags;
> @@ -6399,16 +6419,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
> sizeof(struct ufs_pa_layer_attr));
> hba->clk_scaling.saved_pwr_info.is_valid = true;
> if (!hba->devfreq) {
> - hba->devfreq = devm_devfreq_add_device(hba->dev,
> - &ufs_devfreq_profile,
> - "simple_ondemand",
> - NULL);
> - if (IS_ERR(hba->devfreq)) {
> - ret = PTR_ERR(hba->devfreq);
> - dev_err(hba->dev, "Unable to register
> with devfreq %d\n",
> - ret);
> + ret = ufshcd_devfreq_init(hba);
> + if (ret)
> goto out;
> - }
> }
> hba->clk_scaling.is_allowed = true;
> }
>
Reviewed-by: Chanwoo Choi
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Re: [PATCH v2] scsi: 3w-xxxx: fix a missing-check bug
On Mon, May 7, 2018 at 5:54 PM, Wenwen Wang wrote: > In tw_chrdev_ioctl(), the length of the data buffer is firstly copied from > the userspace pointer 'argp' and saved to the kernel object > 'data_buffer_length'. Then a security check is performed on it to make sure > that the length is not more than 'TW_MAX_IOCTL_SECTORS * 512'. Otherwise, > an error code -EINVAL is returned. If the security check is passed, the > entire ioctl command is copied again from the 'argp' pointer and saved to > the kernel object 'tw_ioctl'. Then, various operations are performed on > 'tw_ioctl' according to the 'cmd'. Given that the 'argp' pointer resides in > userspace, a malicious userspace process can race to change the buffer > length between the two copies. This way, the user can bypass the security > check and inject invalid data buffer length. This can cause potential > security issues in the following execution. > > This patch checks for capable(CAP_SYS_ADMIN) in tw_chrdev_open() to avoid > the above issues. > > Signed-off-by: Wenwen Wang > --- > drivers/scsi/3w-.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/scsi/3w-.c b/drivers/scsi/3w-.c > index 33261b6..f6179e3 100644 > --- a/drivers/scsi/3w-.c > +++ b/drivers/scsi/3w-.c > @@ -1033,6 +1033,9 @@ static int tw_chrdev_open(struct inode *inode, struct > file *file) > > dprintk(KERN_WARNING "3w-: tw_ioctl_open()\n"); > > + if (!capable(CAP_SYS_ADMIN)) > + return -EACCES; > + > minor_number = iminor(inode); > if (minor_number >= tw_device_extension_count) > return -ENODEV; > -- > 2.7.4 > Acked-by: Adam Radford
Re: [PATCH v2] scsi: 3w-9xxx: fix a missing-check bug
On Mon, May 7, 2018 at 5:46 PM, Wenwen Wang wrote:
> In twa_chrdev_ioctl(), the ioctl driver command is firstly copied from the
> userspace pointer 'argp' and saved to the kernel object 'driver_command'.
> Then a security check is performed on the data buffer size indicated by
> 'driver_command', which is 'driver_command.buffer_length'. If the security
> check is passed, the entire ioctl command is copied again from the 'argp'
> pointer and saved to the kernel object 'tw_ioctl'. Then, various operations
> are performed on 'tw_ioctl' according to the 'cmd'. Given that the 'argp'
> pointer resides in userspace, a malicious userspace process can race to
> change the buffer size between the two copies. This way, the user can
> bypass the security check and inject invalid data buffer size. This can
> cause potential security issues in the following execution.
>
> This patch checks for capable(CAP_SYS_ADMIN) in twa_chrdev_open()t o avoid
> the above issues.
>
> Signed-off-by: Wenwen Wang
> ---
> drivers/scsi/3w-9xxx.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
> index b42c9c4..99ba4a7 100644
> --- a/drivers/scsi/3w-9xxx.c
> +++ b/drivers/scsi/3w-9xxx.c
> @@ -882,6 +882,11 @@ static int twa_chrdev_open(struct inode *inode, struct
> file *file)
> unsigned int minor_number;
> int retval = TW_IOCTL_ERROR_OS_ENODEV;
>
> + if (!capable(CAP_SYS_ADMIN)) {
> + retval = -EACCES;
> + goto out;
> + }
> +
> minor_number = iminor(inode);
> if (minor_number >= twa_device_extension_count)
> goto out;
> --
> 2.7.4
>
Acked-by: Adam Radford
[PATCH v2] scsi: 3w-xxxx: fix a missing-check bug
In tw_chrdev_ioctl(), the length of the data buffer is firstly copied from the userspace pointer 'argp' and saved to the kernel object 'data_buffer_length'. Then a security check is performed on it to make sure that the length is not more than 'TW_MAX_IOCTL_SECTORS * 512'. Otherwise, an error code -EINVAL is returned. If the security check is passed, the entire ioctl command is copied again from the 'argp' pointer and saved to the kernel object 'tw_ioctl'. Then, various operations are performed on 'tw_ioctl' according to the 'cmd'. Given that the 'argp' pointer resides in userspace, a malicious userspace process can race to change the buffer length between the two copies. This way, the user can bypass the security check and inject invalid data buffer length. This can cause potential security issues in the following execution. This patch checks for capable(CAP_SYS_ADMIN) in tw_chrdev_open() to avoid the above issues. Signed-off-by: Wenwen Wang --- drivers/scsi/3w-.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/scsi/3w-.c b/drivers/scsi/3w-.c index 33261b6..f6179e3 100644 --- a/drivers/scsi/3w-.c +++ b/drivers/scsi/3w-.c @@ -1033,6 +1033,9 @@ static int tw_chrdev_open(struct inode *inode, struct file *file) dprintk(KERN_WARNING "3w-: tw_ioctl_open()\n"); + if (!capable(CAP_SYS_ADMIN)) + return -EACCES; + minor_number = iminor(inode); if (minor_number >= tw_device_extension_count) return -ENODEV; -- 2.7.4
[PATCH v2] scsi: 3w-9xxx: fix a missing-check bug
In twa_chrdev_ioctl(), the ioctl driver command is firstly copied from the
userspace pointer 'argp' and saved to the kernel object 'driver_command'.
Then a security check is performed on the data buffer size indicated by
'driver_command', which is 'driver_command.buffer_length'. If the security
check is passed, the entire ioctl command is copied again from the 'argp'
pointer and saved to the kernel object 'tw_ioctl'. Then, various operations
are performed on 'tw_ioctl' according to the 'cmd'. Given that the 'argp'
pointer resides in userspace, a malicious userspace process can race to
change the buffer size between the two copies. This way, the user can
bypass the security check and inject invalid data buffer size. This can
cause potential security issues in the following execution.
This patch checks for capable(CAP_SYS_ADMIN) in twa_chrdev_open()t o avoid
the above issues.
Signed-off-by: Wenwen Wang
---
drivers/scsi/3w-9xxx.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
index b42c9c4..99ba4a7 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -882,6 +882,11 @@ static int twa_chrdev_open(struct inode *inode, struct
file *file)
unsigned int minor_number;
int retval = TW_IOCTL_ERROR_OS_ENODEV;
+ if (!capable(CAP_SYS_ADMIN)) {
+ retval = -EACCES;
+ goto out;
+ }
+
minor_number = iminor(inode);
if (minor_number >= twa_device_extension_count)
goto out;
--
2.7.4
Re: [PATCH] scsi: 3w-xxxx: fix a missing-check bug
On Sat, May 5, 2018 at 10:48 PM, Wenwen Wang wrote:
> In tw_chrdev_ioctl(), the length of the data buffer is firstly copied from
> the userspace pointer 'argp' and saved to the kernel object
> 'data_buffer_length'. Then a security check is performed on it to make sure
> that the length is not more than 'TW_MAX_IOCTL_SECTORS * 512'. Otherwise,
> an error code -EINVAL is returned. If the security check is passed, the
> entire ioctl command is copied again from the 'argp' pointer and saved to
> the kernel object 'tw_ioctl'. Then, various operations are performed on
> 'tw_ioctl' according to the 'cmd'. Given that the 'argp' pointer resides in
> userspace, a malicious userspace process can race to change the buffer
> length between the two copies. This way, the user can bypass the security
> check and inject invalid data buffer length. This can cause potential
> security issues in the following execution.
>
> This patch checks the buffer length obtained in the second copy. An error
> code -EINVAL will be returned if it is not same as the original one in the
> first copy.
>
> Signed-off-by: Wenwen Wang
> ---
> drivers/scsi/3w-.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/scsi/3w-.c b/drivers/scsi/3w-.c
> index 33261b6..ef79194 100644
> --- a/drivers/scsi/3w-.c
> +++ b/drivers/scsi/3w-.c
> @@ -919,6 +919,10 @@ static long tw_chrdev_ioctl(struct file *file, unsigned
> int cmd, unsigned long a
> /* Now copy down the entire ioctl */
> if (copy_from_user(tw_ioctl, argp, data_buffer_length +
> sizeof(TW_New_Ioctl) - 1))
> goto out2;
> + if (tw_ioctl->data_buffer_length != data_buffer_length) {
> + retval = -EINVAL;
> + goto out2;
> + }
>
> passthru = (TW_Passthru *)&tw_ioctl->firmware_command;
>
> --
> 2.7.4
>
I would drop this patch and check for !capable(CAP_SYS_ADMIN) in
tw_chrdev_open() instead.
-Adam
Re: [PATCH] scsi: 3w-9xxx: fix a missing-check bug
On Sat, May 5, 2018 at 8:43 PM, Wenwen Wang wrote:
> In twa_chrdev_ioctl(), the ioctl driver command is firstly copied from the
> userspace pointer 'argp' and saved to the kernel object 'driver_command'.
> Then a security check is performed on the data buffer size indicated by
> 'driver_command', which is 'driver_command.buffer_length'. If the security
> check is passed, the entire ioctl command is copied again from the 'argp'
> pointer and saved to the kernel object 'tw_ioctl'. Then, various operations
> are performed on 'tw_ioctl' according to the 'cmd'. Given that the 'argp'
> pointer resides in userspace, a malicious userspace process can race to
> change the buffer size between the two copies. This way, the user can
> bypass the security check and inject invalid data buffer size. This can
> cause potential security issues in the following execution.
>
> This patch checks the buffer size obtained in the second copy. An error
> code -EINVAL will be returned if it is not same as the original one in the
> first copy.
>
> Signed-off-by: Wenwen Wang
> ---
> drivers/scsi/3w-9xxx.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
> index b42c9c4..8bc43db 100644
> --- a/drivers/scsi/3w-9xxx.c
> +++ b/drivers/scsi/3w-9xxx.c
> @@ -684,6 +684,12 @@ static long twa_chrdev_ioctl(struct file *file, unsigned
> int cmd, unsigned long
> if (copy_from_user(tw_ioctl, argp, driver_command.buffer_length +
> sizeof(TW_Ioctl_Buf_Apache) - 1))
> goto out3;
>
> + if (tw_ioctl->driver_command.buffer_length
> +!= driver_command.buffer_length) {
> + retval = TW_IOCTL_ERROR_OS_EINVAL;
> + goto out3;
> + }
> +
> /* See which ioctl we are doing */
> switch (cmd) {
> case TW_IOCTL_FIRMWARE_PASS_THROUGH:
> --
> 2.7.4
>
Drop this patch and create a new one that checks for: if
!capable(CAP_SYS_ADMIN) in twa_chrdev_ioctl() (like 3w-sas.c does) and
I'll ack it.
-Adam
Re: [PATCH] scsi: 3ware: fix a missing-check bug
On Sat, May 5, 2018 at 10:50 PM, Wenwen Wang wrote:
> In twl_chrdev_ioctl(), the ioctl driver command is firstly copied from the
> userspace pointer 'argp' and saved to the kernel object 'driver_command'.
> Then a security check is performed on the data buffer size indicated by
> 'driver_command', which is 'driver_command.buffer_length'. If the security
> check is passed, the entire ioctl command is copied again from the 'argp'
> pointer and saved to the kernel object 'tw_ioctl'. Then, various operations
> are performed on 'tw_ioctl' according to the 'cmd'. Given that the 'argp'
> pointer resides in userspace, a malicious userspace process can race to
> change the buffer size between the two copies. This way, the user can
> bypass the security check and inject invalid data buffer size. This can
> cause potential security issues in the following execution.
>
> This patch checks the buffer size obtained in the second copy. An error
> code -EINVAL will be returned if it is not same as the original one in the
> first copy.
>
> Signed-off-by: Wenwen Wang
> ---
> drivers/scsi/3w-sas.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/scsi/3w-sas.c b/drivers/scsi/3w-sas.c
> index cf9f2a0..ea41969 100644
> --- a/drivers/scsi/3w-sas.c
> +++ b/drivers/scsi/3w-sas.c
> @@ -757,6 +757,11 @@ static long twl_chrdev_ioctl(struct file *file, unsigned
> int cmd, unsigned long
> /* Now copy down the entire ioctl */
> if (copy_from_user(tw_ioctl, argp, driver_command.buffer_length +
> sizeof(TW_Ioctl_Buf_Apache) - 1))
> goto out3;
> + if (tw_ioctl->driver_command.buffer_length !=
> + driver_command.buffer_length) {
> + retval = -EINVAL;
> + goto out3;
> + }
>
> /* See which ioctl we are doing */
> switch (cmd) {
> --
> 2.7.4
>
1. Returning -EINVAL after the copy_from_user() doesn't prevent any
invalid copy down to kernel mode from happening.
2. twl_chrdev_open() checks for capable(CAP_SYS_ADMIN):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/3w-sas.c#n834
I don't see the point in this patch.
-Adam
Re: [PATCH v2] target/file: add support of direct and async I/O
Hi Martin,
Can you review this patch and pull it into scsi since Nicholas has
been out for awhile?
I have tested this patch and really like the performance enhancements
as a result of it.
Thanks,
Bryant
On 4/19/18 1:29 PM, Andrei Vagin wrote:
> Hello Nicholas,
>
> What do you think about this patch?
>
> Thanks,
> Andrei
>
> On Wed, Mar 21, 2018 at 11:55:02PM -0700, Andrei Vagin wrote:
>> There are two advantages:
>> * Direct I/O allows to avoid the write-back cache, so it reduces
>> affects to other processes in the system.
>> * Async I/O allows to handle a few commands concurrently.
>>
>> DIO + AIO shows a better perfomance for random write operations:
>>
>> Mode: O_DSYNC Async: 1
>> $ ./fio --bs=4K --direct=1 --rw=randwrite --ioengine=libaio --iodepth=64
>> --name=/dev/sda --runtime=20 --numjobs=2
>> WRITE: bw=45.9MiB/s (48.1MB/s), 21.9MiB/s-23.0MiB/s (22.0MB/s-25.2MB/s),
>> io=919MiB (963MB), run=20002-20020msec
>>
>> Mode: O_DSYNC Async: 0
>> $ ./fio --bs=4K --direct=1 --rw=randwrite --ioengine=libaio --iodepth=64
>> --name=/dev/sdb --runtime=20 --numjobs=2
>> WRITE: bw=1607KiB/s (1645kB/s), 802KiB/s-805KiB/s (821kB/s-824kB/s),
>> io=31.8MiB (33.4MB), run=20280-20295msec
>>
>> Known issue:
>>
>> DIF (PI) emulation doesn't work when a target uses async I/O, because
>> DIF metadata is saved in a separate file, and it is another non-trivial
>> task how to synchronize writing in two files, so that a following read
>> operation always returns a consisten metadata for a specified block.
>>
>> v2: fix comments from Christoph Hellwig
>>
>> Cc: "Nicholas A. Bellinger"
>> Cc: Christoph Hellwig
>> Cc: Bryant G. Ly
>> Tested-by: Bryant G. Ly
>> Signed-off-by: Andrei Vagin
>> ---
>> drivers/target/target_core_file.c | 137
>> ++
>> drivers/target/target_core_file.h | 1 +
>> 2 files changed, 124 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/target/target_core_file.c
>> b/drivers/target/target_core_file.c
>> index 9b2c0c773022..16751ae55d7b 100644
>> --- a/drivers/target/target_core_file.c
>> +++ b/drivers/target/target_core_file.c
>> @@ -250,6 +250,84 @@ static void fd_destroy_device(struct se_device *dev)
>> }
>> }
>>
>> +struct target_core_file_cmd {
>> +unsigned long len;
>> +struct se_cmd *cmd;
>> +struct kiocbiocb;
>> +};
>> +
>> +static void cmd_rw_aio_complete(struct kiocb *iocb, long ret, long ret2)
>> +{
>> +struct target_core_file_cmd *cmd;
>> +
>> +cmd = container_of(iocb, struct target_core_file_cmd, iocb);
>> +
>> +if (ret != cmd->len)
>> +target_complete_cmd(cmd->cmd, SAM_STAT_CHECK_CONDITION);
>> +else
>> +target_complete_cmd(cmd->cmd, SAM_STAT_GOOD);
>> +
>> +kfree(cmd);
>> +}
>> +
>> +static sense_reason_t
>> +fd_execute_rw_aio(struct se_cmd *cmd, struct scatterlist *sgl, u32
>> sgl_nents,
>> + enum dma_data_direction data_direction)
>> +{
>> +int is_write = !(data_direction == DMA_FROM_DEVICE);
>> +struct se_device *dev = cmd->se_dev;
>> +struct fd_dev *fd_dev = FD_DEV(dev);
>> +struct file *file = fd_dev->fd_file;
>> +struct target_core_file_cmd *aio_cmd;
>> +struct iov_iter iter = {};
>> +struct scatterlist *sg;
>> +struct bio_vec *bvec;
>> +ssize_t len = 0;
>> +int ret = 0, i;
>> +
>> +aio_cmd = kmalloc(sizeof(struct target_core_file_cmd), GFP_KERNEL);
>> +if (!aio_cmd)
>> +return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>> +
>> +bvec = kcalloc(sgl_nents, sizeof(struct bio_vec), GFP_KERNEL);
>> +if (!bvec) {
>> +kfree(aio_cmd);
>> +return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>> +}
>> +
>> +for_each_sg(sgl, sg, sgl_nents, i) {
>> +bvec[i].bv_page = sg_page(sg);
>> +bvec[i].bv_len = sg->length;
>> +bvec[i].bv_offset = sg->offset;
>> +
>> +len += sg->length;
>> +}
>> +
>> +iov_iter_bvec(&iter, ITER_BVEC | is_write, bvec, sgl_nents, len);
>> +
>> +aio_cmd->cmd = cmd;
>> +aio_cmd->len = len;
>> +aio_cmd->iocb.ki_pos = cmd->t_task_lba * dev->dev_attrib.block_size;
>> +aio_cmd->iocb.ki_filp = file;
>> +aio_cmd->iocb.ki_complete = cmd_rw_aio_complete;
>> +aio_cmd->iocb.ki_flags = IOCB_DIRECT;
>> +
>> +if (is_write && (cmd->se_cmd_flags & SCF_FUA))
>> +aio_cmd->iocb.ki_flags |= IOCB_DSYNC;
>> +
>> +if (is_write)
>> +ret = call_write_iter(file, &aio_cmd->iocb, &iter);
>> +else
>> +ret = call_read_iter(file, &aio_cmd->iocb, &iter);
>> +
>> +kfree(bvec);
>> +
>> +if (ret != -EIOCBQUEUED)
>> +cmd_rw_aio_complete(&aio_cmd->iocb, ret, 0);
>> +
>> +return 0;
>> +}
>> +
>> static int fd_do_rw(struct se_cmd *cmd, struct file *fd,
>> u32 block_size, struct scatterlist *sgl,
>> u32 sgl_nents, u32 data_length, int is_write)
>> @@ -527,7 +605,7 @@ fd_execute_unmap(struct se_cmd
[PATCH-next v2] scsi: libsas: dynamically allocate and free ata host
Now the ata host for libsas is embedded in domain_device, and the ->kref
member is not initialized. Afer we add ata transport class,
ata_host_get() will be called when adding transport ATA port and a
warning will be triggered as below:
refcount_t: increment on 0; use-after-free.
WARNING: CPU: 2 PID: 103 at lib/refcount.c:153 refcount_inc+0x40/0x48
..
Call trace:
refcount_inc+0x40/0x48
ata_host_get+0x10/0x18
ata_tport_add+0x40/0x120
ata_sas_tport_add+0xc/0x14
sas_ata_init+0x7c/0xc8
sas_discover_domain+0x380/0x53c
process_one_work+0x12c/0x288
worker_thread+0x58/0x3f0
kthread+0xfc/0x128
ret_from_fork+0x10/0x18
And also when removing transport ATA port ata_host_put() will be called
and another similar warning will be triggered. If the refcount decreased
to zero, the ata host will be freed. But this ata host is only part of
domain_device, it cannot be freed directly.
So we have to change this embedded static ata host to a dynamically
allocated ata host and initialize the ->kref member. To use
ata_host_get() and ata_host_put() in libsas, we need to move the
declaration of these functions to the public libata.h and export them.
v2: export ata_host_get() and ata_host_put().
Fixes: b6240a4df018 ("scsi: libsas: add transport class for ATA devices")
Signed-off-by: Jason Yan
CC: John Garry
---
drivers/ata/libata-core.c | 3 +++
drivers/ata/libata.h | 2 --
drivers/scsi/libsas/sas_ata.c | 40 +-
drivers/scsi/libsas/sas_discover.c | 2 ++
include/linux/libata.h | 2 ++
include/scsi/libsas.h | 2 +-
6 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 68596bd4cf06..cdb48dccfb45 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6424,6 +6424,7 @@ void ata_host_init(struct ata_host *host, struct device
*dev,
host->n_tags = ATA_MAX_QUEUE - 1;
host->dev = dev;
host->ops = ops;
+ kref_init(&host->kref);
}
void __ata_port_probe(struct ata_port *ap)
@@ -7391,3 +7392,5 @@ EXPORT_SYMBOL_GPL(ata_cable_80wire);
EXPORT_SYMBOL_GPL(ata_cable_unknown);
EXPORT_SYMBOL_GPL(ata_cable_ignore);
EXPORT_SYMBOL_GPL(ata_cable_sata);
+EXPORT_SYMBOL_GPL(ata_host_get);
+EXPORT_SYMBOL_GPL(ata_host_put);
\ No newline at end of file
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 9e21c49cf6be..f953cb4bb1ba 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -100,8 +100,6 @@ extern int ata_port_probe(struct ata_port *ap);
extern void __ata_port_probe(struct ata_port *ap);
extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
u8 page, void *buf, unsigned int sectors);
-extern void ata_host_get(struct ata_host *host);
-extern void ata_host_put(struct ata_host *host);
#define to_ata_port(d) container_of(d, struct ata_port, tdev)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index ff1d612f6fb9..41cdda7a926b 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -557,34 +557,46 @@ int sas_ata_init(struct domain_device *found_dev)
{
struct sas_ha_struct *ha = found_dev->port->ha;
struct Scsi_Host *shost = ha->core.shost;
+ struct ata_host *ata_host;
struct ata_port *ap;
int rc;
- ata_host_init(&found_dev->sata_dev.ata_host, ha->dev, &sas_sata_ops);
- ap = ata_sas_port_alloc(&found_dev->sata_dev.ata_host,
- &sata_port_info,
- shost);
+ ata_host = kzalloc(sizeof(*ata_host), GFP_KERNEL);
+ if (!ata_host) {
+ SAS_DPRINTK("ata host alloc failed.\n");
+ return -ENOMEM;
+ }
+
+ ata_host_init(ata_host, ha->dev, &sas_sata_ops);
+
+ ap = ata_sas_port_alloc(ata_host, &sata_port_info, shost);
if (!ap) {
SAS_DPRINTK("ata_sas_port_alloc failed.\n");
- return -ENODEV;
+ rc = -ENODEV;
+ goto free_host;
}
ap->private_data = found_dev;
ap->cbl = ATA_CBL_SATA;
ap->scsi_host = shost;
rc = ata_sas_port_init(ap);
- if (rc) {
- ata_sas_port_destroy(ap);
- return rc;
- }
- rc = ata_sas_tport_add(found_dev->sata_dev.ata_host.dev, ap);
- if (rc) {
- ata_sas_port_destroy(ap);
- return rc;
- }
+ if (rc)
+ goto destroy_port;
+
+ rc = ata_sas_tport_add(ata_host->dev, ap);
+ if (rc)
+ goto destroy_port;
+
+ found_dev->sata_dev.ata_host = ata_host;
found_dev->sata_dev.ap = ap;
return 0;
+
+destroy_port:
+ ata_sas_port_destroy(ap);
+free_host:
+ ata_host_put(ata_host);
+ return rc;
}
void sas_ata_task_abort(struct sas_task *task)
diff --git a/drivers/scsi
Re: [PATCH-next] scsi: libsas: dynamically allocate and free ata host
Hi, Thank you for the patch! Yet something to improve: [auto build test ERROR on mkp-scsi/for-next] [also build test ERROR on next-20180504] [cannot apply to v4.17-rc4] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/linux-kernel-owner-vger-kernel-org/scsi-libsas-dynamically-allocate-and-free-ata-host/20180507-105656 base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next config: x86_64-federa-25 (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): >> ERROR: "ata_host_put" [drivers/scsi/libsas/libsas.ko] undefined! --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
[PATCH 3/3] scsi: arcmsr: Update driver version to v1.40.00.06-20180504
>From Ching Huang Update driver version to v1.40.00.06-20180504 Signed-off-by: Ching Huang --- diff --git a/drivers/scsi/arcmsr/arcmsr.h b/drivers/scsi/arcmsr/arcmsr.h index 62eeef5..eb39623 100755 --- a/drivers/scsi/arcmsr/arcmsr.h +++ b/drivers/scsi/arcmsr/arcmsr.h @@ -49,7 +49,7 @@ struct device_attribute; #define ARCMSR_MAX_OUTSTANDING_CMD 1024 #define ARCMSR_DEFAULT_OUTSTANDING_CMD 128 #define ARCMSR_MIN_OUTSTANDING_CMD 32 -#define ARCMSR_DRIVER_VERSION "v1.40.00.05-20180309" +#define ARCMSR_DRIVER_VERSION "v1.40.00.06-20180504" #define ARCMSR_SCSI_INITIATOR_ID 255 #define ARCMSR_MAX_XFER_SECTORS512 #define ARCMSR_MAX_XFER_SECTORS_B 4096
