On Wed, Oct 23, 2019 at 01:41:06PM -0700, Bart Van Assche wrote:
> Unlike the iSCSI target driver, for the SRP target driver it is sufficient
> if a single TPG can be associated with each RDMA port name. However, users
> started associating multiple TPGs with RDMA port names. Support this by
> converting the single TPG in struct srpt_port_id into a list. This patch
> fixes the following list corruption issue:
>
First of all, this patch do fix the list corruption issue.
I can't reproduce it anymore after apply this patch.
> list_add corruption. prev->next should be next (ffffffffc0a080c0), but was ffffa08a994ce6f0. (prev=ffffa08a994ce6f0).
> WARNING: CPU: 2 PID: 2597 at lib/list_debug.c:28 __list_add_valid+0x6a/0x70
> CPU: 2 PID: 2597 Comm: targetcli Not tainted 5.4.0-rc1.3bfa3c9602a7 #1
> RIP: 0010:__list_add_valid+0x6a/0x70
> Call Trace:
> core_tpg_register+0x116/0x200 [target_core_mod]
> srpt_make_tpg+0x3f/0x60 [ib_srpt]
> target_fabric_make_tpg+0x41/0x290 [target_core_mod]
> configfs_mkdir+0x158/0x3e0
> vfs_mkdir+0x108/0x1a0
> do_mkdirat+0x77/0xe0
> do_syscall_64+0x55/0x1d0
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> +
> + mutex_lock(&sport->port_gid_id.mutex);
> + list_for_each_entry(stpg, &sport->port_gid_id.tpg_list, entry) {
> + if (!IS_ERR_OR_NULL(ch->sess))
^^^^^^^
> + break;
> + ch->sess = target_setup_session(&stpg->tpg, tag_num,
^^^^^^^^^^^^^^^^^^^^
> tag_size, TARGET_PROT_NORMAL, i_port_id,
> ch, NULL);
> - /* Retry without leading "0x" */
> - if (sport->port_gid_id.tpg.se_tpg_wwn && IS_ERR_OR_NULL(ch->sess))
> - ch->sess = target_setup_session(&sport->port_gid_id.tpg, tag_num,
> + if (!IS_ERR_OR_NULL(ch->sess))
^
> + break;
I'm confused about this 'if' statement. In case you repeated the
validation as previous 'if' statement, it is redundance.
In case you check the return of the first target_setup_session,
it seems wrong, we only need to retry in case first target_setup_session
was failed. But you break out, and skip the second target_setup_session.
> + /* Retry without leading "0x" */
> + ch->sess = target_setup_session(&stpg->tpg, tag_num,
^^^^^^^^^^^^^^^^^^^^
> tag_size, TARGET_PROT_NORMAL,
> i_port_id + 2, ch, NULL);
thanks
