Re: [PATCH 0/5] qla2xxx: Bug fixes for the driver
Himanshu, > This patch series fixes issues with load/unload of the driver in a > loop. > > Please apply this series for 4.18/scsi-fixes at your earliest convenience > to be included in 4.18.0-rc6. Applied to 4.18/scsi-fixes, thank you! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH] scsi: csiostor: update ingress pack and pad boundary value
Varun, > T5/T6 can have different pack and pad boundary value. This patch sets > packing boundary based on cache line size and PCI-E maximum payload > size and sets smallest padding boundary value. Applied to 4.19/scsi-queue, thanks! -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 11/15] target: export initiator port values for all sessions
On 07/19/2018 03:47 PM, Christoph Hellwig wrote: > On Thu, Jul 19, 2018 at 05:07:59PM +, Bart Van Assche wrote: >> Initially configfs did not support creation of a directory from the kernel >> side. Last time I brought this up with Christoph he replied that this >> functionality has been added to configfs (if I understood Christoph >> correctly). > > I don't think the functionality was ever missing, but I might be > mistaken. You can always call config_group_init_type_name() + > configfs_add_default_group to add a directory. nvmet makes heavy > use of that. Just to clarify. We can create a dir from the kernel already. It is no problem. I am doing that in this patchset with configfs_register_group. What Bart was requesting originally and what is missing is being able to add a symlink from the kernel. I have not fully looked into it, but I think it would be something like taking part of configfs_symlink and making it so we can call it with config_items for the 2 items to be symlinked.
Re: [PATCH 15/15] iscsi target: merge iscsit_start_nopin_timer and __iscsit_start_nopin_timer
On Sun, Jul 15, 2018 at 06:16:31PM -0500, Mike Christie wrote: > Just have iscsit_start_nopin_timer grab the lock and > call __iscsit_start_nopin_timer. It doesn't really merge them but makes one call the other. Except for that this looks fine: Reviewed-by: Christoph Hellwig
Re: [PATCH 13/15] iscsi target: check nopin_response_timeout before starting timer
Looks good, Reviewed-by: Christoph Hellwig
Re: [PATCH 11/15] target: export initiator port values for all sessions
On Thu, Jul 19, 2018 at 05:07:59PM +, Bart Van Assche wrote: > Initially configfs did not support creation of a directory from the kernel > side. Last time I brought this up with Christoph he replied that this > functionality has been added to configfs (if I understood Christoph > correctly). I don't think the functionality was ever missing, but I might be mistaken. You can always call config_group_init_type_name() + configfs_add_default_group to add a directory. nvmet makes heavy use of that.
Re: [PATCH 08/15] target: add session removal function
On Sun, Jul 15, 2018 at 06:16:24PM -0500, Mike Christie wrote: > For tcm_fc, it should be ok to call > transport_deregister_session_configfs later like in the new > remove function because the transport_deregister_session_configfs call > was not affecting the target_sess_cmd_list_set_waiting/ > target_wait_for_sess_cmds calls and nothing else was being > torn down between that time. > > For drivers that only called transport_deregister_session > the new remove function that calls transport_deregister_session_configfs > should be ok, because those drivers did not access se_nacl->nacl_sess > or sess_acl_list so they will see no difference. It might be worth to split the conversion of the drivers that didn't exactly call transport_deregister_session_configfs + transport_deregister_session into a separate per-driver patch to better document this. Except for that this looks fine: Reviewed-by: Christoph Hellwig
Re: [PATCH 07/15] target: rename target_alloc_session
On Sun, Jul 15, 2018 at 06:16:23PM -0500, Mike Christie wrote: > Rename target_alloc_session to target_setup_session to avoid > confusion with the other transport session allocation > function that only allocates the session and because > the target_alloc_session does so more. It allocates the > session, sets up the nacl and egisters the session. > > The next patch will then add a remove function to match the > setup in this one, so it should make sense for all drivers, > except iscsi, to just call those 2 functions to setup and remove > a session. Ok. > iscsi will continue to be the odd driver. Wish we could sort that out eventually.. Otherwise looks fine: Reviewed-by: Christoph Hellwig
Re: [PATCH 06/15] target: make transport_init_session_tags static
On Sun, Jul 15, 2018 at 06:16:22PM -0500, Mike Christie wrote: > transport_init_session_tags is only called from target_core_transport.c > so make it static. > > Signed-off-by: Mike Christie Looks fine by itself: Reviewed-by: Christoph Hellwig Although folding it into the only caller would be even better.
Re: [PATCH 05/15] target: remove sess_get_index
On Sun, Jul 15, 2018 at 06:16:21PM -0500, Mike Christie wrote: > sess_get_index is meaninless for most drivers. For iscsi, it > is the same as the se_session->sid now and for fcoe it was just > the port id which would not work if multiple initiators > connected to the same target port. So just use the se_session sid > for all drivers and remove sess_get_index callout. Use up all your space again (not going to complain for any further patches). Modulo that and the type that Bart found: Reviewed-by: Christoph Hellwig
Re: [PATCH 03/15] target: fix __transport_register_session locking
On Sun, Jul 15, 2018 at 06:16:19PM -0500, Mike Christie wrote: > When __transport_register_session is called from > transport_register_session irqs will already have been disabled, > so we do not want the unlock irq call to enable them until > the higher level has done the final > spin_unlock_irqrestore/spin_unlock_irq. if you use up your line length this could be: so we do not want the unlock irq call to enable them until the higher level has done the final spin_unlock_irqrestore/spin_unlock_irq. > > This has __transport_register_session use the save/restore > call. Can be condensed as well. Otherwise looks good: Reviewed-by: Christoph Hellwig
Re: [PATCH 11/15] target: export initiator port values for all sessions
On Thu, 2018-07-19 at 11:38 -0500, Mike Christie wrote: > On 07/19/2018 10:37 AM, Bart Van Assche wrote: > > The general recommendation for configfs is that each attribute contains a > > single value, just like for sysfs. Patch 11/15 exports two values through > > a single attribute. Have you considered to split the above into two > > What about just making it the initiator port transport id so it aligns > with the get_initiator_port_transport_id() comment for the other patch. > For iscsi it would be 1 value with the format from SPC/SAM > "target_name,i,0x,isid"? That sounds fine to me. > > attributes, namely the initiator name and the ISID? Can the initiator name > > be changed into a soft link to the se_node_acl configfs directory to make > > it easy for shell scripts to retrieve additional initiator configuration > > information? > > Because the kernel is creating the session instead of it being driven > from a mkdir, there are no existing functions for this. I do not know > configfs code well, but I think making a function to do this is possible > though. Initially configfs did not support creation of a directory from the kernel side. Last time I brought this up with Christoph he replied that this functionality has been added to configfs (if I understood Christoph correctly). > What about the dynamic_acl case? Just make those a normal file? I'm not that familiar with dynamic ACLs. Is there a difference between "dynamic ACL" generation and "demo mode"? How does this interact with the generate_node_acls mode? > Just to make sure we are on the same page too. The initiator name is > always the name of the acl right? It looked like that from > target_fabric_make_nodeacl but I was wondering if you are asking for the > symlink because there are some fabric module quirks where that is not > the case. If it's the same names, then you know the acl already from the > initiator name file. It depends on what is called the "initiator name". E.g. the SRP target driver supports multiple formats to refer to a single initiator port. The first version of the ib_srpt driver supported only 128-bit GIDs as initiator name. Since the 64-bit prefix of a GID may change due to a reboot, later on support for 64-bit GUIDs was added. During login three formats for the initiator name are verified: GID, GUID without "0x" prefix and GUID with "0x" prefix. In any case, target_alloc_session() will store a pointer to the first struct se_node_acl that matches in sess->se_node_acl. I think using the name stored in struct se_node_acl is fine in all cases. Bart.
Re: [PATCH 11/15] target: export initiator port values for all sessions
On 07/19/2018 10:37 AM, Bart Van Assche wrote:
> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
>> +if (se_sess->se_tpg->se_tpg_tfo->sess_get_initiator_sid) {
>> +len = snprintf(page, PAGE_SIZE, "%s 0x%6phN\n",
>> + se_sess->se_node_acl->initiatorname,
>> + &se_sess->sess_bin_isid);
>> +} else {
>> +len = snprintf(page, PAGE_SIZE, "%s\n",
>> + se_sess->se_node_acl->initiatorname);
>> +}
>
> Hello Mike,
>
> The general recommendation for configfs is that each attribute contains a
> single value, just like for sysfs. Patch 11/15 exports two values through
> a single attribute. Have you considered to split the above into two
What about just making it the initiator port transport id so it aligns
with the get_initiator_port_transport_id() comment for the other patch.
For iscsi it would be 1 value with the format from SPC/SAM
"target_name,i,0x,isid"?
> attributes, namely the initiator name and the ISID? Can the initiator name
> be changed into a soft link to the se_node_acl configfs directory to make
> it easy for shell scripts to retrieve additional initiator configuration
> information?
Because the kernel is creating the session instead of it being driven
from a mkdir, there are no existing functions for this. I do not know
configfs code well, but I think making a function to do this is possible
though.
What about the dynamic_acl case? Just make those a normal file?
Just to make sure we are on the same page too. The initiator name is
always the name of the acl right? It looked like that from
target_fabric_make_nodeacl but I was wondering if you are asking for the
symlink because there are some fabric module quirks where that is not
the case. If it's the same names, then you know the acl already from the
initiator name file.
>
> Thanks,
>
> Bart.
>
Re: [PATCH 02/15] target: fix isid copying and comparision
On 07/19/2018 10:15 AM, Bart Van Assche wrote: > On Wed, 2018-07-18 at 20:02 -0500, Mike Christie wrote: >> On 07/18/2018 07:03 PM, Mike Christie wrote: >>> On 07/18/2018 05:09 PM, Bart Van Assche wrote: [ ... ] is that these involve a transport ID and that that transport ID can be up to 228 bytes long for iSCSI. >>> >>> I am talking about the Initiator Session ID above. That along with the >>> iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout >>> table 508 or in SAM 5r21 checkout table A.4. >>> >>> So in the SCSI specs as part of the Initiator Port Transport ID we have >>> this from that SAM table: >>> >>> The Initiator Session Identifier (ISID) portion of the string is a UTF-8 >>> encoded hexadecimal representation of a six byte binary value. >>> >>> --- >>> >>> In the PR parts of SPC it sometimes mentions only "Transport ID" but >>> then clarifies the initiator port so I am assuming in those cases it >>> means "Initiator Port Transport ID" so it is both the name and isid for >>> iscsi. >> >> It looks like we are supposed to go by what the initiator specifies in >> the TPID field, so it can be either the Transport ID or Initiator Port >> Transport ID. > > Hello Mike, > > Since the ISID is iSCSI-specific I think that all code that knows about the > ISID and its encoding should be in the iSCSI target driver instead of the > target core. Do you think an approach similar to that of the SCST function > iscsi_get_initiator_port_transport_id() can be implemented in LIO? The caller Yeah, I can make that work. > of that function is in source file scst/src/scst_targ.c: > > res = sess->tgt->tgtt->get_initiator_port_transport_id( > sess->tgt, sess, &sess->transport_id); > > Thanks, > > Bart.-- > To unsubscribe from this list: send the line "unsubscribe target-devel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Re: [PATCH] tcmu: fix crash for dereferencing the released udev->mb_addr memory
On 07/19/2018 09:30 AM, xiu...@redhat.com wrote:
> From: Xiubo Li
>
> The logs are:
>
> BUG: unable to handle kernel NULL pointer dereference at 0040
> IP: [] tcmu_reset_ring_store+0x149/0x240 [target_core_user]
> PGD 8e254067 PUD e255067 PMD 0
> Oops: 0002 [#1] SMP
> [...]
> CPU: 0 PID: 36077 Comm: tcmu-runner Kdump: loaded Not tainted
> 3.10.0-924.el7.test.x86_64 #1
It is not clear to me how you hit this. It's not a RHEL only bug is it?
Are you sure you are hitting it during device removal?
If we are calling tcmu_reset_ring_store isn't there a refcount on the
item? So we cannot call tcmu_destroy_device -> tcmu_dev_kref_release
until after that has been released, so they would never run at the same
time. And, if userspace were to try to open/write to that reset file
after the rmdir has been done on the se_device then configfs detects
this and fails the open.
I think we can hit your bug during initialization. We do not setup the
mb_addr until the device is configured, but the configfs files are
exported to userspace at device creation time. So between those times,
userspace could be writing to the reset file and hitting this bug. Is
that maybe what you hit?
If that is the bug, we need to grab the cmdr_lock in
tcmu_configure_device when we are setting up the mb_addr and in the
failure path in that function. In tcmu_reset_ring then I think we also
need a check for a NULL mb_addr.
> diff --git a/drivers/target/target_core_user.c
> b/drivers/target/target_core_user.c
> index 847707a..8d7274e 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -1587,16 +1587,16 @@ static void tcmu_dev_kref_release(struct kref *kref)
> bool all_expired = true;
> int i;
>
> - vfree(udev->mb_addr);
> - udev->mb_addr = NULL;
> -
> spin_lock_bh(&timed_out_udevs_lock);
> if (!list_empty(&udev->timedout_entry))
> list_del(&udev->timedout_entry);
> spin_unlock_bh(&timed_out_udevs_lock);
>
> - /* Upper layer should drain all requests before calling this */
> mutex_lock(&udev->cmdr_lock);
> + vfree(udev->mb_addr);
> + udev->mb_addr = NULL;
> +
> + /* Upper layer should drain all requests before calling this */
> idr_for_each_entry(&udev->commands, cmd, i) {
> if (tcmu_check_and_free_pending_cmd(cmd) != 0)
> all_expired = false;
>
Re: [PATCH 11/15] target: export initiator port values for all sessions
On Wed, 2018-07-18 at 21:15 -0500, Mike Christie wrote: > Oh wait, I think I know what you mean. I should have written "se_node_acl" instead of "se_portal_group". Bart.
Re: [PATCH 11/15] target: export initiator port values for all sessions
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
> + if (se_sess->se_tpg->se_tpg_tfo->sess_get_initiator_sid) {
> + len = snprintf(page, PAGE_SIZE, "%s 0x%6phN\n",
> +se_sess->se_node_acl->initiatorname,
> +&se_sess->sess_bin_isid);
> + } else {
> + len = snprintf(page, PAGE_SIZE, "%s\n",
> +se_sess->se_node_acl->initiatorname);
> + }
Hello Mike,
The general recommendation for configfs is that each attribute contains a
single value, just like for sysfs. Patch 11/15 exports two values through
a single attribute. Have you considered to split the above into two
attributes, namely the initiator name and the ISID? Can the initiator name
be changed into a soft link to the se_node_acl configfs directory to make
it easy for shell scripts to retrieve additional initiator configuration
information?
Thanks,
Bart.
Re: [PATCH 04/15] target/iscsi: move session_index to common se_session
On Wed, 2018-07-18 at 22:47 -0500, Mike Christie wrote: > Hey Bart, I looked into this some more and this value is also being used > as the scsiAttIntrPortIndex. For that use, does it need to be unique > across a target when the target has multiple ports? > > So I think it needs to be on the se_wwn right? Hello Mike, I think the best solution would be to decouple the session index that is used to export session information through configfs from the session index used by the SCSI-MIB (scsiAttIntrPortIndex). That approach would allow to make both indexes consecutive integers in all cases for both interfaces. However, neither configfs nor the SCSI-MIB requires that session indexes are consecutive integers. So I think moving se_sess_idr_lock and se_sess_idr into struct se_wwn is fine. Bart.
Re: [PATCH 02/15] target: fix isid copying and comparision
On Wed, 2018-07-18 at 20:02 -0500, Mike Christie wrote: > On 07/18/2018 07:03 PM, Mike Christie wrote: > > On 07/18/2018 05:09 PM, Bart Van Assche wrote: > > > [ ... ] > > > is that these involve a transport ID and that that transport ID can be up > > > to 228 > > > bytes long for iSCSI. > > > > I am talking about the Initiator Session ID above. That along with the > > iscsi name make up the Initiator Port Transport ID. In spc4r37 checkout > > table 508 or in SAM 5r21 checkout table A.4. > > > > So in the SCSI specs as part of the Initiator Port Transport ID we have > > this from that SAM table: > > > > The Initiator Session Identifier (ISID) portion of the string is a UTF-8 > > encoded hexadecimal representation of a six byte binary value. > > > > --- > > > > In the PR parts of SPC it sometimes mentions only "Transport ID" but > > then clarifies the initiator port so I am assuming in those cases it > > means "Initiator Port Transport ID" so it is both the name and isid for > > iscsi. > > It looks like we are supposed to go by what the initiator specifies in > the TPID field, so it can be either the Transport ID or Initiator Port > Transport ID. Hello Mike, Since the ISID is iSCSI-specific I think that all code that knows about the ISID and its encoding should be in the iSCSI target driver instead of the target core. Do you think an approach similar to that of the SCST function iscsi_get_initiator_port_transport_id() can be implemented in LIO? The caller of that function is in source file scst/src/scst_targ.c: res = sess->tgt->tgtt->get_initiator_port_transport_id( sess->tgt, sess, &sess->transport_id); Thanks, Bart.
[PATCH] tcmu: fix crash for dereferencing the released udev->mb_addr memory
From: Xiubo Li
The logs are:
BUG: unable to handle kernel NULL pointer dereference at 0040
IP: [] tcmu_reset_ring_store+0x149/0x240 [target_core_user]
PGD 8e254067 PUD e255067 PMD 0
Oops: 0002 [#1] SMP
[...]
CPU: 0 PID: 36077 Comm: tcmu-runner Kdump: loaded Not tainted
3.10.0-924.el7.test.x86_64 #1
Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference
Platform, BIOS 6.00 05/19/2017
task: 922db95ab0c0 ti: 922d9f8d4000 task.ti: 922d9f8d4000
RIP: 0010:[] []
tcmu_reset_ring_store+0x149/0x240 [target_core_user]
RSP: 0018:922d9f8d7e30 EFLAGS: 00010246
RAX: RBX: 1000 RCX: c100
RDX: 922d9f8d5fd8 RSI: RDI: 922d4b91f440
RBP: 922d9f8d7e70 R08: R09: 0001
R10: R11: R12: 922d4b91e550
R13: 922d4b91f3e8 R14: R15:
FS: 7f70467d7880() GS:922dbb60() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 0040 CR3: 0a2b CR4: 003607f0
DR0: DR1: DR2:
DR3: DR6: fffe0ff0 DR7: 0400
Call Trace:
[] configfs_write_file+0x11f/0x160
>From the crash tools:
crash> bt
PID: 36077 TASK: 922db95ab0c0 CPU: 0 COMMAND: "tcmu-runner"
#0 [922d9f8d7ac8] machine_kexec at abe62d8a
#1 [922d9f8d7b28] __crash_kexec at abf1bb02
#2 [922d9f8d7bf8] crash_kexec at abf1bbf0
#3 [922d9f8d7c10] oops_end at ac564798
#4 [922d9f8d7c38] no_context at ac552b3b
#5 [922d9f8d7c88] __bad_area_nosemaphore at ac552bd2
#6 [922d9f8d7cd8] bad_area_nosemaphore at ac552d43
#7 [922d9f8d7ce8] __do_page_fault at ac567750
#8 [922d9f8d7d50] do_page_fault at ac567945
#9 [922d9f8d7d80] page_fault at ac563788
[exception RIP: tcmu_reset_ring_store+329]
RIP: c072b9a9 RSP: 922d9f8d7e30 RFLAGS: 00010246
RAX: RBX: 1000 RCX: c100
RDX: 922d9f8d5fd8 RSI: RDI: 922d4b91f440
RBP: 922d9f8d7e70 R8: R9: 0001
R10: R11: R12: 922d4b91e550
R13: 922d4b91f3e8 R14: R15:
ORIG_RAX: CS: 0010 SS: 0018
RIP: 7f70458a074d RSP: 7ffcf788dc50 RFLAGS: 0293
RAX: 0001 RBX: RCX:
RDX: 0002 RSI: 7ffcf788dce0 RDI: 0007
RBP: 7ffcf788dcc0 R8: R9: 7f7044cd10fd
R10: 7ffcf788e720 R11: 0293 R12: 00407c80
R13: 7ffcf788f170 R14: R15:
ORIG_RAX: 0001 CS: 0033 SS: 002b
We can see that the IP is tcmu_reset_ring_store+329.
crash> dis tcmu_reset_ring_store
[...]
0xc072b955 : callq 0xac019810
0xc072b95a : jmpq 0xc072b8d8
0xc072b95f : nop
[...]
0xc072b98e : jne0xc072b988
0xc072b990 : testb $0x4,0x3efb(%rip)
# 0xc072f892
0xc072b997 : jne0xc072ba6e
0xc072b99d : movl $0x0,0xe74(%r12)
0xc072b9a9 : movl $0x0,0x40(%r14)
0xc072b9b1 : movl $0x0,0xc(%r14)
0xc072b9b9 : nopl 0x0(%rax)
0xc072b9c0 : sub$0x1000,%rbx
0xc072b9c7 : jne0xc072b9c0
0xc072b9c9 : lea0xf18(%r12),%rdi
0xc072b9d1 : callq 0xabea8b00
[...]
And the related target_core_user.c C code for line:
0xc072b9a9 : movl $0x0,0x40(%r14)
is "mb->cmd_tail = 0;"
Signed-off-by: Xiubo Li
---
drivers/target/target_core_user.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/target/target_core_user.c
b/drivers/target/target_core_user.c
index 847707a..8d7274e 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1587,16 +1587,16 @@ static void tcmu_dev_kref_release(struct kref *kref)
bool all_expired = true;
int i;
- vfree(udev->mb_addr);
- udev->mb_addr = NULL;
-
spin_lock_bh(&timed_out_udevs_lock);
if (!list_empty(&udev->timedout_entry))
list_del(&udev->timedout_entry);
spin_unlock_bh(&timed_out_udevs_lock);
- /* Upper layer should drain all requests before calling this */
mutex_lock(&udev->cmdr_lock);
+ vfree(udev->mb_addr);
+ udev->mb_addr = NULL;
+
+ /* Upper layer should drain all requests before calling this */
idr_for_each_entry(&udev->commands, cmd, i) {
if (tcmu_check_and_free_pending_cmd(cmd) != 0)
all_expired = false;
--
1.8.3.1
