Enable lock context analysis in the SCSI core and UFS driver
Subsystems (6)
Bart Van Assche
reviewerv5 cover Enable lock context analysis in the SCSI core and UFS driver
Hi Martin,
This patch series enables lock context analysis for the SCSI core and the UFS driver. The advantages are as follows:
- The compiler (only Clang) verifies whether the lock and unlock calls match what has been declared via __must_hold(), __acquires() or __releases(). This is useful for catching locking bugs in error paths.
- Support for __guarded_by() is enabled. If a member variable is annotated with __guarded_by(lock), the compiler will issue a warning if that member variable is accessed without holding 'lock'.
Additionally, a patch is included that suppresses KCSAN complaints about SCSI host state changes.
More information about lock context analysis is available in the cover letter of [PATCH v5 00/36] Compiler-Based Context- and Locking-Analysis
(https://lore.kernel.org/lkml/[email protected]/).
Please consider this patch series for the next merge window.
Thanks,
Bart.
Changes compared to v4:
- Added Reviewed-by tags to the UFS patches.
- Made the title and the description of patch 3/6 more detailed.
Changes compared to v3:
- Left out all changes except those for the SCSI core and the UFS driver.
Changes compared to v2:
- Added patch "scsi: core: Pass the SCSI host pointer directly".
- Corrected several annotations that had missing or extra ampersands (&).
- Reduced the number of __assume_ctx_lock() statements.
Changes compared to v1:
- Split patch "Enable lock context analysis" into one patch per driver.
Bart Van Assche (6):
ufs: core: Set task state before io_schedule_timeout()
ufs: core: Enable lock context analysis
scsi: core: Pass the SCSI host pointer directly to scanning functions
scsi: core: Add lock context annotations
scsi: core: Protect host state changes with the host lock
scsi: core: Enable lock context analysis
drivers/scsi/Makefile | 22 ++++++++++ drivers/scsi/device_handler/Makefile | 3 ++ drivers/scsi/hosts.c | 18 ++++---- drivers/scsi/megaraid/megaraid_sas_base.c | 2 +- drivers/scsi/mpi3mr/mpi3mr_os.c | 2 +- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 2 +- drivers/scsi/qla4xxx/ql4_os.c | 6 +-- drivers/scsi/scsi_lib.c | 3 +- drivers/scsi/scsi_scan.c | 52 ++++++++++++++--------- drivers/scsi/scsi_sysfs.c | 7 +-- drivers/ufs/core/Makefile | 2 + drivers/ufs/core/ufs-debugfs.c | 8 +++- drivers/ufs/core/ufshcd.c | 15 +++++++ drivers/ufs/host/Makefile | 2 + include/scsi/scsi_host.h | 23 +++++++--- 15 files changed, 117 insertions(+), 50 deletions(-)
Bart Van Assche
reviewerPATCH v5 1/6 ufs: core: Set task state before io_schedule_timeout()
Set the task state to TASK_UNINTERRUPTIBLE before calling io_schedule_timeout() in ufshcd_wait_for_pending_cmds(). Without setting the task state, io_schedule_timeout() returns immediately because the task state remains TASK_RUNNING. This results in a busy loop that wastes CPU cycles.
--- drivers/ufs/core/ufshcd.c | 1 + 1 file changed, 1 insertion(+)
drivers/ufs/core/ufshcd.c +1 -0
@@ -1317,6 +1317,7 @@ static int ufshcd_wait_for_pending_cmds(struct ufs_hba *hba,break ; } +__set_current_state (TASK_UNINTERRUPTIBLE );io_schedule_timeout (msecs_to_jiffies (20 ));if (ktime_to_us (ktime_sub (ktime_get (), start)) > wait_timeout_us) {
Bart Van Assche
reviewerPATCH v5 2/6 ufs: core: Enable lock context analysis
In reply to Bart Van AsscheAnnotate functions that modify the state of a synchronization object. Remove the struct semaphore annotations because lock context annotations are not supported for semaphores.
--- drivers/ufs/core/Makefile | 2 ++ drivers/ufs/core/ufs-debugfs.c | 8 ++++++-- drivers/ufs/core/ufshcd.c | 14 ++++++++++++++ drivers/ufs/host/Makefile | 2 ++ 4 files changed, 24 insertions(+), 2 deletions(-)
drivers/ufs/core/Makefile +2 -0
@@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 +CONTEXT_ANALYSIS := y + obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o ufshcd-core-y += ufshcd.o ufs-sysfs.o ufs-mcq.o ufs-txeq.o ufshcd-core-$(CONFIG_RPMB) += ufs-rpmb.o
drivers/ufs/core/ufs-debugfs.c +6 -2
@@ -65,8 +65,10 @@ static int ee_usr_mask_get(void *data, u64 *val)return 0 ; } +token_context_lock (ufs_debugfs); +static int ufs_debugfs_get_user_access (struct ufs_hba *hba) -__acquires(&hba->host_sem) +__cond_acquires (0 , ufs_debugfs) {down (&hba->host_sem);if (!ufshcd_is_user_access_allowed (hba)) {
@@ -74,14 +76,16 @@ __acquires(&hba->host_sem)return -EBUSY ; }ufshcd_rpm_get_sync (hba); +__acquire (ufs_debugfs);return 0 ; }static void ufs_debugfs_put_user_access (struct ufs_hba *hba) -__releases(&hba->host_sem) +__releases (ufs_debugfs) {ufshcd_rpm_put_sync (hba);up (&hba->host_sem); +__release (ufs_debugfs); }static int ee_usr_mask_set (void *data,u64 val)
drivers/ufs/core/ufshcd.c +14 -0
@@ -1354,6 +1354,8 @@ static int ufshcd_wait_for_pending_cmds(struct ufs_hba *hba,* On failure, all acquired locks are released and the tagset is unquiesced. */ int ufshcd_pause_command_processing (struct ufs_hba *hba,u64 timeout_us) + __cond_acquires(0, &hba->host->scan_mutex) + __cond_acquires(0, &hba->clk_scaling_lock) {int ret =0 ;
@@ -1378,6 +1380,8 @@ int ufshcd_pause_command_processing(struct ufs_hba *hba, u64 timeout_us)* This function resumes command submissions. */ void ufshcd_resume_command_processing (struct ufs_hba *hba) + __releases(&hba->clk_scaling_lock) + __releases(&hba->host->scan_mutex) {up_write (&hba->clk_scaling_lock);blk_mq_unquiesce_tagset (&hba->host->tag_set);
@@ -1447,6 +1451,9 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, u32 target_gear, bool scale_up* Return: 0 upon success; -EBUSY upon timeout. */ static int ufshcd_clock_scaling_prepare (struct ufs_hba *hba,u64 timeout_us) + __cond_acquires(0, &hba->host->scan_mutex) + __cond_acquires(0, &hba->wb_mutex) + __cond_acquires(0, &hba->clk_scaling_lock) {int ret =0 ;/*
@@ -1476,6 +1483,9 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba, u64 timeout_us) }static void ufshcd_clock_scaling_unprepare (struct ufs_hba *hba,int err) + __releases(&hba->clk_scaling_lock) + __releases(&hba->wb_mutex) + __releases(&hba->host->scan_mutex) {up_write (&hba->clk_scaling_lock);mutex_unlock (&hba->wb_mutex);
@@ -3283,6 +3293,8 @@ ufshcd_dev_cmd_completion(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) }static void ufshcd_dev_man_lock (struct ufs_hba *hba) + __acquires(&hba->dev_cmd.lock) + __acquires_shared(&hba->clk_scaling_lock) {ufshcd_hold (hba);mutex_lock (&hba->dev_cmd.lock);
@@ -3290,6 +3302,8 @@ static void ufshcd_dev_man_lock(struct ufs_hba *hba) }static void ufshcd_dev_man_unlock (struct ufs_hba *hba) + __releases_shared(&hba->clk_scaling_lock) + __releases(&hba->dev_cmd.lock) {up_read (&hba->clk_scaling_lock);mutex_unlock (&hba->dev_cmd.lock);
drivers/ufs/host/Makefile +2 -0
@@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 +CONTEXT_ANALYSIS := y + obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-dwc.o tc-dwc-g210.o obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-dwc-g210.o obj-$(CONFIG_SCSI_UFS_CDNS_PLATFORM) += cdns-pltfrm.o
Bart Van Assche
reviewerPATCH v5 3/6 scsi: core: Pass the SCSI host pointer directly to scanning functions
In reply to Bart Van AsscheIn the functions scsi_probe_and_add_lun(), scsi_sequential_lun_scan(), scsi_report_lun_scan() and __scsi_scan_target() the SCSI host pointer is derived from the SCSI target pointer. Pass the SCSI host pointer directly.
This patch prepares for enabling lock context analysis. With this patch applied, lock context annotations can refer to the SCSI host pointer directly, e.g. __must_hold(&shost->scan_mutex). Without this patch, the following annotation would have to be used:
__must_hold(&dev_to_shost(starget->dev.parent)->scan_mutex) Additionally, in code that locks shost->scan_mutex, the following would have to be added to help the compiler understand that shost == dev_to_shost(starget->dev.parent): __assume_ctx_lock(&dev_to_shost(starget->dev.parent)->scan_mutex);
__assume_ctx_lock() statements should be avoided if there is a good alternative. Hence this patch. No functionality has been changed.
--- drivers/scsi/scsi_scan.c | 47 ++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 21 deletions(-)
drivers/scsi/scsi_scan.c +26 -21
@@ -1169,6 +1169,7 @@ static unsigned char *scsi_inq_str(unsigned char *buf, unsigned char *inq,/** * scsi_probe_and_add_lun - probe a LUN, if a LUN is found add it +* @shost: SCSI host pointer * @starget: pointer to target device structure * @lun: LUN of target device * @bflagsp: store bflags here if not NULL
@@ -1188,7 +1189,8 @@ static unsigned char *scsi_inq_str(unsigned char *buf, unsigned char *inq,* attached at the LUN * - SCSI_SCAN_LUN_PRESENT: a new scsi_device was allocated and initialized **/ -static int scsi_probe_and_add_lun(struct scsi_target *starget, +static int scsi_probe_and_add_lun(struct Scsi_Host *shost, + struct scsi_target *starget, u64 lun, blist_flags_t *bflagsp, struct scsi_device **sdevp, enum scsi_scan_mode rescan,
@@ -1198,7 +1200,6 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,unsigned char *result;blist_flags_t bflags;int res =SCSI_SCAN_NO_RESPONSE , result_len =256 ; -struct Scsi_Host *shost =dev_to_shost (starget->dev.parent);/* * The rescan flag is used as an optimization, the first scan of a
@@ -1334,6 +1335,7 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,/** * scsi_sequential_lun_scan - sequentially scan a SCSI target +* @shost: SCSI host pointer * @starget: pointer to target structure to scan * @bflags: black/white list flag for LUN 0 * @scsi_level: Which version of the standard does this device adhere to
@@ -1346,13 +1348,13 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,* * Modifies sdevscan->lun. **/ -static void scsi_sequential_lun_scan (struct scsi_target *starget, +static void scsi_sequential_lun_scan (struct Scsi_Host *shost, +struct scsi_target *starget,blist_flags_t bflags,int scsi_level,enum scsi_scan_mode rescan) {uint max_dev_lun;u64 sparse_lun, lun; -struct Scsi_Host *shost =dev_to_shost (starget->dev.parent);SCSI_LOG_SCAN_BUS (3 ,starget_printk (KERN_INFO , starget,"scsi scan: Sequential scan\n" ));
@@ -1412,14 +1414,15 @@ static void scsi_sequential_lun_scan(struct scsi_target *starget,* sparse_lun. */ for (lun =1 ; lun < max_dev_lun; ++lun) -if ((scsi_probe_and_add_lun (starget, lun,NULL ,NULL , rescan, -NULL ) !=SCSI_SCAN_LUN_PRESENT ) && +if (scsi_probe_and_add_lun (shost, starget, lun,NULL ,NULL , + rescan,NULL ) !=SCSI_SCAN_LUN_PRESENT && !sparse_lun)return ; }/** * scsi_report_lun_scan - Scan using SCSI REPORT LUN results +* @shost: SCSI host pointer * @starget: which target * @bflags: Zero or a mix of BLIST_NOLUN, BLIST_REPORTLUN2, or BLIST_NOREPORTLUN * @rescan: nonzero if we can skip code only needed on first scan
@@ -1438,8 +1441,9 @@ static void scsi_sequential_lun_scan(struct scsi_target *starget,* 0: scan completed (or no memory, so further scanning is futile) * 1: could not scan with REPORT LUN **/ -static int scsi_report_lun_scan (struct scsi_target *starget,blist_flags_t bflags, -enum scsi_scan_mode rescan) +static int scsi_report_lun_scan (struct Scsi_Host *shost, +struct scsi_target *starget,blist_flags_t bflags, +enum scsi_scan_mode rescan) {unsigned char scsi_cmd[MAX_COMMAND_SIZE ];unsigned int length;
@@ -1448,7 +1452,6 @@ static int scsi_report_lun_scan(struct scsi_target *starget, blist_flags_t bflagint result;struct scsi_lun *lunp, *lun_data;struct scsi_device *sdev; -struct Scsi_Host *shost =dev_to_shost (&starget->dev);struct scsi_failure failure_defs[] = { { .sense =UNIT_ATTENTION ,
@@ -1594,7 +1597,7 @@ static int scsi_report_lun_scan(struct scsi_target *starget, blist_flags_t bflag }else {int res; - res =scsi_probe_and_add_lun (starget, + res =scsi_probe_and_add_lun (shost, starget, lun,NULL ,NULL , rescan,NULL );if (res ==SCSI_SCAN_NO_RESPONSE ) {/*
@@ -1641,7 +1644,7 @@ struct scsi_device *__scsi_add_device(struct Scsi_Host *shost, uint channel,scsi_complete_async_scans ();if (scsi_host_scan_allowed (shost) &&scsi_autopm_get_host (shost) ==0 ) { -scsi_probe_and_add_lun (starget, lun,NULL , &sdev, +scsi_probe_and_add_lun (shost, starget, lun,NULL , &sdev,SCSI_SCAN_RESCAN , hostdata);scsi_autopm_put_host (shost); }
@@ -1763,10 +1766,10 @@ int scsi_rescan_device(struct scsi_device *sdev) }EXPORT_SYMBOL (scsi_rescan_device); -static void __scsi_scan_target (struct device *parent,unsigned int channel, -unsigned int id,u64 lun,enum scsi_scan_mode rescan) +static void __scsi_scan_target (struct Scsi_Host *shost,struct device *parent, +unsigned int channel,unsigned int id,u64 lun, +enum scsi_scan_mode rescan) { -struct Scsi_Host *shost =dev_to_shost (parent);blist_flags_t bflags =0 ;int res;struct scsi_target *starget;
@@ -1786,7 +1789,8 @@ static void __scsi_scan_target(struct device *parent, unsigned int channel,/* * Scan for a specific host/chan/id/lun. */ -scsi_probe_and_add_lun (starget, lun,NULL ,NULL , rescan,NULL ); +scsi_probe_and_add_lun (shost, starget, lun,NULL ,NULL , rescan, +NULL ); goto out_reap; }
@@ -1794,14 +1798,15 @@ static void __scsi_scan_target(struct device *parent, unsigned int channel,* Scan LUN 0, if there is some response, scan further. Ideally, we * would not configure LUN 0 until all LUNs are scanned. */ - res =scsi_probe_and_add_lun (starget,0 , &bflags,NULL , rescan,NULL ); + res =scsi_probe_and_add_lun (shost, starget,0 , &bflags,NULL , rescan, +NULL );if (res ==SCSI_SCAN_LUN_PRESENT || res ==SCSI_SCAN_TARGET_PRESENT ) { -if (scsi_report_lun_scan (starget, bflags, rescan) !=0 ) +if (scsi_report_lun_scan (shost, starget, bflags, rescan) !=0 )/* * The REPORT LUN did not scan the target, * do a sequential scan. */ -scsi_sequential_lun_scan (starget, bflags, +scsi_sequential_lun_scan (shost, starget, bflags, starget->scsi_level, rescan); }
@@ -1851,7 +1856,7 @@ void scsi_scan_target(struct device *parent, unsigned int channel,scsi_complete_async_scans ();if (scsi_host_scan_allowed (shost) &&scsi_autopm_get_host (shost) ==0 ) { -__scsi_scan_target (parent, channel, id, lun, rescan); +__scsi_scan_target (shost, parent, channel, id, lun, rescan);scsi_autopm_put_host (shost); }mutex_unlock (&shost->scan_mutex);
@@ -1882,11 +1887,11 @@ static void scsi_scan_channel(struct Scsi_Host *shost, unsigned int channel, order_id = shost->max_id - id -1 ; else order_id = id; -__scsi_scan_target (&shost->shost_gendev, channel, +__scsi_scan_target (shost, &shost->shost_gendev, channel, order_id, lun, rescan); }else -__scsi_scan_target (&shost->shost_gendev, channel, +__scsi_scan_target (shost, &shost->shost_gendev, channel, id, lun, rescan); }
Re: [PATCH v5 3/6] scsi: core: Pass the SCSI host pointer directly to scanning functions
Quoted 14 lines · Bart Van Assche
In the functions
scsi_probe_and_add_lun(),scsi_sequential_lun_scan(),scsi_report_lun_scan()and__scsi_scan_target()the SCSI host pointer is derived from the SCSI target pointer. Pass the SCSI host pointer directly.This patch prepares for enabling lock context analysis. With this patch applied, lock context annotations can refer to the SCSI host pointer directly, e.g.
__must_hold(&shost->scan_mutex). Without this patch, the following annotation would have to be used:__must_hold(&dev_to_shost(starget->dev.parent)->scan_mutex) Additionally, in code that locks shost->scan_mutex, the following would have to be added to help the compiler understand that shost == dev_to_shost(starget->dev.parent): __assume_ctx_lock(&dev_to_shost(starget->dev.parent)->scan_mutex);
Quoted 4 lines
__assume_ctx_lock()statements should be avoided if there is a good alternative. Hence this patch. No functionality has been changed.Signed-off-by Bart Van Assche<[email protected]>
Bart Van Assche
reviewerPATCH v5 4/6 scsi: core: Add lock context annotations
In reply to Bart Van AsscheDocument which functions expect that shost->scan_mutex is held.
--- drivers/scsi/scsi_scan.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/scsi/scsi_scan.c +5 -0
@@ -1195,6 +1195,7 @@ static int scsi_probe_and_add_lun(struct Scsi_Host *shost,struct scsi_device **sdevp, enum scsi_scan_mode rescan, void *hostdata) + __must_hold(&shost->scan_mutex) { struct scsi_device *sdev;unsigned char *result;
@@ -1352,6 +1353,7 @@ static void scsi_sequential_lun_scan(struct Scsi_Host *shost,struct scsi_target *starget, blist_flags_t bflags, int scsi_level, enum scsi_scan_mode rescan) + __must_hold(&shost->scan_mutex) { uint max_dev_lun;u64 sparse_lun, lun;
@@ -1444,6 +1446,7 @@ static void scsi_sequential_lun_scan(struct Scsi_Host *shost,static int scsi_report_lun_scan (struct Scsi_Host *shost,struct scsi_target *starget,blist_flags_t bflags,enum scsi_scan_mode rescan) + __must_hold(&shost->scan_mutex) {unsigned char scsi_cmd[MAX_COMMAND_SIZE ];unsigned int length;
@@ -1769,6 +1772,7 @@ EXPORT_SYMBOL(scsi_rescan_device);static void __scsi_scan_target (struct Scsi_Host *shost,struct device *parent,unsigned int channel,unsigned int id,u64 lun,enum scsi_scan_mode rescan) + __must_hold(&shost->scan_mutex) {blist_flags_t bflags =0 ;int res;
@@ -1866,6 +1870,7 @@ EXPORT_SYMBOL(scsi_scan_target);static void scsi_scan_channel (struct Scsi_Host *shost,unsigned int channel,unsigned int id,u64 lun,enum scsi_scan_mode rescan) + __must_hold(&shost->scan_mutex) {uint order_id;
[email protected]
AI reviewRe: [PATCH v5 4/6] scsi: core: Add lock context annotations
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Low] Incomplete implementation of lock context annotations.
Pre-existing issues:
- [High] Integer overflow in scsi_report_lun_scan leading to infinite loop or out-of-bounds read. --
--- Patch [4]: [PATCH v5 4/6] scsi: core: Add lock context annotations --- commit f676281064957e5d166348bcd6e0996c11afa242
Author: Bart Van Assche <[email protected]>
scsi: core: Add lock context annotations
This commit adds lock context annotations to document which functions expect shost->scan_mutex to be held.
Quoted 4 lines
drivers/scsi/scsi_scan.c +0
-0
[ ... ]
drivers/scsi/scsi_scan.c static void scsi_sequential_lun_scan(struct Scsi_Host *shost, · line 1450
Quoted 6 lines
drivers/scsi/scsi_scan.c +1
-0@@ -1444,6 +1446,7 @@ static void scsi_sequential_lun_scan(struct Scsi_Host *shost,static int scsi_report_lun_scan (struct Scsi_Host *shost,struct scsi_target *starget,blist_flags_t bflags,enum scsi_scan_mode rescan) + __must_hold(&shost->scan_mutex) {
[Severity: High]
This is a pre-existing issue, but while looking at scsi_report_lun_scan() there appears to be an integer overflow condition when handling untrusted device input.
If a malicious device returns 0xFFFFFFFF as the length in a REPORT LUNS response, the addition with sizeof(struct scsi_lun) can cause an overflow:
if (get_unaligned_be32(lun_data->scsi_lun) +
sizeof(struct scsi_lun) > length) {
length = get_unaligned_be32(lun_data->scsi_lun) +
sizeof(struct scsi_lun);
kfree(lun_data);
goto retry;
}On 64-bit platforms, size_t promotion causes the addition to evaluate to 0x100000007, which is greater than length. When this is assigned back to the 32-bit length variable, it truncates to 7. This causes the code to reallocate a 7-byte buffer and retry indefinitely, leading to a deadlock on scan_mutex.
On 32-bit platforms, the addition wraps to 7, skipping the reallocation entirely. The length variable stays 0xFFFFFFFF, which results in a massive num_luns count and eventually an out-of-bounds read past the buffer.
Could we add a bounds check to ensure the reported length doesn't overflow before we attempt to reallocate?
[ ... ]
[Severity: Low]
Are there other functions that should be included in this update? Looking through the SCSI core code, there are a few other static and internal helpers that rely on scan_mutex being held, such as scsi_sysfs_add_devices() in scsi_scan.c:
static void scsi_sysfs_add_devices(struct Scsi_Host *shost)
{
...
}and __scsi_remove_device() in scsi_sysfs.c:
void __scsi_remove_device(struct scsi_device *sdev)
{
...
}Would it be useful to add the __must_hold annotation to these functions as well so static analysis tools can catch missing locks across the subsystem?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Re: [PATCH v5 4/6] scsi: core: Add lock context annotations
In reply to Bart Van AsscheQuoted 8 lines · Bart Van Assche
Document which functions expect that
shost->scan_mutexis held. Signed-off-by:
Bart Van Assche<bvanassche@ acm. org> --- drivers/scsi/scsi_scan. c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/scsi/scsi_scan. c b/drivers/ scsi/scsi_scan. cDocument which functions expect that
shost->scan_mutexis held.Signed-off-by Bart Van Assche<[email protected]>
Bart Van Assche
reviewerPATCH v5 5/6 scsi: core: Protect host state changes with the host lock
In reply to Bart Van AsscheSome but not all SCSI host state changes are protected with the SCSI host lock. Annotate the SCSI host state with __guarded_by(host_lock), protect all SCSI host state changes with the SCSI host lock and use READ_ONCE() for all SCSI host state reads. This patch prevents that KCSAN complains about data races when accessing the SCSI host state.
--- drivers/scsi/hosts.c | 18 ++++++++++-------- drivers/scsi/megaraid/megaraid_sas_base.c | 2 +- drivers/scsi/mpi3mr/mpi3mr_os.c | 2 +- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 2 +- drivers/scsi/qla4xxx/ql4_os.c | 6 ++---- drivers/scsi/scsi_lib.c | 3 +-- drivers/scsi/scsi_sysfs.c | 7 ++++--- include/scsi/scsi_host.h | 23 ++++++++++++++++------- 8 files changed, 36 insertions(+), 27 deletions(-)
drivers/scsi/hosts.c +10 -8
@@ -74,7 +74,7 @@ static struct class shost_class = {**/ int scsi_host_set_state (struct Scsi_Host *shost,enum scsi_host_state state) { -enum scsi_host_state oldstate = shost->shost_state; +enum scsi_host_state oldstate =READ_ONCE (shost->shost_state);if (state == oldstate)return 0 ;
@@ -145,7 +145,7 @@ int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state) }break ; } - shost->shost_state = state; +WRITE_ONCE (shost->shost_state, state);return 0 ; illegal:
@@ -276,7 +276,8 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,if (error) goto out_disable_runtime_pm; -scsi_host_set_state (shost,SHOST_RUNNING ); + scoped_guard(spinlock_irq, shost->host_lock) +scsi_host_set_state (shost,SHOST_RUNNING );get_device (shost->shost_gendev.parent);device_enable_async_suspend (&shost->shost_dev);
@@ -350,6 +351,7 @@ EXPORT_SYMBOL(scsi_add_host_with_dma);static void scsi_host_dev_release (struct device *dev) {struct Scsi_Host *shost =dev_to_shost (dev); +enum scsi_host_state host_state =scsi_get_host_state (shost);struct device *parent = dev->parent;/* Wait for functions invoked through call_rcu(&scmd->rcu, ...) */
@@ -362,7 +364,7 @@ static void scsi_host_dev_release(struct device *dev)if (shost->work_q)destroy_workqueue (shost->work_q); -if (shost->shost_state ==SHOST_CREATED ) { +if (host_state ==SHOST_CREATED ) {/* * Free the shost_dev device name and remove the proc host dir * here if scsi_host_{alloc,put}() have been called but neither
@@ -378,7 +380,7 @@ static void scsi_host_dev_release(struct device *dev)ida_free (&host_index_ida, shost->host_no); -if (shost->shost_state !=SHOST_CREATED ) +if (host_state !=SHOST_CREATED )put_device (parent);kfree (shost); }
@@ -411,8 +413,8 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int privreturn NULL ; shost->host_lock = &shost->default_lock; -spin_lock_init (shost->host_lock); - shost->shost_state =SHOST_CREATED ; +scoped_guard (spinlock_init, shost->host_lock) + shost->shost_state = SHOST_CREATED;INIT_LIST_HEAD (&shost->__devices);INIT_LIST_HEAD (&shost->__targets);INIT_LIST_HEAD (&shost->eh_abort_list);
@@ -598,7 +600,7 @@ EXPORT_SYMBOL(scsi_host_lookup);**/ struct Scsi_Host *scsi_host_get (struct Scsi_Host *shost) { -if ((shost->shost_state ==SHOST_DEL ) || +if (scsi_get_host_state (shost) ==SHOST_DEL || !get_device (&shost->shost_gendev))return NULL ;return shost;
drivers/scsi/megaraid/megaraid_sas_base.c +1 -1
@@ -3072,7 +3072,7 @@ static int megasas_reset_bus_host(struct scsi_cmnd *scmd)scmd_printk (KERN_INFO , scmd,"SCSI host state: %d SCSI host busy: %d FW outstanding: %d\n" , - scmd->device->host->shost_state, +scsi_get_host_state (scmd->device->host),scsi_host_busy (scmd->device->host),atomic_read (&instance->fw_outstanding));/*
drivers/scsi/mpi3mr/mpi3mr_os.c +1 -1
@@ -5172,7 +5172,7 @@ static enum scsi_qc_status mpi3mr_qcmd(struct Scsi_Host *shost,/* Avoid error handling escalation when device is removed or blocked */ -if (scmd->device->host->shost_state ==SHOST_RECOVERY && +if (scsi_get_host_state (scmd->device->host) ==SHOST_RECOVERY && scmd->cmnd[0 ] ==TEST_UNIT_READY && (stgt_priv_data->dev_removed || (dev_handle ==MPI3MR_INVALID_DEV_HANDLE ))) {scsi_build_sense (scmd,0 ,UNIT_ATTENTION ,0x29 ,0x07 );
drivers/scsi/mpt3sas/mpt3sas_scsih.c +1 -1
@@ -5472,7 +5472,7 @@ static enum scsi_qc_status scsih_qcmd(struct Scsi_Host *shost,* Avoid error handling escallation when device is disconnected */ if (handle ==MPT3SAS_INVALID_DEVICE_HANDLE || sas_device_priv_data->block) { -if (scmd->device->host->shost_state ==SHOST_RECOVERY && +if (scsi_get_host_state (scmd->device->host) ==SHOST_RECOVERY && scmd->cmnd[0 ] ==TEST_UNIT_READY ) {scsi_build_sense (scmd,0 ,UNIT_ATTENTION ,0x29 ,0x07 );scsi_done (scmd);
drivers/scsi/qla4xxx/ql4_os.c +2 -4
@@ -9411,11 +9411,9 @@ static int qla4xxx_eh_target_reset(struct scsi_cmnd *cmd)* This routine finds that if reset host is called in EH * scenario or from some application like sg_reset **/ -static int qla4xxx_is_eh_active (struct Scsi_Host *shost) +static bool qla4xxx_is_eh_active (struct Scsi_Host *shost) { -if (shost->shost_state ==SHOST_RECOVERY ) -return 1 ; -return 0 ; +return scsi_get_host_state (shost) ==SHOST_RECOVERY ; }/**
drivers/scsi/scsi_lib.c +1 -2
@@ -1661,10 +1661,9 @@ static enum scsi_qc_status scsi_dispatch_cmd(struct scsi_cmnd *cmd) goto done; } -if (unlikely (host->shost_state ==SHOST_DEL )) { +if (unlikely (scsi_get_host_state (host) ==SHOST_DEL )) { cmd->result = (DID_NO_CONNECT <<16 ); goto done; - }trace_scsi_dispatch_cmd_start (cmd);
drivers/scsi/scsi_sysfs.c +4 -3
@@ -214,8 +214,9 @@ store_shost_state(struct device *dev, struct device_attribute *attr,if (!state)return -EINVAL ; -if (scsi_host_set_state (shost, state)) -return -EINVAL ; + scoped_guard(spinlock_irq, shost->host_lock) +if (scsi_host_set_state (shost, state)) +return -EINVAL ;return count; }
@@ -223,7 +224,7 @@ static ssize_t show_shost_state(struct device *dev, struct device_attribute *attr, char *buf) { struct Scsi_Host *shost = class_to_shost(dev); -const char *name =scsi_host_state_name (shost->shost_state); +const char *name =scsi_host_state_name (scsi_get_host_state (shost));if (!name)return -EINVAL ;
include/scsi/scsi_host.h +16 -7
@@ -727,7 +727,7 @@ struct Scsi_Host {unsigned int irq; -enum scsi_host_state shost_state; + enum scsi_host_state shost_state __guarded_by(host_lock);/* ldm bits */ struct device shost_gendev, shost_dev;
@@ -785,11 +785,18 @@ static inline struct Scsi_Host *dev_to_shost(struct device *dev) return container_of(dev, struct Scsi_Host, shost_gendev); } +static inline enum scsi_host_state scsi_get_host_state (struct Scsi_Host *shost) +{ +return context_unsafe (READ_ONCE (shost->shost_state)); +} +static inline int scsi_host_in_recovery (struct Scsi_Host *shost) { -return shost->shost_state ==SHOST_RECOVERY || - shost->shost_state ==SHOST_CANCEL_RECOVERY || - shost->shost_state ==SHOST_DEL_RECOVERY || +enum scsi_host_state state =scsi_get_host_state (shost); + +return state ==SHOST_RECOVERY || + state ==SHOST_CANCEL_RECOVERY || + state ==SHOST_DEL_RECOVERY || shost->tmf_in_progress; }
@@ -835,8 +842,9 @@ static inline struct device *scsi_get_device(struct Scsi_Host *shost)**/ static inline int scsi_host_scan_allowed (struct Scsi_Host *shost) { -return shost->shost_state ==SHOST_RUNNING || - shost->shost_state ==SHOST_RECOVERY ; +enum scsi_host_state state =scsi_get_host_state (shost); + +return state ==SHOST_RUNNING || state ==SHOST_RECOVERY ; }extern void scsi_unblock_requests (struct Scsi_Host *);
@@ -940,6 +948,7 @@ static inline unsigned char scsi_host_get_guard(struct Scsi_Host *shost)return shost->prot_guard_type; } -extern int scsi_host_set_state (struct Scsi_Host *,enum scsi_host_state ); +int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state) + __must_hold(shost->host_lock);#endif /* _SCSI_SCSI_HOST_H */
Re: [PATCH v5 5/6] scsi: core: Protect host state changes with the host lock
Quoted 43 lines · Bart Van Assche
--- drivers/scsi/hosts.c | 18 ++++++++++-------- drivers/scsi/megaraid/megaraid_sas_base.c | 2 +- drivers/scsi/mpi3mr/mpi3mr_os.c | 2 +- drivers/scsi/mpt3sas/mpt3sas_scsih.c | 2 +- drivers/scsi/qla4xxx/ql4_os.c | 6 ++---- drivers/scsi/scsi_lib.c | 3 +-- drivers/scsi/scsi_sysfs.c | 7 ++++--- include/scsi/scsi_host.h | 23 ++++++++++++++++------- 8 files changed, 36 insertions(+), 27 deletions(-)drivers/scsi/hosts.c +4
-3@@ -74,7 +74,7 @@ static struct class shost_class = {**/ int scsi_host_set_state (struct Scsi_Host *shost,enum scsi_host_state state) { -enum scsi_host_state oldstate = shost->shost_state; +enum scsi_host_state oldstate =READ_ONCE (shost->shost_state); > if (state == oldstate)return 0 ;@@ -145,7 +145,7 @@ int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state) }break ; } - shost->shost_state = state; +WRITE_ONCE (shost->shost_state, state);return 0 ; illegal:@@ -276,7 +276,8 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,if (error) goto out_disable_runtime_pm; -scsi_host_set_state (shost,SHOST_RUNNING ); + scoped_guard(spinlock_irq, shost->host_lock) +scsi_host_set_state (shost,SHOST_RUNNING );get_device (shost->shost_gendev.parent);device_enable_async_suspend (&shost->shost_dev);
drivers/scsi/hosts.c EXPORT_SYMBOL(scsi_add_host_with_dma); · line 354
Quoted 4 lines
drivers/scsi/hosts.c +1
-0@@ -350,6 +351,7 @@ EXPORT_SYMBOL(scsi_add_host_with_dma);static void scsi_host_dev_release (struct device *dev) {struct Scsi_Host *shost =dev_to_shost (dev); +enum scsi_host_state host_state =scsi_get_host_state (shost);
nit: it would be nice to use consistent variable names throughout the code, either state or host_state , if possible.
Quoted 153 lines
drivers/scsi/hosts.c +5
-5struct device *parent = dev->parent;/* Wait for functions invoked through call_rcu(&scmd->rcu, ...) */ @@ -362,7 +364,7 @@ static void scsi_host_dev_release(struct device *dev)if (shost->work_q)destroy_workqueue (shost->work_q); -if (shost->shost_state ==SHOST_CREATED ) { +if (host_state ==SHOST_CREATED ) {/* * Free the shost_dev device name and remove the proc host dir * here if scsi_host_{alloc,put}() have been called but neither @@ -378,7 +380,7 @@ static void scsi_host_dev_release(struct device *dev)ida_free (&host_index_ida, shost->host_no); -if (shost->shost_state !=SHOST_CREATED ) +if (host_state !=SHOST_CREATED )put_device (parent);kfree (shost); }@@ -411,8 +413,8 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int privreturn NULL ; shost->host_lock = &shost->default_lock; -spin_lock_init (shost->host_lock); - shost->shost_state =SHOST_CREATED ; +scoped_guard (spinlock_init, shost->host_lock) + shost->shost_state = SHOST_CREATED;INIT_LIST_HEAD (&shost->__devices);INIT_LIST_HEAD (&shost->__targets);INIT_LIST_HEAD (&shost->eh_abort_list);@@ -598,7 +600,7 @@ EXPORT_SYMBOL(scsi_host_lookup);**/ struct Scsi_Host *scsi_host_get (struct Scsi_Host *shost) { -if ((shost->shost_state ==SHOST_DEL ) || +if (scsi_get_host_state (shost) ==SHOST_DEL || !get_device (&shost->shost_gendev))return NULL ;return shost;drivers/scsi/megaraid/megaraid_sas_base.c +1
-1@@ -3072,7 +3072,7 @@ static int megasas_reset_bus_host(struct scsi_cmnd *scmd)scmd_printk (KERN_INFO , scmd,"SCSI host state: %d SCSI host busy: %d FW outstanding: %d\n" , - scmd->device->host->shost_state, +scsi_get_host_state (scmd->device->host),scsi_host_busy (scmd->device->host),atomic_read (&instance->fw_outstanding));/* drivers/scsi/mpi3mr/mpi3mr_os.c +1
-1@@ -5172,7 +5172,7 @@ static enum scsi_qc_status mpi3mr_qcmd(struct Scsi_Host *shost,/* Avoid error handling escalation when device is removed or blocked */ -if (scmd->device->host->shost_state ==SHOST_RECOVERY && +if (scsi_get_host_state (scmd->device->host) ==SHOST_RECOVERY && scmd->cmnd[0 ] ==TEST_UNIT_READY && (stgt_priv_data->dev_removed || (dev_handle ==MPI3MR_INVALID_DEV_HANDLE ))) {scsi_build_sense (scmd,0 ,UNIT_ATTENTION ,0x29 ,0x07 );drivers/scsi/mpt3sas/mpt3sas_scsih.c +1
-1@@ -5472,7 +5472,7 @@ static enum scsi_qc_status scsih_qcmd(struct Scsi_Host *shost,* Avoid error handling escallation when device is disconnected */ if (handle ==MPT3SAS_INVALID_DEVICE_HANDLE || sas_device_priv_data->block) { -if (scmd->device->host->shost_state ==SHOST_RECOVERY && +if (scsi_get_host_state (scmd->device->host) ==SHOST_RECOVERY && scmd->cmnd[0 ] ==TEST_UNIT_READY ) {scsi_build_sense (scmd,0 ,UNIT_ATTENTION ,0x29 ,0x07 );scsi_done (scmd);drivers/scsi/qla4xxx/ql4_os.c +2
-4@@ -9411,11 +9411,9 @@ static int qla4xxx_eh_target_reset(struct scsi_cmnd *cmd)* This routine finds that if reset host is called in EH * scenario or from some application like sg_reset **/ -static int qla4xxx_is_eh_active (struct Scsi_Host *shost) +static bool qla4xxx_is_eh_active (struct Scsi_Host *shost) { -if (shost->shost_state ==SHOST_RECOVERY ) -return 1 ; -return 0 ; +return scsi_get_host_state (shost) ==SHOST_RECOVERY ; }/** drivers/scsi/scsi_lib.c +1
-2@@ -1661,10 +1661,9 @@ static enum scsi_qc_status scsi_dispatch_cmd(struct scsi_cmnd *cmd) goto done; } -if (unlikely (host->shost_state ==SHOST_DEL )) { +if (unlikely (scsi_get_host_state (host) ==SHOST_DEL )) { cmd->result = (DID_NO_CONNECT <<16 ); goto done; - }trace_scsi_dispatch_cmd_start (cmd);drivers/scsi/scsi_sysfs.c +4
-3@@ -214,8 +214,9 @@ store_shost_state(struct device *dev, struct device_attribute *attr,if (!state)return -EINVAL ; -if (scsi_host_set_state (shost, state)) -return -EINVAL ; + scoped_guard(spinlock_irq, shost->host_lock) +if (scsi_host_set_state (shost, state)) +return -EINVAL ;return count; }@@ -223,7 +224,7 @@ static ssize_t show_shost_state(struct device *dev, struct device_attribute *attr, char *buf) { struct Scsi_Host *shost = class_to_shost(dev); -const char *name =scsi_host_state_name (shost->shost_state); +const char *name =scsi_host_state_name (scsi_get_host_state (shost));if (!name)return -EINVAL ;include/scsi/scsi_host.h +1
-1@@ -727,7 +727,7 @@ struct Scsi_Host {unsigned int irq; -enum scsi_host_state shost_state; + enum scsi_host_state shost_state __guarded_by(host_lock);/* ldm bits */ struct device shost_gendev, shost_dev;
include/scsi/scsi_host.h static inline struct Scsi_Host *dev_to_shost(struct device *dev) · line 790
Quoted 6 lines
include/scsi/scsi_host.h +3
-0@@ -785,11 +785,18 @@ static inline struct Scsi_Host *dev_to_shost(struct device *dev) return container_of(dev, struct Scsi_Host, shost_gendev); } +static inline enum scsi_host_state scsi_get_host_state (struct Scsi_Host *shost) +{ +return context_unsafe (READ_ONCE (shost->shost_state));
I am wondering if it may be better to protect reading this with the spinlock as well. We could lose the READ_ONCE and WRITE_ONCE. And we would be more symmetrical with the set function.
I really don't feel strongly about this, though.
Quoted 28 lines
include/scsi/scsi_host.h +10
-5+} +static inline int scsi_host_in_recovery (struct Scsi_Host *shost) { -return shost->shost_state ==SHOST_RECOVERY || - shost->shost_state ==SHOST_CANCEL_RECOVERY || - shost->shost_state ==SHOST_DEL_RECOVERY || +enum scsi_host_state state =scsi_get_host_state (shost); + +return state ==SHOST_RECOVERY || + state ==SHOST_CANCEL_RECOVERY || + state ==SHOST_DEL_RECOVERY || shost->tmf_in_progress; }@@ -835,8 +842,9 @@ static inline struct device *scsi_get_device(struct Scsi_Host *shost)**/ static inline int scsi_host_scan_allowed (struct Scsi_Host *shost) { -return shost->shost_state ==SHOST_RUNNING || - shost->shost_state ==SHOST_RECOVERY ; +enum scsi_host_state state =scsi_get_host_state (shost); + +return state ==SHOST_RUNNING || state ==SHOST_RECOVERY ; }extern void scsi_unblock_requests (struct Scsi_Host *);
include/scsi/scsi_host.h static inline unsigned char scsi_host_get_guard(struct Scsi_Host *shost) · line 952
Quoted 6 lines
include/scsi/scsi_host.h +2
-1@@ -940,6 +948,7 @@ static inline unsigned char scsi_host_get_guard(struct Scsi_Host *shost)return shost->prot_guard_type; } -extern int scsi_host_set_state (struct Scsi_Host *,enum scsi_host_state ); +int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state) + __must_hold(shost->host_lock);
How come we have this in the prototype and not the actual function itself?
Quoted 2 lines
include/scsi/scsi_host.h +0
-0#endif /* _SCSI_SCSI_HOST_H */
Bart Van Assche
reviewerRe: [PATCH v5 5/6] scsi: core: Protect host state changes with the host lock
Quoted 12 lines · John Garry
include/scsi/scsi_host.h static inline struct Scsi_Host · line 791
Quoted 8 lines · Bart Van Assche
include/scsi/scsi_host.h +3
-0@@ -785,11 +785,18 @@ static inline struct Scsi_Host *dev_to_shost(struct device *dev) return container_of(dev, struct Scsi_Host, shost_gendev); } +static inline enum scsi_host_state scsi_get_host_state (struct Scsi_Host *shost) +{ + return context_unsafe(READ_ONCE(shost->shost_state));I am wondering if it may be better to protect reading this with the spinlock as well. We could lose the
READ_ONCEandWRITE_ONCE. And we would be more symmetrical with the set function.
Quoted 1 line
I really don't feel strongly about this, though.
I slightly prefer READ_ONCE() because READ_ONCE() makes it clear that a race condition is triggered. Protecting the host state read with a spinlock is misleading in my opinion because it suppresses data race reports while the host state can change as soon as the spinlock has been unlocked.
Quoted 5 lines
Review static inline struct Scsi_Host · line 794
Quoted 3 lines
(quoted diff) +2
-0+int scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state) + __must_hold(shost->host_lock);How come we have this in the prototype and not the actual function itself?
Adding __must_hold() to the function declaration is sufficient. Adding__must_hold() to the function definition is optional if it has already been added to the function definition.
Thanks,
Bart.
Re: [PATCH v5 5/6] scsi: core: Protect host state changes with the host lock
Quoted 9 lines · Bart Van Assche
Quoted 5 lines
Quoted 3 lines
+int
scsi_host_set_state(struct Scsi_Host *shost, enum scsi_host_state state)
+__must_hold(shost->host_lock);How come we have this in the prototype and not the actual function itself?
Adding
__must_hold()to the function declaration is sufficient. Adding__must_hold()to the function definition is optional if it has already been added to the function definition.
Understood, but I would have thought that adding it in the actual function would be nice as when developers analyze the code, they look at the function and not the prototype (and it is nice to know the locking state).
Thanks,
John
Bart Van Assche
reviewerRe: [PATCH v5 5/6] scsi: core: Protect host state changes with the host lock
In reply to John GarryQuoted 9 lines · John Garry
Quoted 4 lines · Bart Van Assche
+static inline enum
scsi_host_statescsi_get_host_state(struct Scsi_Host *shost)
+{
+ return context_unsafe(READ_ONCE(shost->shost_state));I am wondering if it may be better to protect reading this with the spinlock as well. We could lose the
READ_ONCEandWRITE_ONCE. And we would be more symmetrical with the set function.I really don't feel strongly about this, though.
I prefer lockless access from scsi_get_host_state() because this function is called from the hot path. Using locking inside this function would cause lock contention on the host lock in the hot path.
Thanks,
Bart.
Bart Van Assche
reviewerPATCH v5 6/6 scsi: core: Enable lock context analysis
In reply to Bart Van AsscheEnable lock context analysis for those SCSI core files that build without triggering any compiler warnings.
--- drivers/scsi/Makefile | 22 ++++++++++++++++++++++ drivers/scsi/device_handler/Makefile | 3 +++ 2 files changed, 25 insertions(+)
drivers/scsi/Makefile +22 -0
@@ -14,6 +14,28 @@ # satisfy certain initialization assumptions in the SCSI layer. # *!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*! +CONTEXT_ANALYSIS_constants.o := y +CONTEXT_ANALYSIS_scsi.o := y +CONTEXT_ANALYSIS_scsi_common.o := y +CONTEXT_ANALYSIS_scsi_devinfo.o := y +CONTEXT_ANALYSIS_scsi_dh.o := y +CONTEXT_ANALYSIS_scsi_error.o := y +CONTEXT_ANALYSIS_scsi_ioctl.o := y +CONTEXT_ANALYSIS_scsi_lib.o := y +CONTEXT_ANALYSIS_scsi_lib_dma.o := y +CONTEXT_ANALYSIS_scsi_logging.o := y +CONTEXT_ANALYSIS_scsi_netlink.o := y +CONTEXT_ANALYSIS_scsi_pm.o := y +CONTEXT_ANALYSIS_scsi_proc.o := y +CONTEXT_ANALYSIS_scsi_scan.o := y +CONTEXT_ANALYSIS_scsi_sysfs.o := y +CONTEXT_ANALYSIS_scsi_trace.o := y +CONTEXT_ANALYSIS_scsicam.o := y +CONTEXT_ANALYSIS_sd.o := y +CONTEXT_ANALYSIS_sd_dif.o := y +CONTEXT_ANALYSIS_sd_zbc.o := y +CONTEXT_ANALYSIS_sr.o := y +CONTEXT_ANALYSIS_sr_ioctl.o := y CFLAGS_aha152x.o = -DAHA152X_STAT -DAUTOCONF
drivers/scsi/device_handler/Makefile +3 -0
@@ -2,6 +2,9 @@ # # SCSI Device Handler # + +CONTEXT_ANALYSIS := y + obj-$(CONFIG_SCSI_DH_RDAC) += scsi_dh_rdac.o obj-$(CONFIG_SCSI_DH_HP_SW) += scsi_dh_hp_sw.o obj-$(CONFIG_SCSI_DH_EMC) += scsi_dh_emc.o
Re: [PATCH v5 6/6] scsi: core: Enable lock context analysis
Quoted 2 lines · Bart Van Assche
Enable lock context analysis for those SCSI core files that build without triggering any compiler warnings.
Do you mean context analysis compiler warnings? If so, are there files which do trigger context analysis compiler warnings?
Quoted 37 lines
Signed-off-by Bart Van Assche<[email protected]>--- drivers/scsi/Makefile | 22 ++++++++++++++++++++++ drivers/scsi/device_handler/Makefile | 3 +++ 2 files changed, 25 insertions(+)drivers/scsi/Makefile +22
-0@@ -14,6 +14,28 @@ # satisfy certain initialization assumptions in the SCSI layer. # *!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*!*! +CONTEXT_ANALYSIS_constants.o := y +CONTEXT_ANALYSIS_scsi.o := y +CONTEXT_ANALYSIS_scsi_common.o := y +CONTEXT_ANALYSIS_scsi_devinfo.o := y +CONTEXT_ANALYSIS_scsi_dh.o := y +CONTEXT_ANALYSIS_scsi_error.o := y +CONTEXT_ANALYSIS_scsi_ioctl.o := y +CONTEXT_ANALYSIS_scsi_lib.o := y +CONTEXT_ANALYSIS_scsi_lib_dma.o := y +CONTEXT_ANALYSIS_scsi_logging.o := y +CONTEXT_ANALYSIS_scsi_netlink.o := y +CONTEXT_ANALYSIS_scsi_pm.o := y +CONTEXT_ANALYSIS_scsi_proc.o := y +CONTEXT_ANALYSIS_scsi_scan.o := y +CONTEXT_ANALYSIS_scsi_sysfs.o := y +CONTEXT_ANALYSIS_scsi_trace.o := y +CONTEXT_ANALYSIS_scsicam.o := y +CONTEXT_ANALYSIS_sd.o := y +CONTEXT_ANALYSIS_sd_dif.o := y +CONTEXT_ANALYSIS_sd_zbc.o := y +CONTEXT_ANALYSIS_sr.o := y +CONTEXT_ANALYSIS_sr_ioctl.o := y
Bart Van Assche
reviewerRe: [PATCH v5 6/6] scsi: core: Enable lock context analysis
Quoted 5 lines · John Garry
Quoted 2 lines · Bart Van Assche
Enable lock context analysis for those SCSI core files that build without triggering any compiler warnings.
Do you mean context analysis compiler warnings?
Yes, that's what I meant.
Quoted 2 lines
If so, are there files which do trigger context analysis compiler warnings?
Yes, multiple. All code changes and lock context annotations added by this patch series are necessary to make all SCSI drivers build without triggering lock context analysis compiler warnings:
"[PATCH v3 00/57] Enable lock context analysis for the SCSI subsystem"
(https://lore.kernel.org/linux-scsi/[email protected]/)
Thanks,
Bart.
Quoted 11 lines · Bart Van Assche
i Martin,
This patch series enables lock context analysis for the SCSI core and the UFS driver. The advantages are as follows:
- The compiler (only Clang) verifies whether the lock and unlock calls match what has been declared via__must_hold(),__acquires()or__releases(). This is useful for catching locking bugs in error paths.
- Support for__guarded_by()is enabled. If a member variable is annotated with__guarded_by(lock), the compiler will issue a warning if that member variable is accessed without holding 'lock'.
Quoted 2 lines
Additionally, a patch is included that suppresses KCSAN complaints about SCSI host state changes.
Hi Bart,
In future would we enable context analysis for other members of Scsi_Host or other core structures which are protected by shost->host_lock, like __scsi_find_target() which scans &shost->targets and requires that lock to be held? Or __scsi_device_lookup()?
I appreciate that this is just the first series for SCSI, but I am just curious on any plans.
Thanks,
John
Quoted 7 lines
More information about lock context analysis is available in the cover letter of
[PATCH v5 00/36] Compiler-Based Context- and Locking-Analysis
(https://urldefense.com/v3/__https://lore.kernel.org/ lkml/[email protected]/__;!!ACWV5N9M2RV99hQ! J6vIIEsGYaCUO5o4dBP5HeRy8kITKmj51IVOoZN-7h8k2FZDnUuaJGMWAt3E5BL5Xf4r7iNMlthMNat6EJGVFg$<https://urldefense.com/v3/__https://lore.kernel.org/ lkml/[email protected]/__;!!ACWV5N9M2RV99hQ! J6vIIEsGYaCUO5o4dBP5HeRy8kITKmj51IVOoZN-7h8k2FZDnUuaJGMWAt3E5BL5Xf4r7iNMlthMNat6EJGVFg$>).Please consider this patch series for the next merge window.