The timeout handler and the done function are racing. When
qla2x00_async_iocb_timeout() starts to run it can be preempted by the
normal response path (via the firmware?). qla24xx_async_gpsc_sp_done()
releases the SRB unconditionally. When scheduling back to
qla2x00_async_iocb_timeout() qla24xx_async_abort_cmd() will access an freed
sp->qpair pointer:
qla2xxx [0000:83:00.0]-2871:0: Async-gpsc timeout - hdl=63d portid=234500 50:06:0e:80:08:77:b6:21.
qla2xxx [0000:83:00.0]-2853:0: Async done-gpsc res 0, WWPN 50:06:0e:80:08:77:b6:21
qla2xxx [0000:83:00.0]-2854:0: Async-gpsc OUT WWPN 20:45:00:27:f8:75:33:00 speeds=2c00 speed=0400.
qla2xxx [0000:83:00.0]-28d8:0: qla24xx_handle_gpsc_event 50:06:0e:80:08:77:b6:21 DS 7 LS 6 rc 0 login 1|1 rscn 1|0 lid 5
BUG: unable to handle kernel NULL pointer dereference at 0000000000000004
IP: qla24xx_async_abort_cmd+0x1b/0x1c0 [qla2xxx]
Obvious solution to this is to introduce a reference counter. One reference
is taken for the normal code path (the 'good' case) and one for the timeout
path. As we always race between the normal good case and the timeout/abort
handler we need to serialize it. Also we cannot assume any order between
the handlers. Since this is slow path we can use proper synchronization via
locks.
When we are able to cancel a timer (del_timer returns 1) we know there
can't be any error handling in progress because the timeout handler hasn't
expired yet, thus we can safely decrement the refcounter by one.
If we are not able to cancel the timer, we know an abort handler is
running. We have to make sure we call sp->done() in the abort handlers
before calling kref_put().
Link: https://lore.kernel.org/r/20220110050218.3958-3-njavali@marvell.com
Cc: stable@vger.kernel.org
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Co-developed-by: Daniel Wagner <dwagner@suse.de>
Signed-off-by: Daniel Wagner <dwagner@suse.de>
Signed-off-by: Saurav Kashyap <skashyap@marvell.com>
Signed-off-by: Nilesh Javali <njavali@marvell.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
[ commit 31e6cdbe0eae upstream ]
Instead of using the SCp.ptr field to track whether or not a command is
in flight, use the sp->type field to track this information. sp->type
must be set for proper operation of the qla2xxx driver. See e.g. the
switch (sp->type) statement in qla2x00_ct_entry().
Port the commit 8cc7b18f ("qla2x00t-32gbit: Stop using the SCSI pointer")
to the old qlogic driver.
Support for the following scsi changes in the Linux Kernel v5.18:
- ce70fd9a551a ("scsi: core: Remove the cmd field from struct scsi_request")
- 5b794f98074a ("scsi: core: Remove the sense and sense_len fields from struct scsi_request")
- 6aded12b10e0 ("scsi: core: Remove struct scsi_request")
The use of set_fs() for scst_write_file_transactional() was dropped in
94018a42 ("scst: Use kernel_{read,write}() instead of scst_{read,write}()").
Hence rename the goto label associated with this functionality.
Support for the following block/fs changes in the Linux Kernel v5.18:
- 609be1066731 ("block: pass a block_device and opf to bio_alloc_bioset")
- 07888c665b40 ("block: pass a block_device and opf to bio_alloc")
- cbcc268bb1ce ("fs: Move many prototypes to pagemap.h")
There is a regular need in the kernel to provide a way to declare
having a dynamically sized set of trailing elements in a structure.
Kernel code should always use “flexible array members”[1] for these
cases. The older style of one-element or zero-length arrays should
no longer be used[2].
This code was transformed with the help of Coccinelle:
($ spatch --jobs $(getconf _NPROCESSORS_ONLN) --sp-file script.cocci --include-headers --dir . > output.patch)
@@
identifier S, member, array;
type T1, T2;
@@
struct S {
...
T1 member;
T2 array[
- 0
];
};
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/78
Add the opportunity to globally specify multiple addresses, which
iSCSI/iSER portal will listen on.
There is a way to specify the addresses for a target, on which
portals it will be available, using the allowed_portal parameter.
There is also a way to set this globally via the iscsi-scstd
--address parameter. The problem is that the last option allows
you to specify only one local address. So extend this functionality
to specify more then one address.
Also increase the maximum number of listening addresses to 32.
Signed-off-by: Aleksandr Dyadyushkin <alextalker@ya.ru>
For the old qla2x00 driver, the hw_target attribute allows you to
distinguish hardware and virtual targets.
But for the new qla2x00t-32gbit driver, this attribute turned out
to be broken. Regardless of whether it is a hardware or virtual target,
this attribute is created for that target and always returns 1.
Restore this broken functionality by returning 1 for the hardware target
and 0 otherwise.
Unbreak the build for the previous commit against kernel versions
before v4.15.
See also commit ac7fe82b6fcf ("nvme-fc: add a dev_loss_tmo field
to the remoteport") v4.15.
Decrementing the tgt_dev_cmd_count ref happens in
scst_pre_xmit_response1(). This may lead to a race condition between
scst_acg_repl_lun() and scst_tgt_cmd_done()'s cmd processing, which
may lead to a general protection fault.
cmd processing replace LUN
-------------- -----------
[1] atomic_dec(tgt_dev->tgt_dev_cmd_count)
[2] scst_wait_for_tgt_devs()
[3] scst_free_tgt_dev()
[4] scst_tgt_dev_stop_threads()
[5] scst_tgt_cmd_done(cmd, SCST_CONTEXT_THREAD)
[6] scst_process_redirect_cmd()
[7] if (cmd->cmd_thr != NULL) {
[8] active_cmd_list = &cmd->cmd_thr->thr_active_cmd_list;
cmd->cmd_thr is already dead pointer here,
because it is pointing on thread which was freed
in scst_tgt_dev_stop_threads().
[9] list_add(&cmd->cmd_list_entry, active_cmd_list);
general protection fault: 0000
RIP: 0010:__list_add_valid+0x1/0x74
Call Trace:
scst_tgt_cmd_done+0x12b/0x160 [scst]
...
Hence, to avoid this race condition, move the decrement of
tgt_dev_cmd_count from the SCST_CMD_STATE_PRE_XMIT_RESP1 to
the SCST_CMD_STATE_FINISHED state, which is assigned after
scst_tgt_cmd_done() was called.
We check the recopied payload_len with the length of
struct scst_event + payload.
if (event->payload_len != event_len)
This check will never succeed.
So check the recopied payload length with the passed
payload_len from user space.
Fixes: ffd85476 ("scst: Suppress a false positive Coverity memory corruption complaint")
We copy struct scst_event with the payload from user space, but
use event_entry_len as the length of this buffer.
event_entry_len contains the length of struct scst_event_entry and
the payload. struct scst_event is part of struct scst_event_entry
and is therefore shorter in length.
Thus, use the length of struct scst_event + payload when
copying the event from user space.
This patch should fix the following bug:
usercopy: Kernel memory overwrite attempt detected to SLUB object 'kmalloc-512' (offset 232, size 296)!
------------[ cut here ]------------
kernel BUG at mm/usercopy.c:102!
...
RIP: 0010:usercopy_abort+0x74/0x76
Call Trace:
__check_heap_object+0xd3/0x100
__check_object_size+0xff/0x16b
scst_event_get_event_from_user+0xbb/0x2e0 [scst]
scst_event_ioctl+0x5f1/0xde0 [scst]
do_vfs_ioctl+0xa4/0x680
? syscall_trace_enter+0x1d3/0x2c0
ksys_ioctl+0x60/0x90
__x64_sys_ioctl+0x16/0x20
do_syscall_64+0x5b/0x1a0
entry_SYSCALL_64_after_hwframe+0x65/0xca
Fixes: ffd85476 ("scst: Suppress a false positive Coverity memory corruption complaint")
Fixes: https://github.com/SCST-project/scst/issues/25
The thin_provisioned != dev_thin_provisioned condition isn't the best way
for marking manually set thin_provisioned attribute.
-----------------------------------
| thin_prov | dev_thin_prov | key |
----------------------------------
| 0, 1 | 0 | No |
-----------------------------------
| 1 | 1 | No |
-----------------------------------
| 0 | 1 | Yes |
-----------------------------------
The thin_prov=1, dev_thin_prov=0 case leads to thin_prov will be set to
0 due to internal checking.
So, it's possible to see the value of the thin_provisioned attribute
via scstadmin only in the one case (3 row).
It's especially unclear for the fileio devices, because they support
thin provisioning only when the corresponding attribute is manually set.
Thus, to clarify the using of the thin_provisioned attribute always
display it when manually set.
The ini_unchecked_isa_dma in the scst_alloc_add_tgt_dev() may be
used as an uninitialized variable for kernel versions since v5.13.
Fixes: 2a775c42 ("scst: Port to Linux kernel v5.13")
Detected by smatch.
Current driver does unnecessary pause for each session to get to certain
state before allowing the app start call to return. In larger environment,
this introduces a long delay. Originally the delay was meant to
synchronize app and driver. However, the with current implementation the
two sides use various events to synchronize their state.
The same is applied to the authentication failure call.
Link: https://lore.kernel.org/r/20211026115412.27691-6-njavali@marvell.com
Fixes: 4de067e5df12 ("scsi: qla2xxx: edif: Add N2N support for EDIF")
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Signed-off-by: Quinn Tran <qutran@marvell.com>
Signed-off-by: Nilesh Javali <njavali@marvell.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
[ commit b492d6a4880f upstream ]
When user uses issue_lip to do link bounce, driver sends additional target
reset to remote device before resetting the link. The target reset would
affect other paths with active I/Os. This patch will remove the unnecessary
target reset.
Link: https://lore.kernel.org/r/20211026115412.27691-4-njavali@marvell.com
Fixes: 5854771e314e ("[SCSI] qla2xxx: Add ISPFX00 specific bus reset routine")
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Signed-off-by: Quinn Tran <qutran@marvell.com>
Signed-off-by: Nilesh Javali <njavali@marvell.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
[ commit 0b7a9fd934a6 upstream ]
For RSCN of type "Area, Domain, or Fabric", which indicate a portion or
entire fabric was disturbed, current driver does not set the scan_need flag
to indicate a session was affected by the disturbance. This in turn can
lead to I/O timeout and delay of relogin. Hence initiate relogin in the
event of fabric disturbance.
Link: https://lore.kernel.org/r/20211026115412.27691-2-njavali@marvell.com
Fixes: 1560bafdff9e ("scsi: qla2xxx: Use complete switch scan for RSCN events")
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Signed-off-by: Quinn Tran <qutran@marvell.com>
Signed-off-by: Nilesh Javali <njavali@marvell.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
[ commit bb2ca6b3f09a upstream ]
Implement ->map queues and use the block layer blk_mq_pci_map_queues
helper for mapping queues to CPUs.
With this mapping minimum 10%+ increase in performance is noticed.
Signed-off-by: Saurav Kashyap <skashyap@marvell.com>
Signed-off-by: Nilesh Javali <njavali@marvell.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
[ commit 2b2af50ae836 upstream ]
The sgl is freed in the target stack in target_release_cmd_kref() before
calling qlt_free_cmd() but there is an unmap of sgl in qlt_free_cmd() that
causes a panic if sgl is not yet DMA unmapped:
NIP dma_direct_unmap_sg+0xdc/0x180
LR dma_direct_unmap_sg+0xc8/0x180
Call Trace:
ql_dbg_prefix+0x68/0xc0 [qla2xxx] (unreliable)
dma_unmap_sg_attrs+0x54/0xf0
qlt_unmap_sg.part.19+0x54/0x1c0 [qla2xxx]
qlt_free_cmd+0x124/0x1d0 [qla2xxx]
tcm_qla2xxx_release_cmd+0x4c/0xa0 [tcm_qla2xxx]
target_put_sess_cmd+0x198/0x370 [target_core_mod]
transport_generic_free_cmd+0x6c/0x1b0 [target_core_mod]
tcm_qla2xxx_complete_free+0x6c/0x90 [tcm_qla2xxx]
The sgl may be left unmapped in error cases of response sending. For
instance, qlt_rdy_to_xfer() maps sgl and exits when session is being
deleted keeping the sgl mapped.
This patch removes use-after-free of the sgl and ensures that the sgl is
unmapped for any command that was not sent to firmware.
Link: https://lore.kernel.org/r/20211018122650.11846-1-d.bogdanov@yadro.com
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
[ commit 4a8f71014b4d upstream ]
Commit 8c0eb596baa5 ("[SCSI] qla2xxx: Fix a memory leak in an error path of
qla2x00_process_els()"), intended to change:
bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN
to:
bsg_job->request->msgcode != FC_BSG_RPT_ELS
but changed it to:
bsg_job->request->msgcode == FC_BSG_RPT_ELS
instead.
Change the == to a != to avoid leaking the fcport structure or freeing
unallocated memory.
Link: https://lore.kernel.org/r/20211012191834.90306-2-jgu@purestorage.com
Fixes: 8c0eb596baa5 ("[SCSI] qla2xxx: Fix a memory leak in an error path of qla2x00_process_els()")
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Joy Gu <jgu@purestorage.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
[ commit 7fb223d0ad80 upstream ]
