Re: [PATCH 0/3] hisi_sas: some CQ processing fixes
On 2017/1/3 20:24, John Garry wrote: This patchset fixes some issues related to servicing of the completion queue interrupt. The major fix is that sensitive hisi_hba structures need to be locked when free'ing a slot. Another modification is that the v2 hw completion queue irq is now serviced with a tasklet, as too much work was being done in the ISR. Tested on v2 based sas hardware and the crashes are gone, Tested-by: Hanjun Guo Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] target/user: Fix use-after-free cmd->se_cmd if the cmd is expired
On 01/03/2017 02:46 AM, lixi...@cmss.chinamobile.com wrote: > From: Xiubo Li > > This is another use-after-free bug, the crash Call Trace is like: > [ 368.909498] RIP: 0010:[] [] > memcpy+0x16/0x110 > .. > [ 368.909547] Call Trace: > [ 368.909550] [] ?gather_data_area+0x109/0x180 > [ 368.909552] [] tcmu_handle_completions+0x2ff/0x450 > [ 368.909554] [] tcmu_irqcontrol+0x15/0x20 > [ 368.909555] [] uio_write+0x7b/0xc0 > [ 368.909558] [] vfs_write+0xbd/0x1e0 > [ 368.909559] [] SyS_write+0x7f/0xe0 > [ 368.909562] [] system_call_fastpath+0x16/0x1b > > Don't free se_cmd of the expired cmds in tcmu_check_expired_cmd(), > it will be dereferenced by tcmu_handle_completions()---> > tcmu_handle_completion(), after userspace ever resumes processing. > > It will be freed by tcmu_handle_completion() if userspace ever recovers, > or tcmu_free_device if not. > > Cc: sta...@vger.kernel.org > Signed-off-by: Xiubo Li > Signed-off-by: Jianfei Hu > --- > drivers/target/target_core_user.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/target/target_core_user.c > b/drivers/target/target_core_user.c > index 2e33100..6396581 100644 > --- a/drivers/target/target_core_user.c > +++ b/drivers/target/target_core_user.c > @@ -684,7 +684,6 @@ static int tcmu_check_expired_cmd(int id, void *p, void > *data) > > set_bit(TCMU_CMD_BIT_EXPIRED, &cmd->flags); > target_complete_cmd(cmd->se_cmd, SAM_STAT_CHECK_CONDITION); > - cmd->se_cmd = NULL; > How did tcmu_handle_completion get to a point it was accessing the se_cmd if the TCMU_CMD_BIT_EXPIRED bit was set? Were memory accesses out of order? CPU1 set the TCMU_CMD_BIT_EXPIRED bit then cleared cmd->se_cmd, but CPU2 copied cmd->se_cmd to se_cmd and saw it was NULL but did not yet see the TCMU_CMD_BIT_EXPIRED bit set? It looks like, if you do the above patch, the above function will call target_complete_cmd and tcmu_handle_completion will call it again, so we will have a double free issue. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] scsi: hisi_sas: lock sensitive region in hisi_sas_slot_abort()
On 2017年01月03日 20:24, John Garry wrote: When we call hisi_sas_slot_task_free() we should grab the hisi_hba.lock, as hisi_sas_slot_task_free() accesses common hisi_hba elements. Function hisi_sas_slot_abort() is missing this, so add it. Signed-off-by: John Garry Reviewed-by: Zhangfei Gao -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] scsi: hisi_sas: lock sensitive regions when servicing CQ interrupt
On 2017年01月03日 20:24, John Garry wrote: There is a bug in the current driver in that certain hisi_hba and port structure elements which we access when servicing the CQ interrupt do not use thread-safe accesses; these include hisi_sas_port linked-list of active slots (hisi_sas_port.entry), bitmap of currently allocated IPTT (in hisi_hba.slot_index_tags), and completion queue read pointer. As a solution, lock these elements with the hisi_hba.lock. Signed-off-by: John Garry Reviewed-by: Xiang Chen Reviewed-by: Zhangfei Gao -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] scsi: hisi_sas: service v2 hw CQ ISR with tasklet
On 2017年01月03日 20:24, John Garry wrote: Currently the all the slot processing for the completion queue is done in ISR context. It is judged that the slot processing can take a long time, especially when a SATA NCQ completes (upto 32 slots). So, as a solution, defer the bulk of the ISR processing to tasklet context. Each CQ will have its down tasklet. Signed-off-by: John Garry Reviewed-by: Xiang Chen Reviewed-by: Zhangfei Gao -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: ufs-qcom: Fix module autoload
On 2017-01-02 06:04, Javier Martinez Canillas wrote:
If the driver is built as a module, autoload won't work because the
module
alias information is not filled. So user-space can't match the
registered
device with the corresponding module.
Export the module alias information using the MODULE_DEVICE_TABLE()
macro.
Before this patch:
$ modinfo drivers/scsi/ufs/ufs-qcom.ko | grep alias
$
After this patch:
$ modinfo drivers/scsi/ufs/ufs-qcom.ko | grep alias
alias: of:N*T*Cqcom,ufshcC*
alias: of:N*T*Cqcom,ufshc
Signed-off-by: Javier Martinez Canillas
---
drivers/scsi/ufs/ufs-qcom.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index abe617372661..5ff8a6bf6fd3 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -1692,6 +1692,7 @@ static const struct of_device_id
ufs_qcom_of_match[] = {
{ .compatible = "qcom,ufshc"},
{},
};
+MODULE_DEVICE_TABLE(of, ufs_qcom_of_match);
static const struct dev_pm_ops ufs_qcom_pm_ops = {
.suspend= ufshcd_pltfrm_suspend,
Looks good to me.
Reviewed-by: Subhash Jadavani
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: iscsi_trx going into D state
With the last patch it is getting hung up on wait_for_completion in target_wait_for_sess_cmds. I don't know what t_state or fabric state mean. To me it looks like a queue is not being emptied, but it would help if someone confirmed this and has some pointers on how to properly flush them when the communication is interrupted. [ 222.989134] Starting iscsit_close_connection. [ 222.993555] Calling flush_workqueue. [ 222.997703] Returned from flush_workqueue. [ 223.005802] isert_wait_conn calling ib_close_qp/ib_drain_qp. [ 223.011892] isert_wait_conn finished ib_close_qp/ib_drain_qp. [ 223.018063] isert_wait_conn calling isert_put_unsol_pending_cmds. [ 223.024574] isert_wait_conn returned from isert_put_unsol_pending_cmds. [ 223.031582] isert_wait_conn calling isert_wait4cmds. [ 223.036942] isert_wait4cmds calling target_sess_cmd_list_set_waiting. [ 223.043789] isert_wait4cmds returned from target_sess_cmd_list_set_waiting. [ 223.051135] isert_wait4cmds calling target_wait_for_sess_cmds. [ 223.057362] Waiting for se_cmd: 887ebf88bd00 t_state: 6, fabric state: 29 [ 223.064893] target_wait_for_sess_cmds calling spin_unlock_irqrestore. [ 223.071748] target_wait_for_sess_cmds calling wait_for_completion. [ 224.997636] Calling wait_for_common. [ 225.001936] Starting __wait_for_common. [ 225.006226] Calling do_wait_for_common. Thanks Robert LeBlanc PGP Fingerprint 79A2 9CA4 6CC4 45DD A904 C70E E654 3BB2 FA62 B9F1 On Tue, Jan 3, 2017 at 1:07 PM, Robert LeBlanc wrote: > With this patch I'm not seeing the __ib_drain_sq backtraces, but I'm > still seeing the previous backtraces. > > diff --git a/drivers/infiniband/ulp/isert/ib_isert.c > b/drivers/infiniband/ulp/isert/ib_isert.c > index 6dd43f6..1e53502 100644 > --- a/drivers/infiniband/ulp/isert/ib_isert.c > +++ b/drivers/infiniband/ulp/isert/ib_isert.c > @@ -2595,7 +2595,7 @@ static void isert_wait_conn(struct iscsi_conn *conn) >isert_conn_terminate(isert_conn); >mutex_unlock(&isert_conn->mutex); > > - ib_drain_qp(isert_conn->qp); > + ib_close_qp(isert_conn->qp); >isert_put_unsol_pending_cmds(conn); >isert_wait4cmds(conn); >isert_wait4logout(isert_conn); > > I was thinking that if the connection was brought down uncleanly then > there may be messages(??) it the send queue that would never be > consumed by the application, so it would never drain and would have to > be forcibly emptied. Maybe there is something stuck in the command > queue as well? > > [(support-1.0) root@prv-0-13-roberttest ~]# ps aux | grep " D " > root 15426 0.0 0.0 0 0 ?D12:48 0:00 [iscsi_np] > root 15429 0.0 0.0 0 0 ?D12:48 0:00 [iscsi_ttx] > root 16077 0.0 0.0 112656 2216 pts/0S+ 12:55 0:00 grep > --color=auto D > [(support-1.0) root@prv-0-13-roberttest ~]# cat /proc/15426/stack > [] iscsit_stop_session+0x1b1/0x1c0 > [] iscsi_check_for_session_reinstatement+0x1e2/0x270 > [] iscsi_target_check_for_existing_instances+0x30/0x40 > [] iscsi_target_do_login+0x138/0x630 > [] iscsi_target_start_negotiation+0x4e/0xa0 > [] __iscsi_target_login_thread+0x83e/0xf20 > [] iscsi_target_login_thread+0x24/0x30 > [] kthread+0xd9/0xf0 > [] ret_from_fork+0x25/0x30 > [] 0x > [(support-1.0) root@prv-0-13-roberttest ~]# cat /proc/15429/stack > [] target_wait_for_sess_cmds+0x49/0x190 > [] isert_wait_conn+0x1a4/0x2d0 [ib_isert] > [] iscsit_close_connection+0x157/0x860 > [] iscsit_take_action_for_connection_exit+0x7b/0xf0 > [] iscsi_target_tx_thread+0x150/0x1d0 > [] kthread+0xd9/0xf0 > [] ret_from_fork+0x25/0x30 > [] 0x > > Robert LeBlanc > PGP Fingerprint 79A2 9CA4 6CC4 45DD A904 C70E E654 3BB2 FA62 B9F1 > > > On Fri, Dec 30, 2016 at 4:07 PM, Robert LeBlanc wrote: >> I decided to try something completely different... Running the stock >> CentOS 3.10 kernel and OFED 3.4 on both hosts, I'm not seeing the hung >> processes and the tests complete successfully. The same seems to be >> true for the target on 4.9 and the initiator on 3.10. >> >> However, with the target on 3.10 and the initiator on 4.9, I get this >> on the target: >> >> [(support-1.0) root@prv-0-13-roberttest ~]# ps aux | grep " D " >> root 14791 0.0 0.0 0 0 ?D15:08 0:00 [iscsi_np] >> root 14795 0.0 0.0 0 0 ?D15:08 0:00 [iscsi_trx] >> root 14852 0.0 0.0 112648 976 pts/0S+ 15:11 0:00 grep >> --color=auto D >> [(support-1.0) root@prv-0-13-roberttest ~]# uname -a >> Linux prv-0-13-roberttest.betterservers.com 3.10.0-327.36.3.el7.x86_64 >> #1 SMP Mon Oct 24 16:09:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux >> [(support-1.0) root@prv-0-13-roberttest ~]# cat /proc/14791/stack >> [] iscsit_stop_session+0x1c8/0x1e0 [iscsi_target_mod] >> [] iscsi_check_for_session_reinstatement+0x1ea/0x280 >> [iscsi_target_mod] >> [] >> iscsi_target_check_for_existing_instances+0x35/0x40 [iscsi_tar
[PATCH v2] scsi: esas2r: Fix format string type mistakes
From: Emese Revfy
This adds the missing __printf attribute which allows compile time
format string checking (and will be used by the coming initify gcc
plugin). Additionally, this fixes the warnings exposed by the attribute.
Signed-off-by: Emese Revfy
[kees: split scsi/acpi, merged attr and fix, new commit messages]
Signed-off-by: Kees Cook
---
v2:
- fix %lu to %zu on sizeof() values, bart
---
drivers/scsi/esas2r/esas2r_init.c | 2 +-
drivers/scsi/esas2r/esas2r_ioctl.c | 2 +-
drivers/scsi/esas2r/esas2r_log.h | 4 ++--
drivers/scsi/esas2r/esas2r_main.c | 4 ++--
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/esas2r/esas2r_init.c
b/drivers/scsi/esas2r/esas2r_init.c
index d6e53aee2295..6432a50b26d8 100644
--- a/drivers/scsi/esas2r/esas2r_init.c
+++ b/drivers/scsi/esas2r/esas2r_init.c
@@ -237,7 +237,7 @@ static void esas2r_claim_interrupts(struct esas2r_adapter
*a)
flags |= IRQF_SHARED;
esas2r_log(ESAS2R_LOG_INFO,
- "esas2r_claim_interrupts irq=%d (%p, %s, %x)",
+ "esas2r_claim_interrupts irq=%d (%p, %s, %lx)",
a->pcid->irq, a, a->name, flags);
if (request_irq(a->pcid->irq,
diff --git a/drivers/scsi/esas2r/esas2r_ioctl.c
b/drivers/scsi/esas2r/esas2r_ioctl.c
index 3e8483410f61..b35ed3829421 100644
--- a/drivers/scsi/esas2r/esas2r_ioctl.c
+++ b/drivers/scsi/esas2r/esas2r_ioctl.c
@@ -1301,7 +1301,7 @@ int esas2r_ioctl_handler(void *hostdata, int cmd, void
__user *arg)
ioctl = kzalloc(sizeof(struct atto_express_ioctl), GFP_KERNEL);
if (ioctl == NULL) {
esas2r_log(ESAS2R_LOG_WARN,
- "ioctl_handler kzalloc failed for %d bytes",
+ "ioctl_handler kzalloc failed for %zu bytes",
sizeof(struct atto_express_ioctl));
return -ENOMEM;
}
diff --git a/drivers/scsi/esas2r/esas2r_log.h b/drivers/scsi/esas2r/esas2r_log.h
index 7b6397bb5b94..75b9d23cd736 100644
--- a/drivers/scsi/esas2r/esas2r_log.h
+++ b/drivers/scsi/esas2r/esas2r_log.h
@@ -61,8 +61,8 @@ enum {
#endif
};
-int esas2r_log(const long level, const char *format, ...);
-int esas2r_log_dev(const long level,
+__printf(2, 3) int esas2r_log(const long level, const char *format, ...);
+__printf(3, 4) int esas2r_log_dev(const long level,
const struct device *dev,
const char *format,
...);
diff --git a/drivers/scsi/esas2r/esas2r_main.c
b/drivers/scsi/esas2r/esas2r_main.c
index 5092c821d088..f2e9d8aa979c 100644
--- a/drivers/scsi/esas2r/esas2r_main.c
+++ b/drivers/scsi/esas2r/esas2r_main.c
@@ -198,7 +198,7 @@ static ssize_t write_hw(struct file *file, struct kobject
*kobj,
GFP_KERNEL);
if (a->local_atto_ioctl == NULL) {
esas2r_log(ESAS2R_LOG_WARN,
- "write_hw kzalloc failed for %d bytes",
+ "write_hw kzalloc failed for %zu bytes",
sizeof(struct atto_ioctl));
return -ENOMEM;
}
@@ -1186,7 +1186,7 @@ static int esas2r_dev_targ_reset(struct scsi_cmnd *cmd,
bool target_reset)
} else {
esas2r_log(ESAS2R_LOG_CRIT,
"unable to allocate a request for a "
- "device reset (%d:%d)!",
+ "device reset (%d:%llu)!",
cmd->device->id,
cmd->device->lun);
}
--
2.7.4
--
Kees Cook
Nexus Security
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[LSF/MM TOPIC ATTEND] kernel booting using remote storage is a mess
The process of booting a Linux kernel with remote storage (such as iSCSI or FCoE) seems unnecessarily complicated if end users can even figure out how to do it. This is of course exacerbated by the fact that every company seems to CNA cards differently despite the iBFT standard. If you add DM-Multipath on top of that, most bets are off on your chances of getting it to reliably work. I'd like to discuss how we can get this to work in a general way given the world of systemd that we live in now. -- Lee Duncan SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi/bfa: use designated initializers
On Wed, Dec 21, 2016 at 12:33 AM, Christoph Hellwig wrote: > On Fri, Dec 16, 2016 at 05:05:15PM -0800, Kees Cook wrote: >> Prepare to mark sensitive kernel structures for randomization by making >> sure they're using designated initializers. These were identified during >> allyesconfig builds of x86, arm, and arm64, with most initializer fixes >> extracted from grsecurity. > > Instead of further bloating the idiotic dispatch table just kill it off > entirely: Sounds fine to me! Is this going via your tree? Thanks! -Kees -- Kees Cook Nexus Security -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: mpt3sas: fix hang on ata passthru commands
On 01/01/2017 12:39 PM, James Bottomley wrote:
On Sun, 2017-01-01 at 11:33 -0500, David Miller wrote:
From: Bart Van Assche
Date: Sun, 1 Jan 2017 14:22:11 +
My recommendation is to revert commit 18f6084a989b ("scsi: mpt3sas: Fix
secure erase premature termination"). Since the mpt3sas driver uses the
single-queue approach and since the SCSI core unlocks the block layer
request queue lock before the .queuecommand callback function is called,
multiple threads can execute that callback function (scsih_qcmd() in this
case) simultaneously. This means that using scsi_internal_device_block()
from inside .queuecommand to serialize SCSI command execution is wrong.
But this causes a regression for the thing being fixed by that
commit.
Right, we don't do that; that's why I didn't list it as one of the
options.
Why don't we figure out what that semantics that commit was trying to
achieve and implement that properly?
Now that I look at the reviews, each of the reviewers said what the
correct thing to do was: return SAM_STAT_BUSY if SATL commands are
outstanding like the spec says. You all get negative brownie points
for not insisting on a rework.
Does this patch (compile tested only) fix the problems for everyone?
Hi,
Yes, with this patch applied my system boots :)
...
@@ -4083,6 +4077,16 @@ scsih_qcmd(struct Scsi_Host *shost, struct scsi_cmnd
*scmd)
return 0;
}
+ /*
+* Bug work around for firmware SATL handling
+*/
+ if (sas_device_priv_data->ata_command_pending) {
+ scmd->result = SAM_STAT_BUSY;
+ scmd->scsi_done(scmd);
+ return 0;
+ }
+ set_satl_pending(scmd, true);
+
sas_target_priv_data = sas_device_priv_data->sas_target;
/* invalid device handle */
I was also wondering if 2 threads could both fall through and do:
'set_satl_pending(scmd, true)'; ?
Afaict there is nothing preventing that race?
Thanks,
-Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: iscsi_trx going into D state
With this patch I'm not seeing the __ib_drain_sq backtraces, but I'm still seeing the previous backtraces. diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c index 6dd43f6..1e53502 100644 --- a/drivers/infiniband/ulp/isert/ib_isert.c +++ b/drivers/infiniband/ulp/isert/ib_isert.c @@ -2595,7 +2595,7 @@ static void isert_wait_conn(struct iscsi_conn *conn) isert_conn_terminate(isert_conn); mutex_unlock(&isert_conn->mutex); - ib_drain_qp(isert_conn->qp); + ib_close_qp(isert_conn->qp); isert_put_unsol_pending_cmds(conn); isert_wait4cmds(conn); isert_wait4logout(isert_conn); I was thinking that if the connection was brought down uncleanly then there may be messages(??) it the send queue that would never be consumed by the application, so it would never drain and would have to be forcibly emptied. Maybe there is something stuck in the command queue as well? [(support-1.0) root@prv-0-13-roberttest ~]# ps aux | grep " D " root 15426 0.0 0.0 0 0 ?D12:48 0:00 [iscsi_np] root 15429 0.0 0.0 0 0 ?D12:48 0:00 [iscsi_ttx] root 16077 0.0 0.0 112656 2216 pts/0S+ 12:55 0:00 grep --color=auto D [(support-1.0) root@prv-0-13-roberttest ~]# cat /proc/15426/stack [] iscsit_stop_session+0x1b1/0x1c0 [] iscsi_check_for_session_reinstatement+0x1e2/0x270 [] iscsi_target_check_for_existing_instances+0x30/0x40 [] iscsi_target_do_login+0x138/0x630 [] iscsi_target_start_negotiation+0x4e/0xa0 [] __iscsi_target_login_thread+0x83e/0xf20 [] iscsi_target_login_thread+0x24/0x30 [] kthread+0xd9/0xf0 [] ret_from_fork+0x25/0x30 [] 0x [(support-1.0) root@prv-0-13-roberttest ~]# cat /proc/15429/stack [] target_wait_for_sess_cmds+0x49/0x190 [] isert_wait_conn+0x1a4/0x2d0 [ib_isert] [] iscsit_close_connection+0x157/0x860 [] iscsit_take_action_for_connection_exit+0x7b/0xf0 [] iscsi_target_tx_thread+0x150/0x1d0 [] kthread+0xd9/0xf0 [] ret_from_fork+0x25/0x30 [] 0x Robert LeBlanc PGP Fingerprint 79A2 9CA4 6CC4 45DD A904 C70E E654 3BB2 FA62 B9F1 On Fri, Dec 30, 2016 at 4:07 PM, Robert LeBlanc wrote: > I decided to try something completely different... Running the stock > CentOS 3.10 kernel and OFED 3.4 on both hosts, I'm not seeing the hung > processes and the tests complete successfully. The same seems to be > true for the target on 4.9 and the initiator on 3.10. > > However, with the target on 3.10 and the initiator on 4.9, I get this > on the target: > > [(support-1.0) root@prv-0-13-roberttest ~]# ps aux | grep " D " > root 14791 0.0 0.0 0 0 ?D15:08 0:00 [iscsi_np] > root 14795 0.0 0.0 0 0 ?D15:08 0:00 [iscsi_trx] > root 14852 0.0 0.0 112648 976 pts/0S+ 15:11 0:00 grep > --color=auto D > [(support-1.0) root@prv-0-13-roberttest ~]# uname -a > Linux prv-0-13-roberttest.betterservers.com 3.10.0-327.36.3.el7.x86_64 > #1 SMP Mon Oct 24 16:09:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux > [(support-1.0) root@prv-0-13-roberttest ~]# cat /proc/14791/stack > [] iscsit_stop_session+0x1c8/0x1e0 [iscsi_target_mod] > [] iscsi_check_for_session_reinstatement+0x1ea/0x280 > [iscsi_target_mod] > [] > iscsi_target_check_for_existing_instances+0x35/0x40 [iscsi_target_mod] > [] iscsi_target_do_login+0x141/0x670 [iscsi_target_mod] > [] iscsi_target_start_negotiation+0x1c/0xb0 > [iscsi_target_mod] > [] iscsi_target_login_thread+0xadf/0x1050 [iscsi_target_mod] > [] kthread+0xcf/0xe0 > [] ret_from_fork+0x58/0x90 > [] 0x > [(support-1.0) root@prv-0-13-roberttest ~]# cat /proc/14795/stack > [] isert_wait4flush+0x79/0xc0 [ib_isert] > [] isert_wait_conn+0x5b/0x2d0 [ib_isert] > [] iscsit_close_connection+0x15d/0x820 [iscsi_target_mod] > [] iscsit_take_action_for_connection_exit+0x83/0x110 > [iscsi_target_mod] > [] iscsi_target_rx_thread+0x1e7/0xf80 [iscsi_target_mod] > [] kthread+0xcf/0xe0 > [] ret_from_fork+0x58/0x90 > [] 0x > > [ 345.970157] iSCSI Login timeout on Network Portal 0.0.0.0:3260 > [ 483.850714] INFO: task iscsi_np:14791 blocked for more than 120 seconds. > [ 483.857467] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" > disables this message. > [ 483.865326] iscsi_npD 0 14791 2 > 0x0004 > [ 483.872460] 886e3b117be0 0046 887ede579700 > 886e3b117fd8 > [ 483.879983] 886e3b117fd8 886e3b117fd8 887ede579700 > 883ef7898160 > [ 483.887500] 883ef7898168 7fff 887ede579700 > > [ 483.895025] Call Trace: > [ 483.897505] [] schedule+0x29/0x70 > [ 483.902496] [] schedule_timeout+0x209/0x2d0 > [ 483.908355] [] ? simple_strtoull+0x3b/0x70 > [ 483.914128] [] wait_for_completion+0x116/0x170 > [ 483.920253] [] ? wake_up_state+0x20/0x20 > [ 483.925847] [] iscsit_stop_session+0x1c8/0x1e0 > [iscsi_target_mod] > [ 483.933612] []
Re: [4.10, panic, regression] iscsi: null pointer deref at iscsi_tcp_segment_done+0x20d/0x2e0
On Mon 02-01-17 16:11:36, Johannes Weiner wrote: > On Fri, Dec 23, 2016 at 03:33:29AM -0500, Johannes Weiner wrote: > > On Fri, Dec 23, 2016 at 02:32:41AM -0500, Johannes Weiner wrote: > > > On Thu, Dec 22, 2016 at 12:22:27PM -0800, Hugh Dickins wrote: > > > > On Wed, 21 Dec 2016, Linus Torvalds wrote: > > > > > On Wed, Dec 21, 2016 at 9:13 PM, Dave Chinner > > > > > wrote: > > > > > > I unmounted the fs, mkfs'd it again, ran the > > > > > > workload again and about a minute in this fired: > > > > > > > > > > > > [628867.607417] [ cut here ] > > > > > > [628867.608603] WARNING: CPU: 2 PID: 16925 at mm/workingset.c:461 > > > > > > shadow_lru_isolate+0x171/0x220 > > > > > > > > > > Well, part of the changes during the merge window were the shadow > > > > > entry tracking changes that came in through Andrew's tree. Adding > > > > > Johannes Weiner to the participants. > > > > > > > > > > > Now, this workload does not touch the page cache at all - it's > > > > > > entirely an XFS metadata workload, so it should not really be > > > > > > affecting the working set code. > > > > > > > > > > Well, I suspect that anything that creates memory pressure will end up > > > > > triggering the working set code, so .. > > > > > > > > > > That said, obviously memory corruption could be involved and result in > > > > > random issues too, but I wouldn't really expect that in this code. > > > > > > > > > > It would probably be really useful to get more data points - is the > > > > > problem reliably in this area, or is it going to be random and all > > > > > over the place. > > > > > > > > Data point: kswapd got WARNING on mm/workingset.c:457 in > > > > shadow_lru_isolate, > > > > soon followed by NULL pointer deref in list_lru_isolate, one time when > > > > I tried out Sunday's git tree. Not seen since, I haven't had time to > > > > investigate, just set it aside as something to worry about if it happens > > > > again. But it looks like shadow_lru_isolate() has issues beyond Dave's > > > > case (I've no XFS and no iscsi), suspect unrelated to his other > > > > problems. > > > > > > This seems consistent with what Dave observed: we encounter regular > > > pages in radix tree nodes on the shadow LRU that should only contain > > > nodes full of exceptional shadow entries. It could be an issue in the > > > new slot replacement code and the node tracking callback. > > > > Both encounters seem to indicate use-after-free. Dave's node didn't > > warn about an unexpected node->count / node->exceptional state, but > > had entries that were inconsistent with that. Hugh got the counter > > warning but crashed on a list_head that's not NULLed in a live node. > > > > workingset_update_node() should be called on page cache radix tree > > leaf nodes that go empty. I must be missing an update_node callback > > where a leaf node gets freed somewhere. > > Sorry for dropping silent on this. I'm traveling over the holidays > with sporadic access to my emails and no access to real equipment. > > The times I managed to sneak away to look at the code didn't turn up > anything useful yet. > > Andrea encountered the warning as well and I gave him a debugging > patch (attached below), but he hasn't been able to reproduce this > condition. I've personally never seen the warning trigger, even though > the patches have been running on my main development machine for quite > a while now. Albeit against an older base; I've updated to Linus's > master branch now in case it's an interaction with other new code. > > If anybody manages to reproduce this, that would be helpful. Any extra > eyes on this would be much appreciated too until I'm back at my desk. I was looking into this but I didn't find a way how we could possibly leave radix tree node on LRU. So your debug patch looks like a good way forward. Honza -- Jan Kara SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] scsi: hisi_sas: lock sensitive region in hisi_sas_slot_abort()
When we call hisi_sas_slot_task_free() we should grab the
hisi_hba.lock, as hisi_sas_slot_task_free() accesses common
hisi_hba elements.
Function hisi_sas_slot_abort() is missing this, so add it.
Signed-off-by: John Garry
---
drivers/scsi/hisi_sas/hisi_sas_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c
b/drivers/scsi/hisi_sas/hisi_sas_main.c
index d50e9cf..22dba01 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -146,6 +146,7 @@ static void hisi_sas_slot_abort(struct work_struct *work)
struct scsi_lun lun;
struct device *dev = &hisi_hba->pdev->dev;
int tag = abort_slot->idx;
+ unsigned long flags;
if (!(task->task_proto & SAS_PROTOCOL_SSP)) {
dev_err(dev, "cannot abort slot for non-ssp task\n");
@@ -159,7 +160,9 @@ static void hisi_sas_slot_abort(struct work_struct *work)
hisi_sas_debug_issue_ssp_tmf(task->dev, lun.scsi_lun, &tmf_task);
out:
/* Do cleanup for this task */
+ spin_lock_irqsave(&hisi_hba->lock, flags);
hisi_sas_slot_task_free(hisi_hba, task, abort_slot);
+ spin_unlock_irqrestore(&hisi_hba->lock, flags);
if (task->task_done)
task->task_done(task);
if (sas_dev)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] scsi: hisi_sas: service v2 hw CQ ISR with tasklet
Currently the all the slot processing for the completion
queue is done in ISR context. It is judged that the slot
processing can take a long time, especially when a SATA
NCQ completes (upto 32 slots).
So, as a solution, defer the bulk of the ISR processing
to tasklet context. Each CQ will have its down tasklet.
Signed-off-by: John Garry
Reviewed-by: Xiang Chen
---
drivers/scsi/hisi_sas/hisi_sas.h | 1 +
drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 24 ++--
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
index c0cd505..9216dea 100644
--- a/drivers/scsi/hisi_sas/hisi_sas.h
+++ b/drivers/scsi/hisi_sas/hisi_sas.h
@@ -95,6 +95,7 @@ struct hisi_sas_port {
struct hisi_sas_cq {
struct hisi_hba *hisi_hba;
+ struct tasklet_struct tasklet;
int rd_point;
int id;
};
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index b934aec..e506260 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -2481,20 +2481,17 @@ static irqreturn_t fatal_axi_int_v2_hw(int irq_no, void
*p)
return IRQ_HANDLED;
}
-static irqreturn_t cq_interrupt_v2_hw(int irq_no, void *p)
+static void cq_tasklet_v2_hw(unsigned long val)
{
- struct hisi_sas_cq *cq = p;
+ struct hisi_sas_cq *cq = (struct hisi_sas_cq *)val;
struct hisi_hba *hisi_hba = cq->hisi_hba;
struct hisi_sas_slot *slot;
struct hisi_sas_itct *itct;
struct hisi_sas_complete_v2_hdr *complete_queue;
- u32 irq_value, rd_point = cq->rd_point, wr_point, dev_id;
+ u32 rd_point = cq->rd_point, wr_point, dev_id;
int queue = cq->id;
complete_queue = hisi_hba->complete_hdr[queue];
- irq_value = hisi_sas_read32(hisi_hba, OQ_INT_SRC);
-
- hisi_sas_write32(hisi_hba, OQ_INT_SRC, 1 << queue);
wr_point = hisi_sas_read32(hisi_hba, COMPL_Q_0_WR_PTR +
(0x14 * queue));
@@ -2545,6 +2542,18 @@ static irqreturn_t cq_interrupt_v2_hw(int irq_no, void
*p)
/* update rd_point */
cq->rd_point = rd_point;
hisi_sas_write32(hisi_hba, COMPL_Q_0_RD_PTR + (0x14 * queue), rd_point);
+}
+
+static irqreturn_t cq_interrupt_v2_hw(int irq_no, void *p)
+{
+ struct hisi_sas_cq *cq = p;
+ struct hisi_hba *hisi_hba = cq->hisi_hba;
+ int queue = cq->id;
+
+ hisi_sas_write32(hisi_hba, OQ_INT_SRC, 1 << queue);
+
+ tasklet_schedule(&cq->tasklet);
+
return IRQ_HANDLED;
}
@@ -2726,6 +2735,8 @@ static int interrupt_init_v2_hw(struct hisi_hba *hisi_hba)
for (i = 0; i < hisi_hba->queue_count; i++) {
int idx = i + 96; /* First cq interrupt is irq96 */
+ struct hisi_sas_cq *cq = &hisi_hba->cq[i];
+ struct tasklet_struct *t = &cq->tasklet;
irq = irq_map[idx];
if (!irq) {
@@ -2742,6 +2753,7 @@ static int interrupt_init_v2_hw(struct hisi_hba *hisi_hba)
irq, rc);
return -ENOENT;
}
+ tasklet_init(t, cq_tasklet_v2_hw, (unsigned long)cq);
}
return 0;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] hisi_sas: some CQ processing fixes
This patchset fixes some issues related to servicing of the completion queue interrupt. The major fix is that sensitive hisi_hba structures need to be locked when free'ing a slot. Another modification is that the v2 hw completion queue irq is now serviced with a tasklet, as too much work was being done in the ISR. John Garry (3): scsi: hisi_sas: service v2 hw CQ ISR with tasklet scsi: hisi_sas: lock sensitive regions when servicing CQ interrupt scsi: hisi_sas: lock sensitive region in hisi_sas_slot_abort() drivers/scsi/hisi_sas/hisi_sas.h | 1 + drivers/scsi/hisi_sas/hisi_sas_main.c | 3 +++ drivers/scsi/hisi_sas/hisi_sas_v1_hw.c | 2 ++ drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 26 -- 4 files changed, 26 insertions(+), 6 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] scsi: hisi_sas: lock sensitive regions when servicing CQ interrupt
There is a bug in the current driver in that certain hisi_hba and port structure elements which we access when servicing the CQ interrupt do not use thread-safe accesses; these include hisi_sas_port linked-list of active slots (hisi_sas_port.entry), bitmap of currently allocated IPTT (in hisi_hba.slot_index_tags), and completion queue read pointer. As a solution, lock these elements with the hisi_hba.lock. Signed-off-by: John Garry Reviewed-by: Xiang Chen --- drivers/scsi/hisi_sas/hisi_sas_v1_hw.c | 2 ++ drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c index 8a1be0b..854fbea 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c +++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c @@ -1596,6 +1596,7 @@ static irqreturn_t cq_interrupt_v1_hw(int irq, void *p) hisi_hba->complete_hdr[queue]; u32 irq_value, rd_point = cq->rd_point, wr_point; + spin_lock(&hisi_hba->lock); irq_value = hisi_sas_read32(hisi_hba, OQ_INT_SRC); hisi_sas_write32(hisi_hba, OQ_INT_SRC, 1 << queue); @@ -1628,6 +1629,7 @@ static irqreturn_t cq_interrupt_v1_hw(int irq, void *p) /* update rd_point */ cq->rd_point = rd_point; hisi_sas_write32(hisi_hba, COMPL_Q_0_RD_PTR + (0x14 * queue), rd_point); + spin_unlock(&hisi_hba->lock); return IRQ_HANDLED; } diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c index e506260..69b0f06 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c @@ -2493,6 +2493,7 @@ static void cq_tasklet_v2_hw(unsigned long val) complete_queue = hisi_hba->complete_hdr[queue]; + spin_lock(&hisi_hba->lock); wr_point = hisi_sas_read32(hisi_hba, COMPL_Q_0_WR_PTR + (0x14 * queue)); @@ -2542,6 +2543,7 @@ static void cq_tasklet_v2_hw(unsigned long val) /* update rd_point */ cq->rd_point = rd_point; hisi_sas_write32(hisi_hba, COMPL_Q_0_RD_PTR + (0x14 * queue), rd_point); + spin_unlock(&hisi_hba->lock); } static irqreturn_t cq_interrupt_v2_hw(int irq_no, void *p) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
