Now that spc_emulate_request_sense has been taught to process zero-length
REQUEST SENSE correctly, drop the special handling of unit attention
conditions from transport_generic_new_cmd. However, for now REQUEST SENSE
will be the only command that goes through emulation for zero lengths.
(nab: Fix up zero-length check in transport_generic_new_cmd)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Similar to INQUIRY and MODE SENSE, construct the sense data in a
buffer and later copy it to the scatterlist. Do not do anything,
but still clear a pending unit attention condition, if the allocation
length is zero.
However, SPC tells us that "If a REQUEST SENSE command is terminated with
CHECK CONDITION status [and] the REQUEST SENSE command was received on
an I_T nexus with a pending unit attention condition (i.e., before the
device server reports CHECK CONDITION status), then the device server
shall not clear the pending unit attention condition." Do the
transport_kmap_data_sg early to detect this case.
It also tells us "Device servers shall not adjust the additional sense
length to reflect truncation if the allocation length is less than the
sense data available", so do not do that! Note that the err variable
is write-only.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
In order to support zero-size allocation lengths, do not assert
that we have a scatterlist until after checking cmd->data_length.
But once we do this, we can have two cases of transport_kmap_data_sg
returning NULL: a zero-size allocation length, or an out-of-memory
condition. Report the latter using sense codes, so that the SCSI
command that triggered it will fail.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
SPC says:
"The ALLOCATION LENGTH field is defined in 4.3.5.6. The allocation length
should be at least 16. Device servers compliant with SPC return CHECK
CONDITION status, with the sense key set to ILLEGAL REQUEST, and the
additional sense code set to INVALID FIELD IN CDB when the allocation
length is less than 16 bytes".
Testcase: sg_raw -r8 /dev/sdb a0 00 00 00 00 00 00 00 00 08 00 00
should fail with ILLEGAL REQUEST / INVALID FIELD IN CDB sense
does not fail without the patch
fails correctly with the patch
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Several places were not checking that the parameter list length
was large enough, and thus accessing invalid memory. Zero-length
parameter lists are just a special case of this.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Right now, commands with a zero-size payload are skipped completely.
This is wrong; such commands should be passed down to the device and
processed normally.
For physical backends, this ignores completely things such as START
STOP UNIT. For virtual backends, we have a hack in place to clear a
unit attention state on a zero-size REQUEST SENSE, but we still do
not report errors properly on zero-length commands---out-of-bounds
0-block reads and writes, too small parameter list lengths, etc.
This patch fixes this for PSCSI. Uses of transport_kmap_data_sg are
guarded with a check for non-zero cmd->data_length; for all other
commands a zero length is handled properly in pscsi_execute_cmd.
The sole exception will be for now REPORT LUNS, which is handled
through the normal SPC emulation.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
The pointer to the sense buffer is fetched by transport_get_sense_data,
but this is called by target_complete_ok_work long after pscsi_req_done
has freed the struct that contains it.
Pass instead the fabric's sense buffer to transport_complete,
and copy the data to it directly in transport_complete. Setting
SCF_TRANSPORT_TASK_SENSE also becomes a duty of transport_complete.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
The error conditions in transport_get_sense_data are superfluous
and complicate the code unnecessarily:
* SCF_TRANSPORT_TASK_SENSE is checked in the caller;
* it's simply part of the invariants of dev->transport->get_sense_buffer
that it must be there if transport_complete ever returns 1, and that
it must not return NULL. Besides, the entire callback will disappear
with the next patch.
* similarly in the caller we can expect that sense data is only sent
for non-zero cmd->scsi_status.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
We will be calling it from transport_complete_cmd, avoid forward
declarations. No semantic change.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
This patch updates iscsi_login_zero_tsih_s1() usage for generating
iscsi_session->session_index to properly check the return value from
idr_get_new(), and reject the iSCSI login attempt with exception
status ISCSI_LOGIN_STATUS_NO_RESOURCES in the event of a failure.
Signed-off-by: Benjamin Wang <cpwang2009@gmail.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
This patch fixes a long-standing bug with SCSI overflow handling
where se_cmd->data_length was incorrectly being re-assigned to
the larger CDB extracted allocation length, resulting in a number
of fabric level errors that would end up causing a session reset
in most cases. So instead now:
- Only re-assign se_cmd->data_length durining UNDERFLOW (to use the
smaller value)
- Use existing se_cmd->data_length for OVERFLOW (to use the smaller
value)
This fix has been tested with the following CDB to generate an
SCSI overflow:
sg_raw -r512 /dev/sdc 28 0 0 0 0 0 0 0 9 0
Tested using iscsi-target, tcm_qla2xxx, loopback and tcm_vhost fabric
ports. Here is a bit more detail on each case:
- iscsi-target: Bug with open-iscsi with overflow, sg_raw returns
-3584 bytes of data.
- tcm_qla2xxx: Working as expected, returnins 512 bytes of data
- loopback: sg_raw returns CHECK_CONDITION, from overflow rejection
in transport_generic_map_mem_to_cmd()
- tcm_vhost: Same as loopback
Reported-by: Roland Dreier <roland@purestorage.com>
Cc: Roland Dreier <roland@purestorage.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Boaz Harrosh <bharrosh@panasas.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
This was originally for helping fabrics to determine overflow/underflow
status, and has been superceeded by SCF_OVERFLOW_BIT + SCF_UNDERFLOW_BIT.
Signed-off-by: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Use rcu_dereference_protected in order to prevent lockdep
complaint. Sequel of the patch 863555be
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Denis Efremov <yefremov.denis@gmail.com>
Acked-by: Mark D. Rustad <mark.d.rustad@intel.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
This patch fixes a regression bug with the handling of zero-length
data CDBs within transport_generic_new_cmd() code. The bug was introduced
with the following commit as part of the single task conversion work:
commit 4101f0a89d
Author: Christoph Hellwig <hch@infradead.org>
Date: Tue Apr 24 00:25:03 2012 -0400
target: always allocate a single task
where the zero-length check for SCF_SCSI_DATA_SG_IO_CDB was incorrectly
changed to SCF_SCSI_CONTROL_SG_IO_CDB because of the seperate comment
in transport_generic_new_cmd() wrt to control CDBs zero-length handling
introduced in:
commit 91ec1d3535
Author: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Fri Jan 13 12:01:34 2012 -0800
target: Add workaround for zero-length control CDB handling
So go ahead and change transport_generic_new_cmd() to handle control+data
zero-length CDBs in the same manner for this special case.
Tested with iscsi-target + loopback fabric port LUNs on 3.6-rc0 code.
This patch will also need to be picked up for 3.5-stable.
(hch: Add proper comment in transport_generic_new_cmd)
Cc: Christoph Hellwig <hch@lst.de>
Cc: Roland Dreier <roland@purestorage.com>
Cc: Andy Grover <agrover@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
This patch fixes a regression bug in pscsi_transport_complete() callback
code where *pt was being NULL dereferenced during REPORT_LUNS handling,
that was introduced with the spc/sbc refactoring in:
commit 1fd032ee10
Author: Christoph Hellwig <hch@infradead.org>
Date: Sun May 20 11:59:15 2012 -0400
target: move code for CDB emulation
As this is a special case for pscsi_parse_cdb() to call spc_parse_cdb() to
allow TCM to handle REPORT_LUN emulation, pscsi_plugin_task will have not
been allocated..
So now in pscsi_transport_complete() just check for existence of *pt and
return for this special case.
Reported-by: Alex Elsayed <eternaleye+usenet@gmail.com>
Cc: Alex Elsayed <eternaleye+usenet@gmail.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
I am hitting this bug when the target is low in memory that fails the
alloc_page() for the newly submitted command. This is a sort of off-by-one
bug causing NULL pointer dereference in __free_page() since 'i' here is
really the counter of total pages that have been successfully allocated here.
Signed-off-by: Yi Zou <yi.zou@intel.com>
Cc: Andy Grover <agrover@redhat.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Open-FCoE.org <devel@open-fcoe.org>
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Stop doing a pile of work related to debugging messages when
the ft_debug_logging flag is not set. Use unlikely to add the
check in a way that the check can be inlined without inlining the
whole thing.
Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Pull second vfs pile from Al Viro:
"The stuff in there: fsfreeze deadlock fixes by Jan (essentially, the
deadlock reproduced by xfstests 068), symlink and hardlink restriction
patches, plus assorted cleanups and fixes.
Note that another fsfreeze deadlock (emergency thaw one) is *not*
dealt with - the series by Fernando conflicts a lot with Jan's, breaks
userland ABI (FIFREEZE semantics gets changed) and trades the deadlock
for massive vfsmount leak; this is going to be handled next cycle.
There probably will be another pull request, but that stuff won't be
in it."
Fix up trivial conflicts due to unrelated changes next to each other in
drivers/{staging/gdm72xx/usb_boot.c, usb/gadget/storage_common.c}
* 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs: (54 commits)
delousing target_core_file a bit
Documentation: Correct s_umount state for freeze_fs/unfreeze_fs
fs: Remove old freezing mechanism
ext2: Implement freezing
btrfs: Convert to new freezing mechanism
nilfs2: Convert to new freezing mechanism
ntfs: Convert to new freezing mechanism
fuse: Convert to new freezing mechanism
gfs2: Convert to new freezing mechanism
ocfs2: Convert to new freezing mechanism
xfs: Convert to new freezing code
ext4: Convert to new freezing mechanism
fs: Protect write paths by sb_start_write - sb_end_write
fs: Skip atime update on frozen filesystem
fs: Add freezing handling to mnt_want_write() / mnt_drop_write()
fs: Improve filesystem freezing handling
switch the protection of percpu_counter list to spinlock
nfsd: Push mnt_want_write() outside of i_mutex
btrfs: Push mnt_want_write() outside of i_mutex
fat: Push mnt_want_write() outside of i_mutex
...
* set_fs(KERNEL_DS) + getname() is probably the weirdest implementation
of strdup() I've seen. Especially since they don't to copy it at all...
* filp_open() never returns NULL; it's ERR_PTR(-E...) on failure.
* file->f_dentry is never going to be NULL, TYVM.
* match_strdup() + snprintf() + kfree() is a bloody weird way to spell
match_strlcpy().
Pox on cargo-cult programmers...
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
From Al Viro:
BTW, speaking of struct file treatment related to sockets -
there's this piece of code in iscsi:
/*
* The SCTP stack needs struct socket->file.
*/
if ((np->np_network_transport == ISCSI_SCTP_TCP) ||
(np->np_network_transport == ISCSI_SCTP_UDP)) {
if (!new_sock->file) {
new_sock->file = kzalloc(
sizeof(struct file), GFP_KERNEL);
For one thing, as far as I can see it'not true - sctp does *not* depend on
socket->file being non-NULL; it does, in one place, check socket->file->f_flags
for O_NONBLOCK, but there it treats NULL socket->file as "flag not set".
Which is the case here anyway - the fake struct file created in
__iscsi_target_login_thread() (and in iscsi_target_setup_login_socket(), with
the same excuse) do *not* get that flag set.
Moreover, it's a bloody serious violation of a bunch of asserts in VFS;
all struct file instances should come from filp_cachep, via get_empty_filp()
(or alloc_file(), which is a wrapper for it). FWIW, I'm very tempted to
do this and be done with the entire mess:
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andy Grover <agrover@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Christoph Hellwig <hch@lst.de>
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
During a failure in transport_add_device_to_core_hba() code, we called
destroy_workqueue(dev->tmr_wq) before ->tmr_wq was allocated which leads
to an oops.
This fixes a regression introduced in with:
commit af8772926f
Author: Christoph Hellwig <hch@infradead.org>
Date: Sun Jul 8 15:58:49 2012 -0400
target: replace the processing thread with a TMR work queue
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
We want it to be possible for target_submit_cmd() to return errors up
to its fabric module callers. For now just update the prototype to
return an int, and update all callers to handle non-zero return values
as an error.
This is immediately useful for tcm_qla2xxx to fix a long-standing active
I/O session shutdown race, but tcm_fc, usb-gadget, and sbp-target the
fabric maintainers need to check + ACK that handling a target_submit_cmd()
failure due to session shutdown does not introduce regressions
(nab: Respin against for-next after initial NACK + update docbook comment +
fix double se_cmd init in exception path for usb-gadget)
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Arun Easi <arun.easi@qlogic.com>
Cc: Chris Boot <bootc@bootc.net>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Mark Rustad <mark.d.rustad@intel.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Andy Grover <agrover@redhat.com>
Signed-off-by: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Fail UNMAP commands that have more than our reported limit on unmap
descriptors.
Signed-off-by: Roland Dreier <roland@purestorage.com>
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
It's possible for an initiator to send us an UNMAP command with a
descriptor that is less than 8 bytes; in that case it's really bad for
us to set an unsigned int to that value, subtract 8 from it, and then
use that as a limit for our loop (since the value will wrap around to
a huge positive value).
Fix this by making size be signed and only looping if size >= 16 (ie
if we have at least a full descriptor available).
Also remove offset as an obfuscated name for the constant 8.
Signed-off-by: Roland Dreier <roland@purestorage.com>
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
The UNMAP DATA LENGTH and UNMAP BLOCK DESCRIPTOR DATA LENGTH fields
are in the unmap descriptor (the payload transferred to our data out
buffer), not in the CDB itself. Read them from the correct place in
target_emulated_unmap.
Signed-off-by: Roland Dreier <roland@purestorage.com>
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
When processing an UNMAP command, we need to make sure that the number
of blocks we're asked to UNMAP does not exceed our reported maximum
number of blocks per UNMAP, and that the range of blocks we're
unmapping doesn't go past the end of the device.
Signed-off-by: Roland Dreier <roland@purestorage.com>
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Many SCSI commands are defined to return a CHECK CONDITION / ILLEGAL
REQUEST with ASC set to LOGICAL BLOCK ADDRESS OUT OF RANGE if the
initiator sends a command that accesses a too-big LBA. Add an enum
value and case entries so that target code can return this status.
Signed-off-by: Roland Dreier <roland@purestorage.com>
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Since we set se_session.sess_tearing_down and stop new commands from
being added to se_session.sess_cmd_list before we wait for commands to
finish when freeing a session, there's no need for a separate
sess_wait_list -- if we let new commands be added to sess_cmd_list
after setting sess_tearing_down, that would be a bug that breaks the
logic of waiting in-flight commands.
Also rename target_splice_sess_cmd_list() to
target_sess_cmd_list_set_waiting(), since we are no longer splicing
onto a separate list.
Signed-off-by: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Target core code assumes that target_splice_sess_cmd_list() has set
sess_tearing_down and moved the list of pending commands to
sess_wait_list, no more commands will be added to the session; if any
are added, nothing keeps the se_session from being freed while the
command is still in flight, which e.g. leads to use-after-free of
se_cmd->se_sess in target_release_cmd_kref().
To enforce this invariant, put a check of sess_tearing_down inside of
sess_cmd_lock in target_get_sess_cmd(); any checks before this are
racy and can lead to the use-after-free described above. For example,
the qla_target check in qlt_do_work() checks sess_tearing_down from
work thread context but then drops all locks before calling
target_submit_cmd() (as it must, since that is a sleeping function).
However, since no locks are held, anything can happen with respect to
the session it has looked up -- although it does correctly get
sess_kref within its lock, so the memory won't be freed while
target_submit_cmd() is actually running, nothing stops eg an ACL from
being dropped and calling ->shutdown_session() (which calls into
target_splice_sess_cmd_list()) before we get to target_get_sess_cmd().
Once this happens, the se_session memory can be freed as soon as
target_submit_cmd() returns and qlt_do_work() drops its reference,
even though we've just added a command to sess_cmd_list.
To prevent this use-after-free, check sess_tearing_down inside of
sess_cmd_lock right before target_get_sess_cmd() adds a command to
sess_cmd_list; this is synchronized with target_splice_sess_cmd_list()
so that every command is either waited for or not added to the queue.
(nab: Keep target_submit_cmd() returning void for now..)
Signed-off-by: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
There are no in-tree users of target_get_sess_cmd() outside of
target_core_transport.c. Any new code should use the higher-level
target_submit_cmd() interface. So let's un-export target_get_sess_cmd()
and make it static to the one file where it's actually used.
(nab: Fix up minor fuzz to for-next)
Signed-off-by: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Code was almost entirely divided based on value of bool param "enable".
Split it into two functions.
Signed-off-by: Andy Grover <agrover@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Bubble-up retval from iscsi_update_param_value() and
iscsit_ta_authentication().
Other very small retval cleanups.
Signed-off-by: Andy Grover <agrover@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Only used in a debugprint, and function signature is cleaner now.
Signed-off-by: Andy Grover <agrover@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
The last functionality of the target processing thread is offloading possibly
long running task management requests from the submitter context. To keep
TMR semantics the same we need a single threaded ordered queue, which can
be provided by a per-device workqueue with the right flags.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Remove this command submission path which is not used by any in-tree driver.
This also removes the now unused new_cmd_map fabtric method, which a few
drivers implemented despite never calling transport_generic_handle_cdb_map.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
There is no need to schedule the delayed processing in a workqueue that
offloads it to the target processing thread. Instead execute it directly
from the workqueue. There will be a lot of future work in this area,
which I'd likfe to defer for now as it is not nessecary for getting rid
of the target processing thread.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Defer the write processing to the internal to be able to use
target_execute_cmd. I'm not even entirely sure the calling code requires
this due to the convoluted structure in libfc, but let's be safe for now.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Cc: Mark Rustad <mark.d.rustad@intel.com>
Cc: Kiran Patil <Kiran.patil@intel.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
All three callers of transport_generic_handle_data are from user context
and can use target_execute_cmd directly to handle the backend I/O submission
of WRITE I/O.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Cc: Andy Grover <agrover@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
When we call target_execute_cmd for write commands the command has been
on the state list before an abort might have come in before
target_execute_cmd. Call transport_check_aborted_status to deal with
this case.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Just call target_execute_cmd directly. Also, convert loopback, sbp,
usb-gadget to use the newly exported target_execute_cmd().
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Inline the transport_off == 0 case into target_execute_cmd to simplify
the function for the remaining cases.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Existing lio_dump.py code expects this to be in place for /iscsi.
Revert for now to avoid userspace breakage in lio-utils
This reverts commit fd88a785f9ac5d6be437c528571ccd85cdf2d493.
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Having all the unmap payload parsing in the backed is a bit ugly, but until
more drivers support it and we can find a good interface for all of them
that seems the way to go.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
