Re: [PATCH v3] scsi: tcmu: avoid cmd/qfull timers updated whenever a new cmd comes

[Mike Christie]

On 11/20/2018 11:37 PM, Xiubo Li wrote:
> [...]
>>> -is_running = list_empty(>cmdr_queue_entry);
>>> +is_running = test_bit(TCMU_CMD_BIT_INFLIGHT, >flags);
>>>   se_cmd = cmd->se_cmd;
>>> if (is_running) {
>>> @@ -1289,7 +1319,6 @@ static int tcmu_check_expired_cmd(int id, void
>>> *p, void *data)
>>>   scsi_status = SAM_STAT_CHECK_CONDITION;
>>>   } else {
>>>   list_del_init(>cmdr_queue_entry);
>> Move this list_del_init call to outside the if/else.
>>
>> You need do delete it from the cmdr_inflight_queue if that is how it
>> timed out, or if you later call tcmu_get_next_deadline it will still
>> show up and possibly be used to set the next time out which already
>> happened.
> 
> Firstly, this is in the timeout routine, if this cmd was already timed
> out and it must be time_after(jiffies, cmd->deadline), so it won't be
> used again.

It could be stuck a long time. What about jiffies rollover?

Re: [PATCH v5] target: add emulate_pr backstore attr to toggle PR support

[Mike Christie]

On 11/07/2018 07:11 AM, David Disseldorp wrote:
> The new emulate_pr backstore attribute allows for Persistent Reservation
> and SCSI2 RESERVE/RELEASE support to be completely disabled. This can be
> useful for scenarios such as:
> - Ensuring ATS (Compare & Write) usage on recent VMware ESXi initiators.
> - Allowing clustered (e.g. tcm-user) backends to block such requests,
>   avoiding the multi-node reservation state propagation.
> 
> When explicitly disabled, PR and RESERVE/RELEASE requests receive
> Invalid Command Operation Code response sense data.
> 

Reviewed-by: Mike Christie 

Re: [PATCH v3] scsi: tcmu: avoid cmd/qfull timers updated whenever a new cmd comes

[Mike Christie]

On 10/17/2018 02:54 AM, xiu...@redhat.com wrote:
> From: Xiubo Li 
> 
> Currently there has one cmd timeout timer and one qfull timer for
> each udev, and whenever there has any new coming cmd it will update
> the cmd timer or qfull timer. And for some corner case the timers
> are always working only for the ringbuffer's and full queue's newest
> cmd. That's to say the timer won't be fired even there has one cmd
> stuck for a very long time and the deadline is reached.
> 
> This fix will keep the cmd/qfull timers to be pended for the oldest
> cmd in ringbuffer and full queue, and will update them with the next
> cmd's deadline only when the old cmd's deadline is reached or removed
> from the ringbuffer and full queue.
> 
> Signed-off-by: Xiubo Li 
> ---
> 
> Changed in V3:
> - Add inflight queue support, thanks Mike.
> 
> Changed in V2:
> - Add qfull timer
> - Addressed all the comments from Mike, thanks.
> 
>  drivers/target/target_core_user.c | 89 
> ++-
>  1 file changed, 70 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/target/target_core_user.c 
> b/drivers/target/target_core_user.c
> index 9cd404a..00ed7bb 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -148,7 +148,7 @@ struct tcmu_dev {
>   size_t ring_size;
>  
>   struct mutex cmdr_lock;
> - struct list_head cmdr_queue;
> + struct list_head cmdr_qfull_queue;

We can just rename to qfull_queue and inflight_queue and then rename the
function names you modified below.

>  
>   uint32_t dbi_max;
>   uint32_t dbi_thresh;
> @@ -159,6 +159,7 @@ struct tcmu_dev {
>  
>   struct timer_list cmd_timer;
>   unsigned int cmd_time_out;
> + struct list_head cmdr_inflight_queue;
>  
>   struct timer_list qfull_timer;
>   int qfull_time_out;
> @@ -192,6 +193,7 @@ struct tcmu_cmd {
>   unsigned long deadline;
>  
>  #define TCMU_CMD_BIT_EXPIRED 0
> +#define TCMU_CMD_BIT_INFLIGHT 1
>   unsigned long flags;
>  };
>  /*
> @@ -915,11 +917,13 @@ static int tcmu_setup_cmd_timer(struct tcmu_cmd 
> *tcmu_cmd, unsigned int tmo,
>   return 0;
>  
>   tcmu_cmd->deadline = round_jiffies_up(jiffies + msecs_to_jiffies(tmo));
> - mod_timer(timer, tcmu_cmd->deadline);
> + if (!timer_pending(timer))
> + mod_timer(timer, tcmu_cmd->deadline);
> +
>   return 0;
>  }
>  
> -static int add_to_cmdr_queue(struct tcmu_cmd *tcmu_cmd)
> +static int add_to_cmdr_qfull_queue(struct tcmu_cmd *tcmu_cmd)
>  {
>   struct tcmu_dev *udev = tcmu_cmd->tcmu_dev;
>   unsigned int tmo;
> @@ -942,7 +946,7 @@ static int add_to_cmdr_queue(struct tcmu_cmd *tcmu_cmd)
>   if (ret)
>   return ret;
>  
> - list_add_tail(_cmd->cmdr_queue_entry, >cmdr_queue);
> + list_add_tail(_cmd->cmdr_queue_entry, >cmdr_qfull_queue);
>   pr_debug("adding cmd %u on dev %s to ring space wait queue\n",
>tcmu_cmd->cmd_id, udev->name);
>   return 0;
> @@ -999,7 +1003,7 @@ static sense_reason_t queue_cmd_ring(struct tcmu_cmd 
> *tcmu_cmd, int *scsi_err)
>   base_command_size = tcmu_cmd_get_base_cmd_size(tcmu_cmd->dbi_cnt);
>   command_size = tcmu_cmd_get_cmd_size(tcmu_cmd, base_command_size);
>  
> - if (!list_empty(>cmdr_queue))
> + if (!list_empty(>cmdr_qfull_queue))
>   goto queue;
>  
>   mb = udev->mb_addr;
> @@ -1096,13 +1100,16 @@ static sense_reason_t queue_cmd_ring(struct tcmu_cmd 
> *tcmu_cmd, int *scsi_err)
>   UPDATE_HEAD(mb->cmd_head, command_size, udev->cmdr_size);
>   tcmu_flush_dcache_range(mb, sizeof(*mb));
>  
> + list_add_tail(_cmd->cmdr_queue_entry, >cmdr_inflight_queue);
> + set_bit(TCMU_CMD_BIT_INFLIGHT, _cmd->flags);
> +
>   /* TODO: only if FLUSH and FUA? */
>   uio_event_notify(>uio_info);
>  
>   return 0;
>  
>  queue:
> - if (add_to_cmdr_queue(tcmu_cmd)) {
> + if (add_to_cmdr_qfull_queue(tcmu_cmd)) {
>   *scsi_err = TCM_OUT_OF_RESOURCES;
>   return -1;
>   }
> @@ -1194,9 +1201,22 @@ static void tcmu_handle_completion(struct tcmu_cmd 
> *cmd, struct tcmu_cmd_entry *
>   tcmu_free_cmd(cmd);
>  }
>  
> +static unsigned long tcmu_get_next_deadline(struct list_head *queue)
> +{
> + struct tcmu_cmd *tcmu_cmd, *tmp_cmd;
> +
> + list_for_each_entry_safe(tcmu_cmd, tmp_cmd, queue, cmdr_queue_entry) {
> + if (!time_after(jiffies, tcmu_cmd->deadline))
> + return tcmu_cmd->deadline;
> + }
> +
> + return 0;

I think you can just pass the timer to this function, rename it to
tcmu_set_next_deadline() and move the mod/del_timer calls in here too
since it is always the same behavior.


> +}
> +
>  static unsigned int tcmu_handle_completions(struct tcmu_dev *udev)
>  {
>   struct tcmu_mailbox *mb;
> + struct tcmu_cmd *cmd;
>   int handled = 0;
>  
>   if (test_bit(TCMU_DEV_BIT_BROKEN, >flags)) {
> @@ -1210,7 +1230,6 @@ static 

Re: [PATCH v4] target: add emulate_pr backstore attr to toggle PR support

[Mike Christie]

On 10/30/2018 09:26 AM, David Disseldorp wrote:
> The new emulate_pr backstore attribute allows for Persistent Reservation
> and SCSI2 RESERVE/RELEASE support to be completely disabled. This can be
> useful for scenarios such as:
> - Ensuring ATS (Compare & Write) usage on recent VMware ESXi initiators.
> - Allowing clustered (e.g. tcm-user) backends to block such requests,
>   avoiding the need for multi-node reservation state propagation.
> 
> When explicitly disabled, PR and RESERVE/RELEASE requests receive
> Invalid Command Operation Code response sense data.
> 
> Signed-off-by: David Disseldorp 
> ---
>  drivers/target/target_core_configfs.c | 32 
>  drivers/target/target_core_device.c   | 13 +
>  drivers/target/target_core_pr.c   |  2 ++
>  drivers/target/target_core_spc.c  |  8 
>  include/target/target_core_base.h |  3 +++
>  5 files changed, 50 insertions(+), 8 deletions(-)
> 
> Changes since v3:
> * rebase against current mainline
> 
> Changes since v2:
> * handle target_pr_res_aptpl_metadata_store()
> * use common error path for spc_parse_cdb() and passthrough_parse_cdb()
>   checks
> * drop erroneous TRANSPORT_FLAG_PASSTHROUGH_PGR ->
>   TRANSPORT_FLAG_PASSTHROUGH changes
> 
> Changes since v1:
> * block Reservation request passthrough when emulate_pr=0
> * fix some style issues
> * add an emulate_pr check to pgr_support_show()
> 
> diff --git a/drivers/target/target_core_configfs.c 
> b/drivers/target/target_core_configfs.c
> index f6b1549f4142..bacb771a333e 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -532,6 +532,7 @@ DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpu);
>  DEF_CONFIGFS_ATTRIB_SHOW(emulate_tpws);
>  DEF_CONFIGFS_ATTRIB_SHOW(emulate_caw);
>  DEF_CONFIGFS_ATTRIB_SHOW(emulate_3pc);
> +DEF_CONFIGFS_ATTRIB_SHOW(emulate_pr);
>  DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_type);
>  DEF_CONFIGFS_ATTRIB_SHOW(hw_pi_prot_type);
>  DEF_CONFIGFS_ATTRIB_SHOW(pi_prot_format);
> @@ -592,6 +593,7 @@ static ssize_t _name##_store(struct config_item *item, 
> const char *page,  \
>  DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_fua_write);
>  DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_caw);
>  DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_3pc);
> +DEF_CONFIGFS_ATTRIB_STORE_BOOL(emulate_pr);
>  DEF_CONFIGFS_ATTRIB_STORE_BOOL(enforce_pr_isids);
>  DEF_CONFIGFS_ATTRIB_STORE_BOOL(is_nonrot);
>  
> @@ -1100,9 +1102,13 @@ static ssize_t pgr_support_show(struct config_item 
> *item, char *page)
>  {
>   struct se_dev_attrib *da = to_attrib(item);
>   u8 flags = da->da_dev->transport->transport_flags;
> + int pgr_support = 1;
>  
> - return snprintf(page, PAGE_SIZE, "%d\n",
> - flags & TRANSPORT_FLAG_PASSTHROUGH_PGR ? 0 : 1);
> + if (!da->da_dev->dev_attrib.emulate_pr ||
> + (flags & TRANSPORT_FLAG_PASSTHROUGH_PGR))
> + pgr_support = 0;
> +

I think we want to keep this separate still. The file tells userspace if
PRs are supported in the backend module/device or in LIO core.

With the chunk above, if you had emulate_pr=0 and
TRANSPORT_FLAG_PASSTHROUGH_PGR is set, userspace cannot detect what the
backend supports. We would have to temporarily set emaulate_pr sow e can
read the file then clear it.

Re: [RFC PATCH 0/3] target: remove some unused stats

[Mike Christie]

On 10/29/2018 06:00 PM, David Disseldorp wrote:
> On Wed, 17 Oct 2018 17:48:17 +0200, David Disseldorp wrote:
> 
>> This patchset removes a couple of unused error stat counters and a
>> redundant cumulative counter.
>> I've tagged this patchset RFC, as it may be considered a kernel<->user
>> (configfs) API change.
> 
> Ping, any thoughts on this patchset?
> 

I think these stats were supposed to match the iSCSI MIB definitions. It
was not clear why we didn't just fix them so they report the correct values?

Re: [PATCH] scsi: ibmvscsi_tgt: Remove target_wait_for_sess_cmd()

[Mike Christie]

On 10/16/2018 12:34 PM, Ly, Bryant wrote:
> From: "Bryant G. Ly" 
> 
> There is currently a bug with the driver where there is never a
> call to target_sess_cmd_list_set_waiting(), it only called
> target_wait_for_sess_cmd(), which basically means that the
> sess_wait_list would always be empty.
> 
> Thus, list_empty(>sess_wait_list) = true,
> (eg: no se_cmd I/O is quiesced, because no se_cmd in sess_wait_list),
> since commit 712db3eb2c35 ("scsi: ibmvscsis: Properly deregister
> target sessions") in 4.9.y code.
> 
> ibmvscsi_tgt does not remove the I_T Nexus when a VM is
> active so we can fix this issue by removing the call to
> target_wait_for_sess_cmd() altogether.
> 
> Signed-off-by: Bryant G. Ly 
> ---
>  drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
> b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index fac3773..2175e9e 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -2266,7 +2266,6 @@ static int ibmvscsis_drop_nexus(struct ibmvscsis_tport 
> *tport)
>   /*
>* Release the SCSI I_T Nexus to the emulated ibmvscsis Target Port
>*/
> - target_wait_for_sess_cmds(se_sess);
>   target_remove_session(se_sess);
>   tport->ibmv_nexus = NULL;
>   kfree(nexus);
> 

Reviewed-by: Mike Christie 

Re: [PATCH] scsi: tcmu: setup one timeout timer for each cmd

[Mike Christie]

On 09/18/2018 05:32 AM, xiu...@redhat.com wrote:
> From: Xiubo Li 
> 
> Currently there has one cmd timeout timer for each udev, and whenever
> there has any new coming cmd it will update the timer. And for some
> corner case the timer is always working only for the ringbuffer's
> newest cmd. That's to say the timer won't be fired even there has one
> cmd stuck for a very long time.
> 
> Signed-off-by: Xiubo Li 
> ---
>  drivers/target/target_core_user.c | 69 +++
>  1 file changed, 34 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/target/target_core_user.c 
> b/drivers/target/target_core_user.c
> index 9cd404acdb82..ed3d6f055037 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -157,7 +157,6 @@ struct tcmu_dev {
>  
>   struct idr commands;
>  
> - struct timer_list cmd_timer;
>   unsigned int cmd_time_out;
>  
>   struct timer_list qfull_timer;
> @@ -190,6 +189,7 @@ struct tcmu_cmd {
>   uint32_t *dbi;
>  
>   unsigned long deadline;
> + struct timer_list cmd_timer;
>  

I think you can fix this for qfull at the same time and we will not have
to do a mix of per cmd timer along with per device handling.

In tcmu_check_expired_cmd/check_timedout_devices add a next_deadline
variable and when we loop over all the cmds with the idr_for_each have
tcmu_check_expired_cmd return the cmd->deadline. check_timedout_devices
will then track the next timeout value to use and do a mod_timer to set it.

In tcmu_setup_cmd_timer you would only mod_timer if the timer is not
already pending already, so we do not keep resetting it like we do today.

Re: [PATCH v5] target/iblock: split T10 PI SGL across command bios

[Mike Christie]

On 09/04/2018 12:19 PM, Greg Edwards wrote:
> When T10 PI is enabled on a backing device for the iblock backstore, the
> PI SGL for the entire command is attached to the first bio only.  This
> works fine if the command is covered by a single bio, but can result in
> ref tag errors in the client for the other bios in a multi-bio command,
> e.g.
> 
> [   47.631236] sda: ref tag error at location 2048 (rcvd 0)
> [   47.637658] sda: ref tag error at location 4096 (rcvd 0)
> [   47.644228] sda: ref tag error at location 6144 (rcvd 0)
> 
> The command will be split into multiple bios if the number of data SG
> elements exceeds BIO_MAX_PAGES (see iblock_get_bio()).
> 
> The bios may later be split again in the block layer on the host after
> iblock_submit_bios(), depending on the queue limits of the backing
> device.  The block and SCSI layers will pass through the whole PI SGL
> down to the LLDD however that first bio is split up, but the LLDD may
> only use the portion that corresponds to the data length (depends on the
> LLDD, tested with scsi_debug).
> 
> Split the PI SGL across the bios in the command, so each bio's
> bio_integrity_payload contains the protection information for the data
> in the bio.  Use an sg_mapping_iter to keep track of where we are in PI
> SGL, so we know where to start with the next bio.
> 
> Signed-off-by: Greg Edwards 
> ---
> Changes from v4:
>   * use %zu for size_t in pr_debug()
> 
> Changes from v3:
>   * cast a size_t as unsigned long in a pr_debug() for 32-bit arches,
> turned up by the kbuild test robot
> 
> Changes from v2:
>   * add back min(cmd->t_prot_nents, BIO_MAX_PAGES) for bio_integrity_alloc()
> from v1
> 
> Changes from v1:
>   * expand commit message
>   * use an sg_mapping_iter to track where we are in the PI SGL
> 


Looks ok to me.

Reviewed-by: Mike Christie 

Re: [PATCH v4] target/iblock: split T10 PI SGL across command bios

[Mike Christie]

On 09/04/2018 11:21 AM, Greg Edwards wrote:
>  static int
> -iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio)
> +iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio,
> +  struct sg_mapping_iter *miter)
>  {
>   struct se_device *dev = cmd->se_dev;
>   struct blk_integrity *bi;
>   struct bio_integrity_payload *bip;
>   struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
> - struct scatterlist *sg;
> - int i, rc;
> + int rc;
> + size_t resid, len;


>  
> - pr_debug("Added bio integrity page: %p length: %d offset; %d\n",
> -  sg_page(sg), sg->length, sg->offset);
> + pr_debug("Added bio integrity page: %p length: %lu offset: 
> %lu\n",
> +   miter->page, (unsigned long)len,
> +   offset_in_page(miter->addr));

I think we have %z for a size_t.

Re: [PATCH v3] target/iblock: split T10 PI SGL across command bios

[Mike Christie]

r->page, len, offset_in_page(miter->addr));
> +
> + resid -= len;
> + if (len < miter->length)
> + miter->consumed -= miter->length - len;
>   }
> + sg_miter_stop(miter);
>  
>   return 0;
>  }
> @@ -686,12 +695,13 @@ iblock_execute_rw(struct se_cmd *cmd, struct 
> scatterlist *sgl, u32 sgl_nents,
>   struct se_device *dev = cmd->se_dev;
>   sector_t block_lba = target_to_linux_sector(dev, cmd->t_task_lba);
>   struct iblock_req *ibr;
> - struct bio *bio, *bio_start;
> + struct bio *bio;
>   struct bio_list list;
>   struct scatterlist *sg;
>   u32 sg_num = sgl_nents;
>   unsigned bio_cnt;
> - int i, op, op_flags = 0;
> + int i, rc, op, op_flags = 0;
> + struct sg_mapping_iter prot_miter;
>  
>   if (data_direction == DMA_TO_DEVICE) {
>   struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
> @@ -726,13 +736,17 @@ iblock_execute_rw(struct se_cmd *cmd, struct 
> scatterlist *sgl, u32 sgl_nents,
>   if (!bio)
>   goto fail_free_ibr;
>  
> - bio_start = bio;
>   bio_list_init();
>   bio_list_add(, bio);
>  
>   refcount_set(>pending, 2);
>   bio_cnt = 1;
>  
> + if (cmd->prot_type && dev->dev_attrib.pi_prot_type)
> + sg_miter_start(_miter, cmd->t_prot_sg, cmd->t_prot_nents,
> +op == REQ_OP_READ ? SG_MITER_FROM_SG :
> +SG_MITER_TO_SG);
> +
>   for_each_sg(sgl, sg, sgl_nents, i) {
>   /*
>* XXX: if the length the device accepts is shorter than the
> @@ -741,6 +755,12 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist 
> *sgl, u32 sgl_nents,
>*/
>   while (bio_add_page(bio, sg_page(sg), sg->length, sg->offset)
>   != sg->length) {
> + if (cmd->prot_type && dev->dev_attrib.pi_prot_type) {
> + rc = iblock_alloc_bip(cmd, bio, _miter);
> +     if (rc)
> + goto fail_put_bios;
> + }
> +
>   if (bio_cnt >= IBLOCK_MAX_BIO_PER_TASK) {
>   iblock_submit_bios();
>   bio_cnt = 0;
> @@ -762,7 +782,7 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist 
> *sgl, u32 sgl_nents,
>   }
>  
>   if (cmd->prot_type && dev->dev_attrib.pi_prot_type) {
> - int rc = iblock_alloc_bip(cmd, bio_start);
> + rc = iblock_alloc_bip(cmd, bio, _miter);
>   if (rc)
>   goto fail_put_bios;
>   }
> 

Seems ok to me.

Reviewed-by: Mike Christie 

Re: [PATCH v2] target/iblock: split T10 PI SGL across command bios

[Mike Christie]

On 08/29/2018 03:36 PM, Greg Edwards wrote:
> When T10 PI is enabled on a backing device for the iblock backstore, the
> PI SGL for the entire command is attached to the first bio only.  This
> works fine if the command is covered by a single bio, but can result in
> ref tag errors in the client for the other bios in a multi-bio command,
> e.g.
> 
> [   47.631236] sda: ref tag error at location 2048 (rcvd 0)
> [   47.637658] sda: ref tag error at location 4096 (rcvd 0)
> [   47.644228] sda: ref tag error at location 6144 (rcvd 0)
> 
> The command will be split into multiple bios if the number of data SG
> elements exceeds BIO_MAX_PAGES (see iblock_get_bio()).
> 
> The bios may later be split again in the block layer on the host after
> iblock_submit_bios(), depending on the queue limits of the backing
> device.  The block and SCSI layers will pass through the whole PI SGL
> down to the LLDD however that first bio is split up, but the LLDD may
> only use the portion that corresponds to the data length (depends on the
> LLDD, tested with scsi_debug).
> 
> Split the PI SGL across the bios in the command, so each bio's
> bio_integrity_payload contains the protection information for the data
> in the bio.  Use an sg_mapping_iter to keep track of where we are in PI
> SGL, so we know where to start with the next bio.
> 
> Signed-off-by: Greg Edwards 
> ---
> Changes v1 -> v2:
>   * expand commit message
>   * use an sg_mapping_iter to track where we are in the PI SGL
> 
> 
>  drivers/target/target_core_iblock.c | 51 -
>  1 file changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/target/target_core_iblock.c 
> b/drivers/target/target_core_iblock.c
> index ce1321a5cb7b..9f9c3c934ce1 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -635,14 +635,15 @@ static ssize_t iblock_show_configfs_dev_params(struct 
> se_device *dev, char *b)
>  }
>  
>  static int
> -iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio)
> +iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio,
> +  struct sg_mapping_iter *miter)
>  {
>   struct se_device *dev = cmd->se_dev;
>   struct blk_integrity *bi;
>   struct bio_integrity_payload *bip;
>   struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
> - struct scatterlist *sg;
> - int i, rc;
> + int rc;
> + size_t resid, len;
>  
>   bi = bdev_get_integrity(ib_dev->ibd_bd);
>   if (!bi) {
> @@ -656,25 +657,32 @@ iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio)
>   return PTR_ERR(bip);
>   }

Can you still hit the issue where t_prot_nents > BIO_MAX_PAGES so
bio_integrity_alloc fails or is t_prot_nents always going to be smaller.
Was wondering why you dropped that from the last patch.

[PATCH 1/2] iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails

[Mike Christie]

From: Vincent Pelletier 

Fixes a use-after-free reported by KASAN when later
iscsi_target_login_sess_out gets called and it tries to access
conn->sess->se_sess:

Disabling lock debugging due to kernel taint
iSCSI Login timeout on Network Portal [::]:3260
iSCSI Login negotiation failed.
==
BUG: KASAN: use-after-free in
iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
Read of size 8 at addr 880109d070c8 by task iscsi_np/980

CPU: 1 PID: 980 Comm: iscsi_np Tainted: G   O
4.17.8kasan.sess.connops+ #4
Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB,
BIOS 5.6.5 05/19/2014
Call Trace:
 dump_stack+0x71/0xac
 print_address_description+0x65/0x22e
 ? iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
 kasan_report.cold.6+0x241/0x2fd
 iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
 iscsi_target_login_thread+0x1086/0x1710 [iscsi_target_mod]
 ? __sched_text_start+0x8/0x8
 ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
 ? __kthread_parkme+0xcc/0x100
 ? parse_args.cold.14+0xd3/0xd3
 ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
 kthread+0x1a0/0x1c0
 ? kthread_bind+0x30/0x30
 ret_from_fork+0x35/0x40

Allocated by task 980:
 kasan_kmalloc+0xbf/0xe0
 kmem_cache_alloc_trace+0x112/0x210
 iscsi_target_login_thread+0x816/0x1710 [iscsi_target_mod]
 kthread+0x1a0/0x1c0
 ret_from_fork+0x35/0x40

Freed by task 980:
 __kasan_slab_free+0x125/0x170
 kfree+0x90/0x1d0
 iscsi_target_login_thread+0x1577/0x1710 [iscsi_target_mod]
 kthread+0x1a0/0x1c0
 ret_from_fork+0x35/0x40

The buggy address belongs to the object at 880109d06f00
 which belongs to the cache kmalloc-512 of size 512
The buggy address is located 456 bytes inside of
 512-byte region [880109d06f00, 880109d07100)
The buggy address belongs to the page:
page:ea0004274180 count:1 mapcount:0 mapping:
index:0x0 compound_mapcount: 0
flags: 0x17fffc08100(slab|head)
raw: 017fffc08100   0001000c000c
raw: dead0100 dead0200 88011b002e00 
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 880109d06f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 880109d07000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>880109d07080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
  ^
 880109d07100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 880109d07180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==

Signed-off-by: Vincent Pelletier 
[rebased against idr/ida changes and to handle ret review comments from Matthew]
Signed-off-by: Mike Christie 
Cc: Matthew Wilcox 

---

v2:
- Rebase against Linus's tree with idr/ida changes.
- Always return -ENOMEM on failure because caller does not care and
to simplify the code.

 drivers/target/iscsi/iscsi_target_login.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_login.c 
b/drivers/target/iscsi/iscsi_target_login.c
index 9e74f8b..f58b9c1 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -310,11 +310,9 @@ static int iscsi_login_zero_tsih_s1(
return -ENOMEM;
}
 
-   ret = iscsi_login_set_conn_values(sess, conn, pdu->cid);
-   if (unlikely(ret)) {
-   kfree(sess);
-   return ret;
-   }
+   if (iscsi_login_set_conn_values(sess, conn, pdu->cid))
+   goto free_sess;
+
sess->init_task_tag = pdu->itt;
memcpy(>isid, pdu->isid, 6);
sess->exp_cmd_sn= be32_to_cpu(pdu->cmdsn);
-- 
2.7.2

[PATCH 0/2] iscsi target: login fixes

[Mike Christie]

The following patches are login fixes from the list. The set has been rebased
against Linus's current tree so they apply over the idr/ida patches that have
been merged.

[PATCH 2/2] iscsi target: fix conn_ops double free

[Mike Christie]

If iscsi_login_init_conn fails it can free conn_ops.
__iscsi_target_login_thread will then call iscsi_target_login_sess_out
which will also free it.

This fixes the problem by organizing conn allocation/setup into parts
that are needed through the life of the conn and parts that are only
needed for the login. The free functions then release what was allocated
in the alloc functions.

With this patch we have:

iscsit_alloc_conn/iscsit_free_conn - allocs/frees the conn we need
for the entire life of the conn.

iscsi_login_init_conn/iscsi_target_nego_release - allocs/frees the parts
of the conn that are only needed during login.

Signed-off-by: Mike Christie 
---
 drivers/target/iscsi/iscsi_target.c   |   9 +-
 drivers/target/iscsi/iscsi_target_login.c | 141 --
 drivers/target/iscsi/iscsi_target_login.h |   2 +-
 3 files changed, 77 insertions(+), 75 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index 94bad43..9cdfccb 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4208,22 +4208,15 @@ int iscsit_close_connection(
crypto_free_ahash(tfm);
}
 
-   free_cpumask_var(conn->conn_cpumask);
-
-   kfree(conn->conn_ops);
-   conn->conn_ops = NULL;
-
if (conn->sock)
sock_release(conn->sock);
 
if (conn->conn_transport->iscsit_free_conn)
conn->conn_transport->iscsit_free_conn(conn);
 
-   iscsit_put_transport(conn->conn_transport);
-
pr_debug("Moving to TARG_CONN_STATE_FREE.\n");
conn->conn_state = TARG_CONN_STATE_FREE;
-   kfree(conn);
+   iscsit_free_conn(conn);
 
spin_lock_bh(>conn_lock);
atomic_dec(>nconn);
diff --git a/drivers/target/iscsi/iscsi_target_login.c 
b/drivers/target/iscsi/iscsi_target_login.c
index f58b9c1..bb90c80 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -67,45 +67,10 @@ static struct iscsi_login *iscsi_login_init_conn(struct 
iscsi_conn *conn)
goto out_req_buf;
}
 
-   conn->conn_ops = kzalloc(sizeof(struct iscsi_conn_ops), GFP_KERNEL);
-   if (!conn->conn_ops) {
-   pr_err("Unable to allocate memory for"
-   " struct iscsi_conn_ops.\n");
-   goto out_rsp_buf;
-   }
-
-   init_waitqueue_head(>queues_wq);
-   INIT_LIST_HEAD(>conn_list);
-   INIT_LIST_HEAD(>conn_cmd_list);
-   INIT_LIST_HEAD(>immed_queue_list);
-   INIT_LIST_HEAD(>response_queue_list);
-   init_completion(>conn_post_wait_comp);
-   init_completion(>conn_wait_comp);
-   init_completion(>conn_wait_rcfr_comp);
-   init_completion(>conn_waiting_on_uc_comp);
-   init_completion(>conn_logout_comp);
-   init_completion(>rx_half_close_comp);
-   init_completion(>tx_half_close_comp);
-   init_completion(>rx_login_comp);
-   spin_lock_init(>cmd_lock);
-   spin_lock_init(>conn_usage_lock);
-   spin_lock_init(>immed_queue_lock);
-   spin_lock_init(>nopin_timer_lock);
-   spin_lock_init(>response_queue_lock);
-   spin_lock_init(>state_lock);
-
-   if (!zalloc_cpumask_var(>conn_cpumask, GFP_KERNEL)) {
-   pr_err("Unable to allocate conn->conn_cpumask\n");
-   goto out_conn_ops;
-   }
conn->conn_login = login;
 
return login;
 
-out_conn_ops:
-   kfree(conn->conn_ops);
-out_rsp_buf:
-   kfree(login->rsp_buf);
 out_req_buf:
kfree(login->req_buf);
 out_login:
@@ -1147,6 +1112,75 @@ iscsit_conn_set_transport(struct iscsi_conn *conn, 
struct iscsit_transport *t)
return 0;
 }
 
+static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
+{
+   struct iscsi_conn *conn;
+
+   conn = kzalloc(sizeof(struct iscsi_conn), GFP_KERNEL);
+   if (!conn) {
+   pr_err("Could not allocate memory for new connection\n");
+   return NULL;
+   }
+   pr_debug("Moving to TARG_CONN_STATE_FREE.\n");
+   conn->conn_state = TARG_CONN_STATE_FREE;
+
+   init_waitqueue_head(>queues_wq);
+   INIT_LIST_HEAD(>conn_list);
+   INIT_LIST_HEAD(>conn_cmd_list);
+   INIT_LIST_HEAD(>immed_queue_list);
+   INIT_LIST_HEAD(>response_queue_list);
+   init_completion(>conn_post_wait_comp);
+   init_completion(>conn_wait_comp);
+   init_completion(>conn_wait_rcfr_comp);
+   init_completion(>conn_waiting_on_uc_comp);
+   init_completion(>conn_logout_comp);
+   init_completion(>rx_half_close_comp);
+   init_completion(>tx_half_close_comp);
+   init_completion(>rx_login_comp);
+   spin_lock_init(>cmd_lock);
+  

Re: [PATCH 0/3]: iscsi target login fixes

[Mike Christie]

On 08/27/2018 12:03 AM, Matthew Wilcox wrote:
> On Fri, Aug 24, 2018 at 01:37:10PM -0500, Mike Christie wrote:
>> The following patchset is a round up of login fixes that have been
>> on the list and in Mathew's tree. They fix a couple of bugs in the
>> iscsi login failure handling path.
>>
>> The patches were made against Martin's 4.19/scsi-queue branch.
>>
>> Matthew, the first patch is one that I had sent to you that had a fix
>> for the idr issue you asked about on the list. There was a conflict so
>> I had to update the patch. I was not sure if you wanted me to keep
>> your signed off so to be safe I dropped it.
>>
>> Also, it sounded like your ida/idr patches were not going to make 4.19,
>> so hopefully the idr iscsi login fix will be merged already when your
>> patches get merged for 4.20, so you do not have to carry it anymore.
> 
> Christmas came early, and Linus decided to pull the updated ida patches.
> 
> commit 26abc916a898d34c5ad159315a2f683def3c
> Author: Mike Christie 
> Date:   Thu Jul 26 12:13:49 2018 -0500
> 
> iscsi target: fix session creation failure handling
> 
> so patch 1 does not need to be added to v4.19/scsi-queue
> 

Martin,

I will remake and resend the last 2 patches against the current Linus
tree, because the second patch does not cleanly apply against the
idr/ida changes.

[PATCH 3/3] iscsi target: fix conn_ops double free

[Mike Christie]

If iscsi_login_init_conn fails it can free conn_ops.
__iscsi_target_login_thread will then call iscsi_target_login_sess_out
which will also free it.

This fixes the problem by organizing conn allocation/setup into parts
that are needed through the life of the conn and parts that are only
needed for the login. The free functions then release what was allocated
in the alloc functions.

With this patch we have:

iscsit_alloc_conn/iscsit_free_conn - allocs/frees the conn we need
for the entire life of the conn.

iscsi_login_init_conn/iscsi_target_nego_release - allocs/frees the parts
of the conn that are only needed during login.

Signed-off-by: Mike Christie 
---
 drivers/target/iscsi/iscsi_target.c   |   9 +-
 drivers/target/iscsi/iscsi_target_login.c | 141 --
 drivers/target/iscsi/iscsi_target_login.h |   2 +-
 3 files changed, 77 insertions(+), 75 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index 8e22379..a4ecc9d 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4211,22 +4211,15 @@ int iscsit_close_connection(
crypto_free_ahash(tfm);
}
 
-   free_cpumask_var(conn->conn_cpumask);
-
-   kfree(conn->conn_ops);
-   conn->conn_ops = NULL;
-
if (conn->sock)
sock_release(conn->sock);
 
if (conn->conn_transport->iscsit_free_conn)
conn->conn_transport->iscsit_free_conn(conn);
 
-   iscsit_put_transport(conn->conn_transport);
-
pr_debug("Moving to TARG_CONN_STATE_FREE.\n");
conn->conn_state = TARG_CONN_STATE_FREE;
-   kfree(conn);
+   iscsit_free_conn(conn);
 
spin_lock_bh(>conn_lock);
atomic_dec(>nconn);
diff --git a/drivers/target/iscsi/iscsi_target_login.c 
b/drivers/target/iscsi/iscsi_target_login.c
index f6b382d..590c39b 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -67,45 +67,10 @@ static struct iscsi_login *iscsi_login_init_conn(struct 
iscsi_conn *conn)
goto out_req_buf;
}
 
-   conn->conn_ops = kzalloc(sizeof(struct iscsi_conn_ops), GFP_KERNEL);
-   if (!conn->conn_ops) {
-   pr_err("Unable to allocate memory for"
-   " struct iscsi_conn_ops.\n");
-   goto out_rsp_buf;
-   }
-
-   init_waitqueue_head(>queues_wq);
-   INIT_LIST_HEAD(>conn_list);
-   INIT_LIST_HEAD(>conn_cmd_list);
-   INIT_LIST_HEAD(>immed_queue_list);
-   INIT_LIST_HEAD(>response_queue_list);
-   init_completion(>conn_post_wait_comp);
-   init_completion(>conn_wait_comp);
-   init_completion(>conn_wait_rcfr_comp);
-   init_completion(>conn_waiting_on_uc_comp);
-   init_completion(>conn_logout_comp);
-   init_completion(>rx_half_close_comp);
-   init_completion(>tx_half_close_comp);
-   init_completion(>rx_login_comp);
-   spin_lock_init(>cmd_lock);
-   spin_lock_init(>conn_usage_lock);
-   spin_lock_init(>immed_queue_lock);
-   spin_lock_init(>nopin_timer_lock);
-   spin_lock_init(>response_queue_lock);
-   spin_lock_init(>state_lock);
-
-   if (!zalloc_cpumask_var(>conn_cpumask, GFP_KERNEL)) {
-   pr_err("Unable to allocate conn->conn_cpumask\n");
-   goto out_conn_ops;
-   }
conn->conn_login = login;
 
return login;
 
-out_conn_ops:
-   kfree(conn->conn_ops);
-out_rsp_buf:
-   kfree(login->rsp_buf);
 out_req_buf:
kfree(login->req_buf);
 out_login:
@@ -1157,6 +1122,75 @@ int iscsit_put_login_tx(struct iscsi_conn *conn, struct 
iscsi_login *login,
return 0;
 }
 
+static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
+{
+   struct iscsi_conn *conn;
+
+   conn = kzalloc(sizeof(struct iscsi_conn), GFP_KERNEL);
+   if (!conn) {
+   pr_err("Could not allocate memory for new connection\n");
+   return NULL;
+   }
+   pr_debug("Moving to TARG_CONN_STATE_FREE.\n");
+   conn->conn_state = TARG_CONN_STATE_FREE;
+
+   init_waitqueue_head(>queues_wq);
+   INIT_LIST_HEAD(>conn_list);
+   INIT_LIST_HEAD(>conn_cmd_list);
+   INIT_LIST_HEAD(>immed_queue_list);
+   INIT_LIST_HEAD(>response_queue_list);
+   init_completion(>conn_post_wait_comp);
+   init_completion(>conn_wait_comp);
+   init_completion(>conn_wait_rcfr_comp);
+   init_completion(>conn_waiting_on_uc_comp);
+   init_completion(>conn_logout_comp);
+   init_completion(>rx_half_close_comp);
+   init_completion(>tx_half_close_comp);
+   init_completion(>rx_login_comp);
+   spin_lock_init(>cmd_lock);
+  

[PATCH 0/3]: iscsi target login fixes

[Mike Christie]

The following patchset is a round up of login fixes that have been
on the list and in Mathew's tree. They fix a couple of bugs in the
iscsi login failure handling path.

The patches were made against Martin's 4.19/scsi-queue branch.

Matthew, the first patch is one that I had sent to you that had a fix
for the idr issue you asked about on the list. There was a conflict so
I had to update the patch. I was not sure if you wanted me to keep
your signed off so to be safe I dropped it.

Also, it sounded like your ida/idr patches were not going to make 4.19,
so hopefully the idr iscsi login fix will be merged already when your
patches get merged for 4.20, so you do not have to carry it anymore.

[PATCH 2/3] iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails

[Mike Christie]

From: Vincent Pelletier 

Fixes a use-after-free reported by KASAN when later
iscsi_target_login_sess_out gets called and it tries to access
conn->sess->se_sess:

Disabling lock debugging due to kernel taint
iSCSI Login timeout on Network Portal [::]:3260
iSCSI Login negotiation failed.
==
BUG: KASAN: use-after-free in iscsi_target_login_sess_out.cold.12+0x58/0xff 
[iscsi_target_mod]
Read of size 8 at addr 880109d070c8 by task iscsi_np/980

CPU: 1 PID: 980 Comm: iscsi_np Tainted: G   O  
4.17.8kasan.sess.connops+ #4
Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 
5.6.5 05/19/2014
Call Trace:
 dump_stack+0x71/0xac
 print_address_description+0x65/0x22e
 ? iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
 kasan_report.cold.6+0x241/0x2fd
 iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
 iscsi_target_login_thread+0x1086/0x1710 [iscsi_target_mod]
 ? __sched_text_start+0x8/0x8
 ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
 ? __kthread_parkme+0xcc/0x100
 ? parse_args.cold.14+0xd3/0xd3
 ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
 kthread+0x1a0/0x1c0
 ? kthread_bind+0x30/0x30
 ret_from_fork+0x35/0x40

Allocated by task 980:
 kasan_kmalloc+0xbf/0xe0
 kmem_cache_alloc_trace+0x112/0x210
 iscsi_target_login_thread+0x816/0x1710 [iscsi_target_mod]
 kthread+0x1a0/0x1c0
 ret_from_fork+0x35/0x40

Freed by task 980:
 __kasan_slab_free+0x125/0x170
 kfree+0x90/0x1d0
 iscsi_target_login_thread+0x1577/0x1710 [iscsi_target_mod]
 kthread+0x1a0/0x1c0
 ret_from_fork+0x35/0x40

The buggy address belongs to the object at 880109d06f00
 which belongs to the cache kmalloc-512 of size 512
The buggy address is located 456 bytes inside of
 512-byte region [880109d06f00, 880109d07100)
The buggy address belongs to the page:
page:ea0004274180 count:1 mapcount:0 mapping: index:0x0 
compound_mapcount: 0
flags: 0x17fffc08100(slab|head)
raw: 017fffc08100   0001000c000c
raw: dead0100 dead0200 88011b002e00 
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 880109d06f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 880109d07000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>880109d07080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
  ^
 880109d07100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 880109d07180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==

Also, let idr_alloc return value through instead of replacing it with -ENOMEM,
as it is already a negative value and caller checks sign, not exact value.

Signed-off-by: Vincent Pelletier 
Signed-off-by: Mike Christie 
---
 drivers/target/iscsi/iscsi_target_login.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_login.c 
b/drivers/target/iscsi/iscsi_target_login.c
index 2d8d353..f6b382d 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -311,10 +311,8 @@ static int iscsi_login_zero_tsih_s1(
}
 
ret = iscsi_login_set_conn_values(sess, conn, pdu->cid);
-   if (unlikely(ret)) {
-   kfree(sess);
-   return ret;
-   }
+   if (unlikely(ret))
+   goto free_sess;
sess->init_task_tag = pdu->itt;
memcpy(>isid, pdu->isid, 6);
sess->exp_cmd_sn= be32_to_cpu(pdu->cmdsn);
@@ -364,6 +362,7 @@ static int iscsi_login_zero_tsih_s1(
ISCSI_LOGIN_STATUS_NO_RESOURCES);
pr_err("Unable to allocate memory for"
" struct iscsi_sess_ops.\n");
+   ret = -ENOMEM;
goto remove_idr;
}
 
@@ -371,6 +370,7 @@ static int iscsi_login_zero_tsih_s1(
if (IS_ERR(sess->se_sess)) {
iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
ISCSI_LOGIN_STATUS_NO_RESOURCES);
+   ret = -ENOMEM;
goto free_ops;
}
 
@@ -385,7 +385,7 @@ static int iscsi_login_zero_tsih_s1(
 free_sess:
kfree(sess);
conn->sess = NULL;
-   return -ENOMEM;
+   return ret;
 }
 
 static int iscsi_login_zero_tsih_s2(
-- 
1.8.3.1

[PATCH 1/3] iscsi target: fix session creation failure handling

[Mike Christie]

The problem is that iscsi_login_zero_tsih_s1 sets conn->sess early in
iscsi_login_set_conn_values. If the function fails later like when we
alloc the idr it does kfree(sess) it leaves the conn->sess pointer set.
iscsi_login_zero_tsih_s1 then returns -Exyz and we then call
iscsi_target_login_sess_out and access the freed memory.

This patch has iscsi_login_zero_tsih_s1 either completely setup the
session or completely tear it down, so later in
iscsi_target_login_sess_out we can just check for it being set to the
connection.

Fixes: 0957627a9960 ("iscsi-target: Fix sess allocation leak in...")
Signed-off-by: Mike Christie 
Cc: Matthew Wilcox 

---
 drivers/target/iscsi/iscsi_target_login.c | 35 ++-
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_login.c 
b/drivers/target/iscsi/iscsi_target_login.c
index 923b1a9..2d8d353 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -348,8 +348,7 @@ static int iscsi_login_zero_tsih_s1(
pr_err("idr_alloc() for sess_idr failed\n");
iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
ISCSI_LOGIN_STATUS_NO_RESOURCES);
-   kfree(sess);
-   return -ENOMEM;
+   goto free_sess;
}
 
sess->creation_time = get_jiffies_64();
@@ -365,20 +364,28 @@ static int iscsi_login_zero_tsih_s1(
ISCSI_LOGIN_STATUS_NO_RESOURCES);
pr_err("Unable to allocate memory for"
" struct iscsi_sess_ops.\n");
-   kfree(sess);
-   return -ENOMEM;
+   goto remove_idr;
}
 
sess->se_sess = transport_alloc_session(TARGET_PROT_NORMAL);
if (IS_ERR(sess->se_sess)) {
iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
ISCSI_LOGIN_STATUS_NO_RESOURCES);
-   kfree(sess->sess_ops);
-   kfree(sess);
-   return -ENOMEM;
+   goto free_ops;
}
 
return 0;
+
+free_ops:
+   kfree(sess->sess_ops);
+remove_idr:
+   spin_lock_bh(_idr_lock);
+   idr_remove(_idr, sess->session_index);
+   spin_unlock_bh(_idr_lock);
+free_sess:
+   kfree(sess);
+   conn->sess = NULL;
+   return -ENOMEM;
 }
 
 static int iscsi_login_zero_tsih_s2(
@@ -1161,13 +1168,13 @@ void iscsi_target_login_sess_out(struct iscsi_conn 
*conn,
   ISCSI_LOGIN_STATUS_INIT_ERR);
if (!zero_tsih || !conn->sess)
goto old_sess_out;
-   if (conn->sess->se_sess)
-   transport_free_session(conn->sess->se_sess);
-   if (conn->sess->session_index != 0) {
-   spin_lock_bh(_idr_lock);
-   idr_remove(_idr, conn->sess->session_index);
-   spin_unlock_bh(_idr_lock);
-   }
+
+   transport_free_session(conn->sess->se_sess);
+
+   spin_lock_bh(_idr_lock);
+   idr_remove(_idr, conn->sess->session_index);
+   spin_unlock_bh(_idr_lock);
+
kfree(conn->sess->sess_ops);
kfree(conn->sess);
conn->sess = NULL;
-- 
1.8.3.1

Re: [PATCHv2] iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails

[Mike Christie]

On 08/23/2018 09:11 PM, Martin K. Petersen wrote:
> 
> Mike,
> 
>> this was made over that patch that was going through Mathew's tree. I
>> do not know exactly which one though. It is in next here
> 
> Doesn't look like Linus will pull Matthew's tree for 4.19.
>

It looks like there is also going to be a conflict with another patch
upstream. I will just round up all the patches and resend in one set
rebased against your current for-4.19 tree.

Re: [PATCH] target/iblock: split T10 PI SGL across command bios

[Mike Christie]

On 08/08/2018 02:31 PM, Greg Edwards wrote:
> When T10 PI is enabled on a backing device for the iblock backstore, the
> PI SGL for the entire command is attached to the first bio only.  This
> works fine if the command is covered by a single bio, but results in
> integrity verification errors for the other bios in a multi-bio command.
> 

Did you hit this with a older distro kernel?

It looks like iblock_get_bio will alloc a bio that has enough vecs for
the entire cmd (bi_max_vecs will equal sgl_nents). So it is not clear to
me how does the bio_add_page call ever return a value other than
sg->length, and we end up doing another iblock_get_bio call?


> Split the PI SGL across the bios in the command, so each bip contains
> the protection information for the data in the bio.  In a multi-bio
> command, store where we left off in the PI SGL so we know where to start
> with the next bio.
> 
> Signed-off-by: Greg Edwards 
> ---
> This patch depends on commit 359f642700f2 ("block: move
> bio_integrity_{intervals,bytes} into blkdev.h") in Jens' block for-next 
> branch.
> 
>  drivers/target/target_core_iblock.c | 60 +
>  1 file changed, 44 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/target/target_core_iblock.c 
> b/drivers/target/target_core_iblock.c
> index ce1321a5cb7b..d3ab83282f61 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -635,7 +635,8 @@ static ssize_t iblock_show_configfs_dev_params(struct 
> se_device *dev, char *b)
>  }
>  
>  static int
> -iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio)
> +iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio,
> +  struct scatterlist **sgl, unsigned int *skip)
>  {
>   struct se_device *dev = cmd->se_dev;
>   struct blk_integrity *bi;
> @@ -643,6 +644,7 @@ iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio)
>   struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
>   struct scatterlist *sg;
>   int i, rc;
> + unsigned int size, nr_pages, len, off;
>  
>   bi = bdev_get_integrity(ib_dev->ibd_bd);
>   if (!bi) {
> @@ -650,32 +652,52 @@ iblock_alloc_bip(struct se_cmd *cmd, struct bio *bio)
>   return -ENODEV;
>   }
>  
> - bip = bio_integrity_alloc(bio, GFP_NOIO, cmd->t_prot_nents);
> + nr_pages = min_t(unsigned int, cmd->t_prot_nents, BIO_MAX_PAGES);
> + bip = bio_integrity_alloc(bio, GFP_NOIO, nr_pages);
>   if (IS_ERR(bip)) {
>   pr_err("Unable to allocate bio_integrity_payload\n");
>   return PTR_ERR(bip);
>   }
>  
> - bip->bip_iter.bi_size = (cmd->data_length / dev->dev_attrib.block_size) 
> *
> -  dev->prot_length;
> - bip->bip_iter.bi_sector = bio->bi_iter.bi_sector;
> + bip->bip_iter.bi_size = bio_integrity_bytes(bi, bio_sectors(bio));
> + bip_set_seed(bip, bio->bi_iter.bi_sector);
>  
>   pr_debug("IBLOCK BIP Size: %u Sector: %llu\n", bip->bip_iter.bi_size,
>(unsigned long long)bip->bip_iter.bi_sector);
>  
> - for_each_sg(cmd->t_prot_sg, sg, cmd->t_prot_nents, i) {
> + size = bip->bip_iter.bi_size;
> + for_each_sg(*sgl, sg, nr_pages, i) {
>  
> - rc = bio_integrity_add_page(bio, sg_page(sg), sg->length,
> - sg->offset);
> - if (rc != sg->length) {
> + len = sg->length - *skip;
> + off = sg->offset + *skip;
> +
> + if (*skip)
> + *skip = 0;
> +
> + if (len > size) {
> + len = size;
> + *skip = len;
> + }
> +
> + rc = bio_integrity_add_page(bio, sg_page(sg), len, off);
> + if (rc != len) {
>   pr_err("bio_integrity_add_page() failed; %d\n", rc);
>   return -ENOMEM;
>   }
>  
>   pr_debug("Added bio integrity page: %p length: %d offset; %d\n",
> -  sg_page(sg), sg->length, sg->offset);
> +  sg_page(sg), len, off);
> +
> + size -= len;
> + if (!size)
> + break;
>   }
>  
> + if (*skip == 0)
> + *sgl = sg_next(sg);
> + else
> + *sgl = sg;
> +
>   return 0;
>  }
>  
> @@ -686,12 +708,12 @@ iblock_execute_rw(struct se_cmd *cmd, struct 
> scatterlist *sgl, u32 sgl_nents,
>   struct se_device *dev = cmd->se_dev;
>   sector_t block_lba = target_to_linux_sector(dev, cmd->t_task_lba);
>   struct iblock_req *ibr;
> - struct bio *bio, *bio_start;
> + struct bio *bio;
>   struct bio_list list;
> - struct scatterlist *sg;
> + struct scatterlist *sg, *sg_prot = cmd->t_prot_sg;
>   u32 sg_num = sgl_nents;
> - unsigned bio_cnt;
> - int i, op, op_flags = 0;
> + unsigned int bio_cnt, skip = 0;
> + int i, rc, op, op_flags = 0;
>  
>   if (data_direction == DMA_TO_DEVICE) {
> 

Re: [PATCHv2] iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails

[Mike Christie]

On 08/15/2018 05:56 PM, Vincent Pelletier wrote:
> Fixes a use-after-free reported by KASAN when later
> iscsi_target_login_sess_out gets called and it tries to access
> conn->sess->se_sess:
> 
> Disabling lock debugging due to kernel taint
> iSCSI Login timeout on Network Portal [::]:3260
> iSCSI Login negotiation failed.
> ==
> BUG: KASAN: use-after-free in iscsi_target_login_sess_out.cold.12+0x58/0xff 
> [iscsi_target_mod]
> Read of size 8 at addr 880109d070c8 by task iscsi_np/980
> 
> CPU: 1 PID: 980 Comm: iscsi_np Tainted: G   O  
> 4.17.8kasan.sess.connops+ #4
> Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 
> 5.6.5 05/19/2014
> Call Trace:
>  dump_stack+0x71/0xac
>  print_address_description+0x65/0x22e
>  ? iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
>  kasan_report.cold.6+0x241/0x2fd
>  iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
>  iscsi_target_login_thread+0x1086/0x1710 [iscsi_target_mod]
>  ? __sched_text_start+0x8/0x8
>  ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
>  ? __kthread_parkme+0xcc/0x100
>  ? parse_args.cold.14+0xd3/0xd3
>  ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
>  kthread+0x1a0/0x1c0
>  ? kthread_bind+0x30/0x30
>  ret_from_fork+0x35/0x40
> 
> Allocated by task 980:
>  kasan_kmalloc+0xbf/0xe0
>  kmem_cache_alloc_trace+0x112/0x210
>  iscsi_target_login_thread+0x816/0x1710 [iscsi_target_mod]
>  kthread+0x1a0/0x1c0
>  ret_from_fork+0x35/0x40
> 
> Freed by task 980:
>  __kasan_slab_free+0x125/0x170
>  kfree+0x90/0x1d0
>  iscsi_target_login_thread+0x1577/0x1710 [iscsi_target_mod]
>  kthread+0x1a0/0x1c0
>  ret_from_fork+0x35/0x40
> 
> The buggy address belongs to the object at 880109d06f00
>  which belongs to the cache kmalloc-512 of size 512
> The buggy address is located 456 bytes inside of
>  512-byte region [880109d06f00, 880109d07100)
> The buggy address belongs to the page:
> page:ea0004274180 count:1 mapcount:0 mapping: index:0x0 
> compound_mapcount: 0
> flags: 0x17fffc08100(slab|head)
> raw: 017fffc08100   0001000c000c
> raw: dead0100 dead0200 88011b002e00 
> page dumped because: kasan: bad access detected
> 
> Memory state around the buggy address:
>  880109d06f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  880109d07000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> 880109d07080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>   ^
>  880109d07100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>  880109d07180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ==
> 
> Also, let idr_alloc return value through instead of replacing it with -ENOMEM,
> as it is already a negative value and caller checks sign, not exact value.
> 
> Signed-off-by: Vincent Pelletier 
> ---
>  drivers/target/iscsi/iscsi_target_login.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_login.c 
> b/drivers/target/iscsi/iscsi_target_login.c
> index df81b2f7cad9..7337ff80394e 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c
> @@ -296,10 +296,8 @@ static int iscsi_login_zero_tsih_s1(
>   }
>  
>   ret = iscsi_login_set_conn_values(sess, conn, pdu->cid);
> - if (unlikely(ret)) {
> - kfree(sess);
> - return ret;
> - }
> + if (unlikely(ret))
> + goto free_sess;
>   sess->init_task_tag = pdu->itt;
>   memcpy(>isid, pdu->isid, 6);
>   sess->exp_cmd_sn= be32_to_cpu(pdu->cmdsn);
> @@ -349,6 +347,7 @@ static int iscsi_login_zero_tsih_s1(
>   ISCSI_LOGIN_STATUS_NO_RESOURCES);
>   pr_err("Unable to allocate memory for"
>   " struct iscsi_sess_ops.\n");
> + ret = -ENOMEM;
>   goto remove_idr;
>   }
>  
> @@ -356,6 +355,7 @@ static int iscsi_login_zero_tsih_s1(
>   if (IS_ERR(sess->se_sess)) {
>   iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
>   ISCSI_LOGIN_STATUS_NO_RESOURCES);
> + ret = -ENOMEM;
>   goto free_ops;
>   }
>  
> @@ -370,7 +370,7 @@ static int iscsi_login_zero_tsih_s1(
>  free_sess:
>   kfree(sess);
>   

Re: iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails

[Mike Christie]

On 08/15/2018 10:59 AM, Mike Christie wrote:
> On 08/15/2018 05:19 AM, Vincent Pelletier wrote:
>> Fixes a use-after-free reported by KASAN when later
>> iscsi_target_login_sess_out gets called and it tries to access
>> conn->sess->se_sess:
>>
>> Disabling lock debugging due to kernel taint
>> iSCSI Login timeout on Network Portal [::]:3260
>> iSCSI Login negotiation failed.
>> ==
>> BUG: KASAN: use-after-free in iscsi_target_login_sess_out.cold.12+0x58/0xff 
>> [iscsi_target_mod]
>> Read of size 8 at addr 880109d070c8 by task iscsi_np/980
>>
>> CPU: 1 PID: 980 Comm: iscsi_np Tainted: G   O  
>> 4.17.8kasan.sess.connops+ #4
>> Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 
>> 5.6.5 05/19/2014
>> Call Trace:
>>  dump_stack+0x71/0xac
>>  print_address_description+0x65/0x22e
>>  ? iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
>>  kasan_report.cold.6+0x241/0x2fd
>>  iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
>>  iscsi_target_login_thread+0x1086/0x1710 [iscsi_target_mod]
>>  ? __sched_text_start+0x8/0x8
>>  ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
>>  ? __kthread_parkme+0xcc/0x100
>>  ? parse_args.cold.14+0xd3/0xd3
>>  ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
>>  kthread+0x1a0/0x1c0
>>  ? kthread_bind+0x30/0x30
>>  ret_from_fork+0x35/0x40
>>
>> Allocated by task 980:
>>  kasan_kmalloc+0xbf/0xe0
>>  kmem_cache_alloc_trace+0x112/0x210
>>  iscsi_target_login_thread+0x816/0x1710 [iscsi_target_mod]
>>  kthread+0x1a0/0x1c0
>>  ret_from_fork+0x35/0x40
>>
>> Freed by task 980:
>>  __kasan_slab_free+0x125/0x170
>>  kfree+0x90/0x1d0
>>  iscsi_target_login_thread+0x1577/0x1710 [iscsi_target_mod]
>>  kthread+0x1a0/0x1c0
>>  ret_from_fork+0x35/0x40
>>
>> The buggy address belongs to the object at 880109d06f00
>>  which belongs to the cache kmalloc-512 of size 512
>> The buggy address is located 456 bytes inside of
>>  512-byte region [880109d06f00, 880109d07100)
>> The buggy address belongs to the page:
>> page:ea0004274180 count:1 mapcount:0 mapping: index:0x0 
>> compound_mapcount: 0
>> flags: 0x17fffc08100(slab|head)
>> raw: 017fffc08100   0001000c000c
>> raw: dead0100 dead0200 88011b002e00 
>> page dumped because: kasan: bad access detected
>>
>> Memory state around the buggy address:
>>  880109d06f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>  880109d07000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>> 880109d07080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>   ^
>>  880109d07100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>>  880109d07180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> ==
>>
>> Signed-off-by: Vincent Pelletier 
>> ---
>>  drivers/target/iscsi/iscsi_target_login.c | 9 -
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/target/iscsi/iscsi_target_login.c 
>> b/drivers/target/iscsi/iscsi_target_login.c
>> index df81b2f7cad9..c95f4261a089 100644
>> --- a/drivers/target/iscsi/iscsi_target_login.c
>> +++ b/drivers/target/iscsi/iscsi_target_login.c
>> @@ -296,10 +296,8 @@ static int iscsi_login_zero_tsih_s1(
>>  }
>>  
>>  ret = iscsi_login_set_conn_values(sess, conn, pdu->cid);
>> -if (unlikely(ret)) {
>> -kfree(sess);
>> -return ret;
>> -}
>> +if (unlikely(ret))
>> +goto free_sess;
>>  sess->init_task_tag = pdu->itt;
>>  memcpy(>isid, pdu->isid, 6);
>>  sess->exp_cmd_sn= be32_to_cpu(pdu->cmdsn);
>> @@ -333,6 +331,7 @@ static int iscsi_login_zero_tsih_s1(
>>  pr_err("idr_alloc() for sess_idr failed\n");
>>  iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
>>  ISCSI_LOGIN_STATUS_NO_RESOURCES);
>> +ret = -ENOMEM;

Sorry to cause more confusion. I misunderstood what you were saying in
the other mails.

Here, I think it will be ok to not set ret.

>>  goto free_sess;
>>  }

But, you need to set ret = -ENOMEM in the goto remove_idr and goto
free_ops paths below the above chunk, because ret will be set to >= 0 at
those points and the caller checks for ret < 0.

>>  
>> @@ -370,7 +369,7 @@ static int iscsi_login_zero_tsih_s1(
>>  free_sess:
>>  kfree(sess);
>>  conn->sess = NULL;
>> -return -ENOMEM;
>> +return ret;
>>  }
>>  
>>  static int iscsi_login_zero_tsih_s2(
>>
> 
> Reviewed-by: Mike Christie 
> 

Please drop that reviewed-by.

Re: iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails

[Mike Christie]

On 08/15/2018 05:19 AM, Vincent Pelletier wrote:
> Fixes a use-after-free reported by KASAN when later
> iscsi_target_login_sess_out gets called and it tries to access
> conn->sess->se_sess:
> 
> Disabling lock debugging due to kernel taint
> iSCSI Login timeout on Network Portal [::]:3260
> iSCSI Login negotiation failed.
> ==
> BUG: KASAN: use-after-free in iscsi_target_login_sess_out.cold.12+0x58/0xff 
> [iscsi_target_mod]
> Read of size 8 at addr 880109d070c8 by task iscsi_np/980
> 
> CPU: 1 PID: 980 Comm: iscsi_np Tainted: G   O  
> 4.17.8kasan.sess.connops+ #4
> Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 
> 5.6.5 05/19/2014
> Call Trace:
>  dump_stack+0x71/0xac
>  print_address_description+0x65/0x22e
>  ? iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
>  kasan_report.cold.6+0x241/0x2fd
>  iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
>  iscsi_target_login_thread+0x1086/0x1710 [iscsi_target_mod]
>  ? __sched_text_start+0x8/0x8
>  ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
>  ? __kthread_parkme+0xcc/0x100
>  ? parse_args.cold.14+0xd3/0xd3
>  ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
>  kthread+0x1a0/0x1c0
>  ? kthread_bind+0x30/0x30
>  ret_from_fork+0x35/0x40
> 
> Allocated by task 980:
>  kasan_kmalloc+0xbf/0xe0
>  kmem_cache_alloc_trace+0x112/0x210
>  iscsi_target_login_thread+0x816/0x1710 [iscsi_target_mod]
>  kthread+0x1a0/0x1c0
>  ret_from_fork+0x35/0x40
> 
> Freed by task 980:
>  __kasan_slab_free+0x125/0x170
>  kfree+0x90/0x1d0
>  iscsi_target_login_thread+0x1577/0x1710 [iscsi_target_mod]
>  kthread+0x1a0/0x1c0
>  ret_from_fork+0x35/0x40
> 
> The buggy address belongs to the object at 880109d06f00
>  which belongs to the cache kmalloc-512 of size 512
> The buggy address is located 456 bytes inside of
>  512-byte region [880109d06f00, 880109d07100)
> The buggy address belongs to the page:
> page:ea0004274180 count:1 mapcount:0 mapping: index:0x0 
> compound_mapcount: 0
> flags: 0x17fffc08100(slab|head)
> raw: 017fffc08100   0001000c000c
> raw: dead0100 dead0200 88011b002e00 
> page dumped because: kasan: bad access detected
> 
> Memory state around the buggy address:
>  880109d06f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  880109d07000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> 880109d07080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>   ^
>  880109d07100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>  880109d07180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ==
> 
> Signed-off-by: Vincent Pelletier 
> ---
>  drivers/target/iscsi/iscsi_target_login.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_login.c 
> b/drivers/target/iscsi/iscsi_target_login.c
> index df81b2f7cad9..c95f4261a089 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c
> @@ -296,10 +296,8 @@ static int iscsi_login_zero_tsih_s1(
>   }
>  
>   ret = iscsi_login_set_conn_values(sess, conn, pdu->cid);
> - if (unlikely(ret)) {
> - kfree(sess);
> - return ret;
> - }
> + if (unlikely(ret))
> + goto free_sess;
>   sess->init_task_tag = pdu->itt;
>   memcpy(>isid, pdu->isid, 6);
>   sess->exp_cmd_sn= be32_to_cpu(pdu->cmdsn);
> @@ -333,6 +331,7 @@ static int iscsi_login_zero_tsih_s1(
>   pr_err("idr_alloc() for sess_idr failed\n");
>   iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
>   ISCSI_LOGIN_STATUS_NO_RESOURCES);
> + ret = -ENOMEM;
>   goto free_sess;
>   }
>  
> @@ -370,7 +369,7 @@ static int iscsi_login_zero_tsih_s1(
>  free_sess:
>   kfree(sess);
>   conn->sess = NULL;
> - return -ENOMEM;
> + return ret;
>  }
>  
>  static int iscsi_login_zero_tsih_s2(
> 

Reviewed-by: Mike Christie 

Martin, this was made over that patch that was going through Mathew's
tree. It is in next here

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/target/iscsi?id=ad86353cbddbb6f1c95072ead744d547a9ac8211

Re: iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails

[Mike Christie]

On 08/15/2018 10:44 AM, Mike Christie wrote:
> On 08/15/2018 05:19 AM, Vincent Pelletier wrote:
>> Fixes a use-after-free reported by KASAN when later
>> iscsi_target_login_sess_out gets called and it tries to access
>> conn->sess->se_sess:
>>
>> Disabling lock debugging due to kernel taint
>> iSCSI Login timeout on Network Portal [::]:3260
>> iSCSI Login negotiation failed.
>> ==
>> BUG: KASAN: use-after-free in iscsi_target_login_sess_out.cold.12+0x58/0xff 
>> [iscsi_target_mod]
>> Read of size 8 at addr 880109d070c8 by task iscsi_np/980
>>
>> CPU: 1 PID: 980 Comm: iscsi_np Tainted: G   O  
>> 4.17.8kasan.sess.connops+ #4
>> Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 
>> 5.6.5 05/19/2014
>> Call Trace:
>>  dump_stack+0x71/0xac
>>  print_address_description+0x65/0x22e
>>  ? iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
>>  kasan_report.cold.6+0x241/0x2fd
>>  iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
>>  iscsi_target_login_thread+0x1086/0x1710 [iscsi_target_mod]
>>  ? __sched_text_start+0x8/0x8
>>  ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
>>  ? __kthread_parkme+0xcc/0x100
>>  ? parse_args.cold.14+0xd3/0xd3
>>  ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
>>  kthread+0x1a0/0x1c0
>>  ? kthread_bind+0x30/0x30
>>  ret_from_fork+0x35/0x40
>>
>> Allocated by task 980:
>>  kasan_kmalloc+0xbf/0xe0
>>  kmem_cache_alloc_trace+0x112/0x210
>>  iscsi_target_login_thread+0x816/0x1710 [iscsi_target_mod]
>>  kthread+0x1a0/0x1c0
>>  ret_from_fork+0x35/0x40
>>
>> Freed by task 980:
>>  __kasan_slab_free+0x125/0x170
>>  kfree+0x90/0x1d0
>>  iscsi_target_login_thread+0x1577/0x1710 [iscsi_target_mod]
>>  kthread+0x1a0/0x1c0
>>  ret_from_fork+0x35/0x40
>>
>> The buggy address belongs to the object at 880109d06f00
>>  which belongs to the cache kmalloc-512 of size 512
>> The buggy address is located 456 bytes inside of
>>  512-byte region [880109d06f00, 880109d07100)
>> The buggy address belongs to the page:
>> page:ea0004274180 count:1 mapcount:0 mapping: index:0x0 
>> compound_mapcount: 0
>> flags: 0x17fffc08100(slab|head)
>> raw: 017fffc08100   0001000c000c
>> raw: dead0100 dead0200 88011b002e00 
>> page dumped because: kasan: bad access detected
>>
>> Memory state around the buggy address:
>>  880109d06f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>  880109d07000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>> 880109d07080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>   ^
>>  880109d07100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>>  880109d07180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> ==
>>
>> Signed-off-by: Vincent Pelletier 
>> ---
>>  drivers/target/iscsi/iscsi_target_login.c | 9 -
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/target/iscsi/iscsi_target_login.c 
>> b/drivers/target/iscsi/iscsi_target_login.c
>> index df81b2f7cad9..c95f4261a089 100644
>> --- a/drivers/target/iscsi/iscsi_target_login.c
>> +++ b/drivers/target/iscsi/iscsi_target_login.c
>> @@ -296,10 +296,8 @@ static int iscsi_login_zero_tsih_s1(
>>  }
>>  
>>  ret = iscsi_login_set_conn_values(sess, conn, pdu->cid);
>> -if (unlikely(ret)) {
>> -kfree(sess);
>> -return ret;
>> -}
>> +if (unlikely(ret))
>> +goto free_sess;
>>  sess->init_task_tag = pdu->itt;
>>  memcpy(>isid, pdu->isid, 6);
>>  sess->exp_cmd_sn= be32_to_cpu(pdu->cmdsn);
>> @@ -333,6 +331,7 @@ static int iscsi_login_zero_tsih_s1(
>>  pr_err("idr_alloc() for sess_idr failed\n");
>>  iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
>>  ISCSI_LOGIN_STATUS_NO_RESOURCES);
>> +ret = -ENOMEM;
>>  goto free_sess;
>>  }
>>  
>> @@ -370,7 +369,7 @@ static int iscsi_login_zero_tsih_s1(
>>  free_sess:
>>  kfree(sess);
>>  conn->sess = NULL;
>> -return -ENOMEM;
>> +return ret;
>>  }
>>  
>>  static int iscsi_login_zero_tsih_s2(
>>
> 
> This is the issue I said was fixed in:
> 
> https://www.spinics.net/lists/target-devel/msg17018.html
> 

Sorry. I see your patch is made over mine.

You are right. We need your patch too, because
iscsi_login_set_conn_values does not clear the sess on failure before
returning. The new return value in your patch fine.

Re: iscsi target: Set conn->sess to NULL when iscsi_login_set_conn_values fails

[Mike Christie]

On 08/15/2018 05:19 AM, Vincent Pelletier wrote:
> Fixes a use-after-free reported by KASAN when later
> iscsi_target_login_sess_out gets called and it tries to access
> conn->sess->se_sess:
> 
> Disabling lock debugging due to kernel taint
> iSCSI Login timeout on Network Portal [::]:3260
> iSCSI Login negotiation failed.
> ==
> BUG: KASAN: use-after-free in iscsi_target_login_sess_out.cold.12+0x58/0xff 
> [iscsi_target_mod]
> Read of size 8 at addr 880109d070c8 by task iscsi_np/980
> 
> CPU: 1 PID: 980 Comm: iscsi_np Tainted: G   O  
> 4.17.8kasan.sess.connops+ #4
> Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 
> 5.6.5 05/19/2014
> Call Trace:
>  dump_stack+0x71/0xac
>  print_address_description+0x65/0x22e
>  ? iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
>  kasan_report.cold.6+0x241/0x2fd
>  iscsi_target_login_sess_out.cold.12+0x58/0xff [iscsi_target_mod]
>  iscsi_target_login_thread+0x1086/0x1710 [iscsi_target_mod]
>  ? __sched_text_start+0x8/0x8
>  ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
>  ? __kthread_parkme+0xcc/0x100
>  ? parse_args.cold.14+0xd3/0xd3
>  ? iscsi_target_login_sess_out+0x250/0x250 [iscsi_target_mod]
>  kthread+0x1a0/0x1c0
>  ? kthread_bind+0x30/0x30
>  ret_from_fork+0x35/0x40
> 
> Allocated by task 980:
>  kasan_kmalloc+0xbf/0xe0
>  kmem_cache_alloc_trace+0x112/0x210
>  iscsi_target_login_thread+0x816/0x1710 [iscsi_target_mod]
>  kthread+0x1a0/0x1c0
>  ret_from_fork+0x35/0x40
> 
> Freed by task 980:
>  __kasan_slab_free+0x125/0x170
>  kfree+0x90/0x1d0
>  iscsi_target_login_thread+0x1577/0x1710 [iscsi_target_mod]
>  kthread+0x1a0/0x1c0
>  ret_from_fork+0x35/0x40
> 
> The buggy address belongs to the object at 880109d06f00
>  which belongs to the cache kmalloc-512 of size 512
> The buggy address is located 456 bytes inside of
>  512-byte region [880109d06f00, 880109d07100)
> The buggy address belongs to the page:
> page:ea0004274180 count:1 mapcount:0 mapping: index:0x0 
> compound_mapcount: 0
> flags: 0x17fffc08100(slab|head)
> raw: 017fffc08100   0001000c000c
> raw: dead0100 dead0200 88011b002e00 
> page dumped because: kasan: bad access detected
> 
> Memory state around the buggy address:
>  880109d06f80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  880109d07000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> 880109d07080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>   ^
>  880109d07100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>  880109d07180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ==
> 
> Signed-off-by: Vincent Pelletier 
> ---
>  drivers/target/iscsi/iscsi_target_login.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_login.c 
> b/drivers/target/iscsi/iscsi_target_login.c
> index df81b2f7cad9..c95f4261a089 100644
> --- a/drivers/target/iscsi/iscsi_target_login.c
> +++ b/drivers/target/iscsi/iscsi_target_login.c
> @@ -296,10 +296,8 @@ static int iscsi_login_zero_tsih_s1(
>   }
>  
>   ret = iscsi_login_set_conn_values(sess, conn, pdu->cid);
> - if (unlikely(ret)) {
> - kfree(sess);
> - return ret;
> - }
> + if (unlikely(ret))
> + goto free_sess;
>   sess->init_task_tag = pdu->itt;
>   memcpy(>isid, pdu->isid, 6);
>   sess->exp_cmd_sn= be32_to_cpu(pdu->cmdsn);
> @@ -333,6 +331,7 @@ static int iscsi_login_zero_tsih_s1(
>   pr_err("idr_alloc() for sess_idr failed\n");
>   iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
>   ISCSI_LOGIN_STATUS_NO_RESOURCES);
> + ret = -ENOMEM;
>   goto free_sess;
>   }
>  
> @@ -370,7 +369,7 @@ static int iscsi_login_zero_tsih_s1(
>  free_sess:
>   kfree(sess);
>   conn->sess = NULL;
> - return -ENOMEM;
> + return ret;
>  }
>  
>  static int iscsi_login_zero_tsih_s2(
> 

This is the issue I said was fixed in:

https://www.spinics.net/lists/target-devel/msg17018.html

Your patch still has issues where the sess_ops can be double freed, and
there is a issue with the idr.

Re: BUG in slab_free after iSCSI login timeout

[Mike Christie]

On 08/13/2018 04:42 PM, Mike Christie wrote:
> On 08/13/2018 02:48 PM, Mike Christie wrote:
>> On 08/11/2018 10:51 PM, Vincent Pelletier wrote:
>>> On Sun, 12 Aug 2018 02:55:31 +, Vincent Pelletier
>>>  wrote:
>>>> Aug 12 04:44:53 boke kernel: [   64.737069] BUG: KASAN: use-after-free in 
>>>> iscsi_target_login_sess_out.cold.11+0x58/0x123 [iscsi_target_mod]
>>>> Aug 12 04:44:53 boke kernel: [   64.771148] BUG: KASAN: double-free or 
>>>> invalid-free in iscsi_target_login_sess_out.cold.11+0x103/0x123 
>>>> [iscsi_target_mod]
>>>
>>> If I'm reading the code correctly, the double-free would be
>>> iscsi_login_init_conn and iscsi_target_login_sess_out both calling
>>> kfree(conn->conn_ops), with the latter called by
>>> __iscsi_target_login_thread precisely when the former fails (returns
>>> NULL after freeing).
>>>
>>
>> I think I fixed that with this patch:
>>
>> https://www.spinics.net/lists/target-devel/msg17018.html
>>
>> It fixes a mix of problems double free of the ops, session and reference
>> after free.
> 
> Ignore this. I see you said conn. My patch fixed basically the same
> issue but with the session.

Could you try the attached patch? I have done a couple login/logout
tests only, but have not yet completed testing.

>From b6d6e8da919b775e9a0dae64628f4e32ec705feb Mon Sep 17 00:00:00 2001
From: Mike Christie 
Date: Mon, 13 Aug 2018 17:52:18 -0500
Subject: [PATCH] iscsi target: fix conn_ops double free

If iscsi_login_init_conn fails it can free conn_ops.
__iscsi_target_login_thread will then call iscsi_target_login_sess_out
which will also free it.

This prevents the bug by moving the non login-only items that need to
be allocated/setup to new functions iscsit_alloc/free_conn. These alloc
function is then called in __iscsi_target_login_thread and the free
unction is only called if the alloc function is successfull.

Signed-off-by: Mike Christie 
---
 drivers/target/iscsi/iscsi_target.c   |   9 +--
 drivers/target/iscsi/iscsi_target_login.c | 101 --
 drivers/target/iscsi/iscsi_target_login.h |   2 +-
 3 files changed, 57 insertions(+), 55 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 8e22379..a4ecc9d 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4211,22 +4211,15 @@ int iscsit_close_connection(
 		crypto_free_ahash(tfm);
 	}
 
-	free_cpumask_var(conn->conn_cpumask);
-
-	kfree(conn->conn_ops);
-	conn->conn_ops = NULL;
-
 	if (conn->sock)
 		sock_release(conn->sock);
 
 	if (conn->conn_transport->iscsit_free_conn)
 		conn->conn_transport->iscsit_free_conn(conn);
 
-	iscsit_put_transport(conn->conn_transport);
-
 	pr_debug("Moving to TARG_CONN_STATE_FREE.\n");
 	conn->conn_state = TARG_CONN_STATE_FREE;
-	kfree(conn);
+	iscsit_free_conn(conn);
 
 	spin_lock_bh(>conn_lock);
 	atomic_dec(>nconn);
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 923b1a9..e1bdfd5 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -67,13 +67,6 @@ static struct iscsi_login *iscsi_login_init_conn(struct iscsi_conn *conn)
 		goto out_req_buf;
 	}
 
-	conn->conn_ops = kzalloc(sizeof(struct iscsi_conn_ops), GFP_KERNEL);
-	if (!conn->conn_ops) {
-		pr_err("Unable to allocate memory for"
-			" struct iscsi_conn_ops.\n");
-		goto out_rsp_buf;
-	}
-
 	init_waitqueue_head(>queues_wq);
 	INIT_LIST_HEAD(>conn_list);
 	INIT_LIST_HEAD(>conn_cmd_list);
@@ -94,18 +87,10 @@ static struct iscsi_login *iscsi_login_init_conn(struct iscsi_conn *conn)
 	spin_lock_init(>response_queue_lock);
 	spin_lock_init(>state_lock);
 
-	if (!zalloc_cpumask_var(>conn_cpumask, GFP_KERNEL)) {
-		pr_err("Unable to allocate conn->conn_cpumask\n");
-		goto out_conn_ops;
-	}
 	conn->conn_login = login;
 
 	return login;
 
-out_conn_ops:
-	kfree(conn->conn_ops);
-out_rsp_buf:
-	kfree(login->rsp_buf);
 out_req_buf:
 	kfree(login->req_buf);
 out_login:
@@ -1150,6 +1135,55 @@ int iscsit_put_login_tx(struct iscsi_conn *conn, struct iscsi_login *login,
 	return 0;
 }
 
+static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np)
+{
+	struct iscsi_conn *conn;
+
+	conn = kzalloc(sizeof(struct iscsi_conn), GFP_KERNEL);
+	if (!conn) {
+		pr_err("Could not allocate memory for new connection\n");
+		return NULL;
+	}
+	pr_debug("Moving to TARG_CONN_STATE_FREE.\n");
+	conn->conn_state = TARG_CONN_STATE_FREE;
+
+	timer_setup(>nopin_response_timer,
+		iscsit_handle_nopin_response_timeout, 0);
+	timer_setup(>nopin_timer, iscsit_handle_nopin_timeout, 0);
+
+	if (iscsit

Re: BUG in slab_free after iSCSI login timeout

[Mike Christie]

On 08/13/2018 02:48 PM, Mike Christie wrote:
> On 08/11/2018 10:51 PM, Vincent Pelletier wrote:
>> On Sun, 12 Aug 2018 02:55:31 +, Vincent Pelletier
>>  wrote:
>>> Aug 12 04:44:53 boke kernel: [   64.737069] BUG: KASAN: use-after-free in 
>>> iscsi_target_login_sess_out.cold.11+0x58/0x123 [iscsi_target_mod]
>>> Aug 12 04:44:53 boke kernel: [   64.771148] BUG: KASAN: double-free or 
>>> invalid-free in iscsi_target_login_sess_out.cold.11+0x103/0x123 
>>> [iscsi_target_mod]
>>
>> If I'm reading the code correctly, the double-free would be
>> iscsi_login_init_conn and iscsi_target_login_sess_out both calling
>> kfree(conn->conn_ops), with the latter called by
>> __iscsi_target_login_thread precisely when the former fails (returns
>> NULL after freeing).
>>
> 
> I think I fixed that with this patch:
> 
> https://www.spinics.net/lists/target-devel/msg17018.html
> 
> It fixes a mix of problems double free of the ops, session and reference
> after free.

Ignore this. I see you said conn. My patch fixed basically the same
issue but with the session.

Re: BUG in slab_free after iSCSI login timeout

[Mike Christie]

On 08/11/2018 10:51 PM, Vincent Pelletier wrote:
> On Sun, 12 Aug 2018 02:55:31 +, Vincent Pelletier
>  wrote:
>> Aug 12 04:44:53 boke kernel: [   64.737069] BUG: KASAN: use-after-free in 
>> iscsi_target_login_sess_out.cold.11+0x58/0x123 [iscsi_target_mod]
>> Aug 12 04:44:53 boke kernel: [   64.771148] BUG: KASAN: double-free or 
>> invalid-free in iscsi_target_login_sess_out.cold.11+0x103/0x123 
>> [iscsi_target_mod]
> 
> If I'm reading the code correctly, the double-free would be
> iscsi_login_init_conn and iscsi_target_login_sess_out both calling
> kfree(conn->conn_ops), with the latter called by
> __iscsi_target_login_thread precisely when the former fails (returns
> NULL after freeing).
> 

I think I fixed that with this patch:

https://www.spinics.net/lists/target-devel/msg17018.html

It fixes a mix of problems double free of the ops, session and reference
after free.

Re: [PATCH RESEND v2] target: move the rx hdr buffer out of the stack

[Mike Christie]

On 08/03/2018 03:48 AM, Maurizio Lombardi wrote:
> When HeaderDigest is enabled on aarch64 machines, target triggers
> a lot of failed crc checksum errors:
> 
> example of output in dmesg:
> [  154.495031] HeaderDigest CRC32C failed, received 0x60571c8d, computed 
> 0x288c3ab9
> [  154.583821] Got unknown iSCSI OpCode: 0xff
> [  162.979857] HeaderDigest CRC32C failed, received 0x0712e57c, computed 
> 0x288c3ab9
> ...
> 
> The problem is that the iscsit_get_rx_pdu() function uses
> a stack buffer as input for the scatterlist crypto library.
> This should be avoided on kernels >= 4.9 because stack buffers
> may not be directly mappable to struct page when the
> CONFIG_VMAP_STACK option is enabled.
> 
> This patch modifies the code so the buffer will be allocated on the heap
> and adds a pointer to it in the iscsi_conn structure.
> 
> v2: allocate conn_rx_buf in iscsi_target_login_thread()
> and fix a memory leak
> 
> Signed-off-by: Maurizio Lombardi 
> ---
>  drivers/target/iscsi/iscsi_target.c   |  6 +-
>  drivers/target/iscsi/iscsi_target_login.c | 15 +++
>  include/target/iscsi/iscsi_target_core.h  |  2 ++
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target.c 
> b/drivers/target/iscsi/iscsi_target.c
> index 8e22379..149fa91 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -3913,10 +3913,12 @@ static bool iscsi_target_check_conn_state(struct 
> iscsi_conn *conn)
>  static void iscsit_get_rx_pdu(struct iscsi_conn *conn)
>  {
>   int ret;
> - u8 buffer[ISCSI_HDR_LEN], opcode;
> + u8 *buffer, opcode;
>   u32 checksum = 0, digest = 0;
>   struct kvec iov;
>  
> + buffer = conn->conn_rx_buf;

If the buffer is only used in the loop below why does it need to be
allocated in __iscsi_target_login_thread?

It just seems like the error handling and freeing of that buffer would
be more simple if done here.

[PATCH 0/8] target: a misc lock fix and some cleanup

[Mike Christie]

The following patches were made over Martin's for-next branch. They are
the patches from my session RFC patchset that were reviewed by Bart and
Christoph and are fixes/cleanups that I think are ok changes in general.

[PATCH 1/8] target: fix __transport_register_session locking

[Mike Christie]

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.

This has __transport_register_session use the save/restore call.

Signed-off-by: Mike Christie 
Reviewed-by: Bart Van Assche 
Reviewed-by: Christoph Hellwig 
---
 drivers/target/target_core_transport.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 7261561..b419d4f 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -347,6 +347,7 @@ void __transport_register_session(
 {
const struct target_core_fabric_ops *tfo = se_tpg->se_tpg_tfo;
unsigned char buf[PR_REG_ISID_LEN];
+   unsigned long flags;
 
se_sess->se_tpg = se_tpg;
se_sess->fabric_sess_ptr = fabric_sess_ptr;
@@ -383,7 +384,7 @@ void __transport_register_session(
se_sess->sess_bin_isid = get_unaligned_be64([0]);
}
 
-   spin_lock_irq(_nacl->nacl_sess_lock);
+   spin_lock_irqsave(_nacl->nacl_sess_lock, flags);
/*
 * The se_nacl->nacl_sess pointer will be set to the
 * last active I_T Nexus for each struct se_node_acl.
@@ -392,7 +393,7 @@ void __transport_register_session(
 
list_add_tail(_sess->sess_acl_list,
  _nacl->acl_sess_list);
-   spin_unlock_irq(_nacl->nacl_sess_lock);
+   spin_unlock_irqrestore(_nacl->nacl_sess_lock, flags);
}
list_add_tail(_sess->sess_list, _tpg->tpg_sess_list);
 
-- 
2.7.2

[PATCH 5/8] target: add session removal function

[Mike Christie]

This adds a function to remove a session which should be used by drivers
that use target_setup_session. The next patches will convert the target
drivers to use this new function.

Signed-off-by: Mike Christie 
Reviewed-by: Bart Van Assche 
Reviewed-by: Christoph Hellwig 
Cc: Chris Boot 
Cc: Bryant G. Ly 
Cc: Michael Cyr 
Cc: 
Cc: Johannes Thumshirn 
Cc: Felipe Balbi 
Cc: Sebastian Andrzej Siewior 
Cc: Andrzej Pietrasiewicz 
Cc: Michael S. Tsirkin 
Cc: Juergen Gross 

---
 drivers/target/target_core_transport.c | 7 +++
 include/target/target_core_fabric.h| 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 57fadec..86c0156 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -616,6 +616,13 @@ void transport_deregister_session(struct se_session 
*se_sess)
 }
 EXPORT_SYMBOL(transport_deregister_session);
 
+void target_remove_session(struct se_session *se_sess)
+{
+   transport_deregister_session_configfs(se_sess);
+   transport_deregister_session(se_sess);
+}
+EXPORT_SYMBOL(target_remove_session);
+
 static void target_remove_from_state_list(struct se_cmd *cmd)
 {
struct se_device *dev = cmd->se_dev;
diff --git a/include/target/target_core_fabric.h 
b/include/target/target_core_fabric.h
index fbcc9a4..f4147b3 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -114,6 +114,7 @@ struct se_session *target_setup_session(struct 
se_portal_group *,
const char *, void *,
int (*callback)(struct se_portal_group *,
struct se_session *, void *));
+void target_remove_session(struct se_session *);
 
 void transport_init_session(struct se_session *);
 struct se_session *transport_alloc_session(enum target_prot_op);
-- 
2.7.2

[PATCH 6/8] target: srp, vscsi, sbp, qla: use target_remove_session

[Mike Christie]

This converts the drivers that called transport_deregister_session_configfs
and then immediately called transport_deregister_session to use
target_remove_session.

Signed-off-by: Mike Christie 
Reviewed-by: Bart Van Assche 
Reviewed-by: Christoph Hellwig 
Cc: Chris Boot 
Cc: Bryant G. Ly 
Cc: Michael Cyr 
Cc: 
---
 drivers/infiniband/ulp/srpt/ib_srpt.c| 3 +--
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 3 +--
 drivers/scsi/qla2xxx/tcm_qla2xxx.c   | 3 +--
 drivers/target/sbp/sbp_target.c  | 3 +--
 4 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 21435a7..1ae638b 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2029,8 +2029,7 @@ static void srpt_release_channel_work(struct work_struct 
*w)
target_sess_cmd_list_set_waiting(se_sess);
target_wait_for_sess_cmds(se_sess);
 
-   transport_deregister_session_configfs(se_sess);
-   transport_deregister_session(se_sess);
+   target_remove_session(se_sess);
ch->sess = NULL;
 
if (ch->using_rdma_cm)
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index 1bdf937..fac3773 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -2267,8 +2267,7 @@ static int ibmvscsis_drop_nexus(struct ibmvscsis_tport 
*tport)
 * Release the SCSI I_T Nexus to the emulated ibmvscsis Target Port
 */
target_wait_for_sess_cmds(se_sess);
-   transport_deregister_session_configfs(se_sess);
-   transport_deregister_session(se_sess);
+   target_remove_session(se_sess);
tport->ibmv_nexus = NULL;
kfree(nexus);
 
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index b9ce4e7..f71ec94 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1461,8 +1461,7 @@ static void tcm_qla2xxx_free_session(struct fc_port *sess)
}
target_wait_for_sess_cmds(se_sess);
 
-   transport_deregister_session_configfs(sess->se_sess);
-   transport_deregister_session(sess->se_sess);
+   target_remove_session(se_sess);
 }
 
 static int tcm_qla2xxx_session_cb(struct se_portal_group *se_tpg,
diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index c4369b5..3d10189 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -235,8 +235,7 @@ static void sbp_session_release(struct sbp_session *sess, 
bool cancel_work)
if (cancel_work)
cancel_delayed_work_sync(>maint_work);
 
-   transport_deregister_session_configfs(sess->se_sess);
-   transport_deregister_session(sess->se_sess);
+   target_remove_session(sess->se_sess);
 
if (sess->card)
fw_card_put(sess->card);
-- 
2.7.2

[PATCH 8/8] target: loop, usb, vhost, xen: use target_remove_session

[Mike Christie]

This converts drivers that were only calling transport_deregister_session
to use target_remove_session. The calling of
transport_deregister_session_configfs via target_remove_session for these
types of drivers is ok, because they were not exporting info from fields
like sess_acl_list, sess->se_tpg and sess->fabric_sess_ptr from configfs
accessible functions, so they will see no difference.

Signed-off-by: Mike Christie 
Reviewed-by: Bart Van Assche 
Reviewed-by: Christoph Hellwig 
Cc: Felipe Balbi 
Cc: Sebastian Andrzej Siewior 
Cc: Andrzej Pietrasiewicz 
Cc: Michael S. Tsirkin 
Cc: Juergen Gross 
---
 drivers/target/loopback/tcm_loop.c  | 2 +-
 drivers/usb/gadget/function/f_tcm.c | 2 +-
 drivers/vhost/scsi.c| 2 +-
 drivers/xen/xen-scsiback.c  | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c 
b/drivers/target/loopback/tcm_loop.c
index 54b5b94..bc8918f 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -805,7 +805,7 @@ static int tcm_loop_drop_nexus(
/*
 * Release the SCSI I_T Nexus to the emulated Target Port
 */
-   transport_deregister_session(tl_nexus->se_sess);
+   target_remove_session(se_sess);
tpg->tl_nexus = NULL;
kfree(tl_nexus);
return 0;
diff --git a/drivers/usb/gadget/function/f_tcm.c 
b/drivers/usb/gadget/function/f_tcm.c
index ba7c5f6..106988a 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1638,7 +1638,7 @@ static int tcm_usbg_drop_nexus(struct usbg_tpg *tpg)
/*
 * Release the SCSI I_T Nexus to the emulated vHost Target Port
 */
-   transport_deregister_session(tv_nexus->tvn_se_sess);
+   target_remove_session(se_sess);
tpg->tpg_nexus = NULL;
 
kfree(tv_nexus);
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index e936884..76f8d64 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1797,7 +1797,7 @@ static int vhost_scsi_drop_nexus(struct vhost_scsi_tpg 
*tpg)
/*
 * Release the SCSI I_T Nexus to the emulated vhost Target Port
 */
-   transport_deregister_session(tv_nexus->tvn_se_sess);
+   target_remove_session(se_sess);
tpg->tpg_nexus = NULL;
mutex_unlock(>tv_tpg_mutex);
 
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 2ffc7ed..2aadabd 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -1576,7 +1576,7 @@ static int scsiback_drop_nexus(struct scsiback_tpg *tpg)
/*
 * Release the SCSI I_T Nexus to the emulated xen-pvscsi Target Port
 */
-   transport_deregister_session(tv_nexus->tvn_se_sess);
+   target_remove_session(se_sess);
tpg->tpg_nexus = NULL;
mutex_unlock(>tv_tpg_mutex);
 
-- 
2.7.2

[PATCH 7/8] tcm_fc: use target_remove_session

[Mike Christie]

This converts tcm_fc to use target_remove_session

tcm_fc was calling transport_deregister_session_configfs then calling
transport_deregister_session when commands have completed. It should be
ok for it to call transport_deregister_session_configfs later via
target_remove_session because transport_deregister_session_configfs
only prevents access from configfs via tpg removal and its call to the
close_session callback for that driver, and this is already protected by
the ft_lport_lock and its port lookup handling.

Signed-off-by: Mike Christie 
Reviewed-by: Bart Van Assche 
Reviewed-by: Christoph Hellwig 
Cc: Johannes Thumshirn 
---
 drivers/target/tcm_fc/tfc_sess.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/target/tcm_fc/tfc_sess.c b/drivers/target/tcm_fc/tfc_sess.c
index efd7d5e..6d4adf5 100644
--- a/drivers/target/tcm_fc/tfc_sess.c
+++ b/drivers/target/tcm_fc/tfc_sess.c
@@ -287,7 +287,6 @@ static struct ft_sess *ft_sess_delete(struct ft_tport 
*tport, u32 port_id)
 
 static void ft_close_sess(struct ft_sess *sess)
 {
-   transport_deregister_session_configfs(sess->se_sess);
target_sess_cmd_list_set_waiting(sess->se_sess);
target_wait_for_sess_cmds(sess->se_sess);
ft_sess_put(sess);
@@ -448,7 +447,7 @@ static void ft_sess_free(struct kref *kref)
 {
struct ft_sess *sess = container_of(kref, struct ft_sess, kref);
 
-   transport_deregister_session(sess->se_sess);
+   target_remove_session(sess->se_sess);
kfree_rcu(sess, rcu);
 }
 
-- 
2.7.2

[PATCH 4/8] target: rename target_alloc_session

[Mike Christie]

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 much more. It
allocates the session, sets up the nacl and registers 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.

iscsi will continue to be the odd driver.

Signed-off-by: Mike Christie 
Reviewed-by: Bart Van Assche 
Reviewed-by: Christoph Hellwig 
Cc: Chris Boot 
Cc: Bryant G. Ly 
Cc: Michael Cyr 
Cc: 
Cc: Johannes Thumshirn 
Cc: Felipe Balbi 
Cc: Sebastian Andrzej Siewior 
Cc: Andrzej Pietrasiewicz 
Cc: Michael S. Tsirkin 
Cc: Juergen Gross 

---
 drivers/infiniband/ulp/srpt/ib_srpt.c| 6 +++---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 2 +-
 drivers/scsi/qla2xxx/tcm_qla2xxx.c   | 2 +-
 drivers/target/loopback/tcm_loop.c   | 2 +-
 drivers/target/sbp/sbp_target.c  | 2 +-
 drivers/target/target_core_transport.c   | 4 ++--
 drivers/target/tcm_fc/tfc_sess.c | 2 +-
 drivers/usb/gadget/function/f_tcm.c  | 2 +-
 drivers/vhost/scsi.c | 2 +-
 drivers/xen/xen-scsiback.c   | 2 +-
 include/target/target_core_fabric.h  | 2 +-
 11 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 07b3e1c..21435a7 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2221,16 +2221,16 @@ static int srpt_cm_req_recv(struct srpt_device *const 
sdev,
pr_debug("registering session %s\n", ch->sess_name);
 
if (sport->port_guid_tpg.se_tpg_wwn)
-   ch->sess = target_alloc_session(>port_guid_tpg, 0, 0,
+   ch->sess = target_setup_session(>port_guid_tpg, 0, 0,
TARGET_PROT_NORMAL,
ch->sess_name, ch, NULL);
if (sport->port_gid_tpg.se_tpg_wwn && IS_ERR_OR_NULL(ch->sess))
-   ch->sess = target_alloc_session(>port_gid_tpg, 0, 0,
+   ch->sess = target_setup_session(>port_gid_tpg, 0, 0,
TARGET_PROT_NORMAL, i_port_id, ch,
NULL);
/* Retry without leading "0x" */
if (sport->port_gid_tpg.se_tpg_wwn && IS_ERR_OR_NULL(ch->sess))
-   ch->sess = target_alloc_session(>port_gid_tpg, 0, 0,
+   ch->sess = target_setup_session(>port_gid_tpg, 0, 0,
TARGET_PROT_NORMAL,
i_port_id + 2, ch, NULL);
if (IS_ERR_OR_NULL(ch->sess)) {
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index fdda04e..1bdf937 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -2233,7 +2233,7 @@ static int ibmvscsis_make_nexus(struct ibmvscsis_tport 
*tport)
return -ENOMEM;
}
 
-   nexus->se_sess = target_alloc_session(>se_tpg, 0, 0,
+   nexus->se_sess = target_setup_session(>se_tpg, 0, 0,
  TARGET_PROT_NORMAL, name, nexus,
  NULL);
if (IS_ERR(nexus->se_sess)) {
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index cfb5d60..b9ce4e7 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1539,7 +1539,7 @@ static int tcm_qla2xxx_check_initiator_node_acl(
 * Locate our struct se_node_acl either from an explict NodeACL created
 * via ConfigFS, or via running in TPG demo mode.
 */
-   se_sess = target_alloc_session(>se_tpg, num_tags,
+   se_sess = target_setup_session(>se_tpg, num_tags,
   sizeof(struct qla_tgt_cmd),
   TARGET_PROT_ALL, port_name,
   qlat_sess, tcm_qla2xxx_session_cb);
diff --git a/drivers/target/loopback/tcm_loop.c 
b/drivers/target/loopback/tcm_loop.c
index b2e7ff5..54b5b94 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -765,7 +765,7 @@ static int tcm_loop_make_nexus(
if (!tl_nexus)
return -ENOMEM;
 
-   tl_nexus->se_sess = target_alloc_session(_tpg->tl_se_tpg, 0, 0,
+   tl_nexus->se_sess = target_setup_session(_tpg->tl_se_tpg, 0, 0,
TARGET_PROT_DIN_PASS | 
TARGET_PROT_DOUT_PASS,
name, tl_nexus, tcm_loop_alloc_

[PATCH 2/8] iscsi target: have iscsit_start_nopin_timer call __iscsit_start_nopin_timer

[Mike Christie]

Just have iscsit_start_nopin_timer grab the lock and call
__iscsit_start_nopin_timer.

Signed-off-by: Mike Christie 
Reviewed-by: Christoph Hellwig 
---
 drivers/target/iscsi/iscsi_target_util.c | 20 +---
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_util.c 
b/drivers/target/iscsi/iscsi_target_util.c
index 8cfcf90..49be1e4 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -1053,26 +1053,8 @@ void __iscsit_start_nopin_timer(struct iscsi_conn *conn)
 
 void iscsit_start_nopin_timer(struct iscsi_conn *conn)
 {
-   struct iscsi_session *sess = conn->sess;
-   struct iscsi_node_attrib *na = iscsit_tpg_get_node_attrib(sess);
-   /*
-* NOPIN timeout is disabled..
-*/
-   if (!na->nopin_timeout)
-   return;
-
spin_lock_bh(>nopin_timer_lock);
-   if (conn->nopin_timer_flags & ISCSI_TF_RUNNING) {
-   spin_unlock_bh(>nopin_timer_lock);
-   return;
-   }
-
-   conn->nopin_timer_flags &= ~ISCSI_TF_STOP;
-   conn->nopin_timer_flags |= ISCSI_TF_RUNNING;
-   mod_timer(>nopin_timer, jiffies + na->nopin_timeout * HZ);
-
-   pr_debug("Started NOPIN Timer on CID: %d at %u second"
-   " interval\n", conn->cid, na->nopin_timeout);
+   __iscsit_start_nopin_timer(conn);
spin_unlock_bh(>nopin_timer_lock);
 }
 
-- 
2.7.2

Re: [PATCH 11/15] target: export initiator port values for all sessions

[Mike Christie]

On 08/01/2018 11:44 AM, Mike Christie wrote:
> 1. Create a file named "acl" in the session's dir.
> 2. For dynamic_node_acl=0 the acl file will return a empty string or

Miswrote this.

Above should be dynamic_node_acl=1

> "generate_node_acls=1" or "demo mode enabled".
> 3. For dynamic_node_acl=1 the acl file will return

This should be dynamic_node_acl=0

> se_node_acl->initiatorname which is the name of the acl in
> /tpgt_$N/acls/.
> 

Re: [PATCH 11/15] target: export initiator port values for all sessions

[Mike Christie]

On 07/19/2018 12:07 PM, Bart Van Assche wrote:
> 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?

Ah sorry, I think I made up that term. I was referring to when
se_node_acl->dynamic_node_acl=1, so generate_node_acls=1 and demo mode
and dynamic_node_acl=1 is the same state.

>  
>> 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.
> 

Hey Bart,

I did patches to add symlinks. There is one problem that this will break
userspace though.

The userspace tools assume that they can tear down a tpgt while sessions
are running because currently a rmdir on the acl will force running
sessions to be shutdown. For example, a FC or iscsi initiator can be
logged into the target and userspace currently does something like this
simplified sequence:

for each object under a tpgt
ulink luns
rmdir luns
rmdir acls
rmdir tpt

The problem is that because the session has a symlink to the acl and
configfs has done a config_item_get on the acl config_item to make sure
it does not get freed while in use the "rmdir acl" will now fail.

I agree knowing the acl info is useful, so how about the following:

1. Create a file named "acl" in the session's dir.
2. For dynamic_node_acl=0 the acl file will return a empty string or
"generate_node_acls=1" or "demo mode enabled".
3. For dynamic_node_acl=1 the acl file will return
se_node_acl->initiatorname which is the name of the acl in
/tpgt_$N/acls/.

[PATCH 9/9] tcmu: use u64 for dev_size

[Mike Christie]

We use unsigned long, size_t and u64 for dev_size. This has us
standardize on u64.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 21 -
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index cfe4f4b..b34179a 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -137,7 +137,7 @@ struct tcmu_dev {
struct inode *inode;
 
struct tcmu_mailbox *mb_addr;
-   size_t dev_size;
+   uint64_t dev_size;
u32 cmdr_size;
u32 cmdr_last_cleaned;
/* Offset of data area from start of mb */
@@ -2016,7 +2016,7 @@ enum {
 
 static match_table_t tokens = {
{Opt_dev_config, "dev_config=%s"},
-   {Opt_dev_size, "dev_size=%u"},
+   {Opt_dev_size, "dev_size=%s"},
{Opt_hw_block_size, "hw_block_size=%d"},
{Opt_hw_max_sectors, "hw_max_sectors=%d"},
{Opt_nl_reply_supported, "nl_reply_supported=%d"},
@@ -2083,7 +2083,7 @@ static ssize_t tcmu_set_configfs_dev_params(struct 
se_device *dev,
const char *page, ssize_t count)
 {
struct tcmu_dev *udev = TCMU_DEV(dev);
-   char *orig, *ptr, *opts, *arg_p;
+   char *orig, *ptr, *opts;
substring_t args[MAX_OPT_ARGS];
int ret = 0, token;
 
@@ -2108,15 +2108,10 @@ static ssize_t tcmu_set_configfs_dev_params(struct 
se_device *dev,
pr_debug("TCMU: Referencing Path: %s\n", 
udev->dev_config);
break;
case Opt_dev_size:
-   arg_p = match_strdup([0]);
-   if (!arg_p) {
-   ret = -ENOMEM;
-   break;
-   }
-   ret = kstrtoul(arg_p, 0, (unsigned long *) 
>dev_size);
-   kfree(arg_p);
+   ret = match_u64([0], >dev_size);
if (ret < 0)
-   pr_err("kstrtoul() failed for dev_size=\n");
+   pr_err("match_u64() failed for dev_size=. Error 
%d.\n",
+  ret);
break;
case Opt_hw_block_size:
ret = tcmu_set_dev_attrib([0],
@@ -2154,7 +2149,7 @@ static ssize_t tcmu_show_configfs_dev_params(struct 
se_device *dev, char *b)
 
bl = sprintf(b + bl, "Config: %s ",
 udev->dev_config[0] ? udev->dev_config : "NULL");
-   bl += sprintf(b + bl, "Size: %zu ", udev->dev_size);
+   bl += sprintf(b + bl, "Size: %llu ", udev->dev_size);
bl += sprintf(b + bl, "MaxDataAreaMB: %u\n",
  TCMU_BLOCKS_TO_MBS(udev->max_blocks));
 
@@ -2323,7 +2318,7 @@ static ssize_t tcmu_dev_size_show(struct config_item 
*item, char *page)
struct se_dev_attrib, da_group);
struct tcmu_dev *udev = TCMU_DEV(da->da_dev);
 
-   return snprintf(page, PAGE_SIZE, "%zu\n", udev->dev_size);
+   return snprintf(page, PAGE_SIZE, "%llu\n", udev->dev_size);
 }
 
 static int tcmu_send_dev_size_event(struct tcmu_dev *udev, u64 size)
-- 
1.8.3.1

[PATCH 5/9] tcmu: check if dev is configured before block/reset

[Mike Christie]

Do not allow userspace to block or reset the ring until the device has
been configured. This will prevent the bug where userspace can write to
those files and access mb_addr before it has been setup.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index bc8121f..d6b4022 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -2480,6 +2480,11 @@ static ssize_t tcmu_block_dev_store(struct config_item 
*item, const char *page,
u8 val;
int ret;
 
+   if (!target_dev_configured(>se_dev)) {
+   pr_err("Device is not configured.\n");
+   return -EINVAL;
+   }
+
ret = kstrtou8(page, 0, );
if (ret < 0)
return ret;
@@ -2507,6 +2512,11 @@ static ssize_t tcmu_reset_ring_store(struct config_item 
*item, const char *page,
u8 val;
int ret;
 
+   if (!target_dev_configured(>se_dev)) {
+   pr_err("Device is not configured.\n");
+   return -EINVAL;
+   }
+
ret = kstrtou8(page, 0, );
if (ret < 0)
return ret;
-- 
1.8.3.1

[PATCH 4/9] tcmu: use lio core se_device configuration helper

[Mike Christie]

Use the lio core helper to check if the device is configured.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index b010ed7..bc8121f 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1908,11 +1908,6 @@ static int tcmu_configure_device(struct se_device *dev)
return ret;
 }
 
-static bool tcmu_dev_configured(struct tcmu_dev *udev)
-{
-   return udev->uio_info.uio_dev ? true : false;
-}
-
 static void tcmu_free_device(struct se_device *dev)
 {
struct tcmu_dev *udev = TCMU_DEV(dev);
@@ -2305,7 +2300,7 @@ static ssize_t tcmu_dev_config_store(struct config_item 
*item, const char *page,
return -EINVAL;
 
/* Check if device has been configured before */
-   if (tcmu_dev_configured(udev)) {
+   if (target_dev_configured(>se_dev)) {
ret = tcmu_send_dev_config_event(udev, page);
if (ret) {
pr_err("Unable to reconfigure device\n");
@@ -2367,7 +2362,7 @@ static ssize_t tcmu_dev_size_store(struct config_item 
*item, const char *page,
return ret;
 
/* Check if device has been configured before */
-   if (tcmu_dev_configured(udev)) {
+   if (target_dev_configured(>se_dev)) {
ret = tcmu_send_dev_size_event(udev, val);
if (ret) {
pr_err("Unable to reconfigure device\n");
@@ -2449,7 +2444,7 @@ static ssize_t tcmu_emulate_write_cache_store(struct 
config_item *item,
return ret;
 
/* Check if device has been configured before */
-   if (tcmu_dev_configured(udev)) {
+   if (target_dev_configured(>se_dev)) {
ret = tcmu_send_emulate_write_cache(udev, val);
if (ret) {
pr_err("Unable to reconfigure device\n");
-- 
1.8.3.1

[PATCH 8/9] tcmu: use match_int for dev params

[Mike Christie]

Instead of doing strdup and kstrto* just use match_int for dev params.

It will be ok to use int instead of unsigned long in tcmu_set_dev_attrib
because that is only being used for max sectors and block size and the
supported values for them are well under the max possible integer value.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 37 ++---
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 969ccba..cfe4f4b 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -2017,8 +2017,8 @@ enum {
 static match_table_t tokens = {
{Opt_dev_config, "dev_config=%s"},
{Opt_dev_size, "dev_size=%u"},
-   {Opt_hw_block_size, "hw_block_size=%u"},
-   {Opt_hw_max_sectors, "hw_max_sectors=%u"},
+   {Opt_hw_block_size, "hw_block_size=%d"},
+   {Opt_hw_max_sectors, "hw_max_sectors=%d"},
{Opt_nl_reply_supported, "nl_reply_supported=%d"},
{Opt_max_data_area_mb, "max_data_area_mb=%d"},
{Opt_err, NULL}
@@ -2026,25 +2026,21 @@ enum {
 
 static int tcmu_set_dev_attrib(substring_t *arg, u32 *dev_attrib)
 {
-   unsigned long tmp_ul;
-   char *arg_p;
-   int ret;
-
-   arg_p = match_strdup(arg);
-   if (!arg_p)
-   return -ENOMEM;
+   int val, ret;
 
-   ret = kstrtoul(arg_p, 0, _ul);
-   kfree(arg_p);
+   ret = match_int(arg, );
if (ret < 0) {
-   pr_err("kstrtoul() failed for dev attrib\n");
+   pr_err("match_int() failed for dev attrib. Error %d.\n",
+  ret);
return ret;
}
-   if (!tmp_ul) {
-   pr_err("dev attrib must be nonzero\n");
+
+   if (val <= 0) {
+   pr_err("Invalid dev attrib value %d. Must be greater than 
zero.\n",
+  val);
return -EINVAL;
}
-   *dev_attrib = tmp_ul;
+   *dev_attrib = val;
return 0;
 }
 
@@ -2131,15 +2127,10 @@ static ssize_t tcmu_set_configfs_dev_params(struct 
se_device *dev,
&(dev->dev_attrib.hw_max_sectors));
break;
case Opt_nl_reply_supported:
-   arg_p = match_strdup([0]);
-   if (!arg_p) {
-   ret = -ENOMEM;
-   break;
-   }
-   ret = kstrtoint(arg_p, 0, >nl_reply_supported);
-   kfree(arg_p);
+   ret = match_int([0], >nl_reply_supported);
if (ret < 0)
-   pr_err("kstrtoint() failed for 
nl_reply_supported=\n");
+   pr_err("match_int() failed for 
nl_reply_supported=. Error %d.\n",
+  ret);
break;
case Opt_max_data_area_mb:
ret = tcmu_set_max_blocks_param(udev, [0]);
-- 
1.8.3.1

[PATCH 7/9] tcmu: do not set max_blocks if data_bitmap has been setup

[Mike Christie]

This patch prevents a bug where data_bitmap is allocated in
tcmu_configure_device, userspace changes the max_blocks setting, the
device is mapped to a LUN, then we try to access the data_bitmap based
on the new max_blocks limit which may now be out of range.

To prevent this, we just check if data_bitmap has been setup. If it has
then we fail the max_blocks update operation.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 73 +--
 1 file changed, 40 insertions(+), 33 deletions(-)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 31cfe83..969ccba 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1811,9 +1811,11 @@ static int tcmu_configure_device(struct se_device *dev)
 
info = >uio_info;
 
+   mutex_lock(>cmdr_lock);
udev->data_bitmap = kcalloc(BITS_TO_LONGS(udev->max_blocks),
sizeof(unsigned long),
GFP_KERNEL);
+   mutex_unlock(>cmdr_lock);
if (!udev->data_bitmap) {
ret = -ENOMEM;
goto err_bitmap_alloc;
@@ -2018,7 +2020,7 @@ enum {
{Opt_hw_block_size, "hw_block_size=%u"},
{Opt_hw_max_sectors, "hw_max_sectors=%u"},
{Opt_nl_reply_supported, "nl_reply_supported=%d"},
-   {Opt_max_data_area_mb, "max_data_area_mb=%u"},
+   {Opt_max_data_area_mb, "max_data_area_mb=%d"},
{Opt_err, NULL}
 };
 
@@ -2046,13 +2048,48 @@ static int tcmu_set_dev_attrib(substring_t *arg, u32 
*dev_attrib)
return 0;
 }
 
+static int tcmu_set_max_blocks_param(struct tcmu_dev *udev, substring_t *arg)
+{
+   int val, ret;
+
+   ret = match_int(arg, );
+   if (ret < 0) {
+   pr_err("match_int() failed for max_data_area_mb=. Error %d.\n",
+  ret);
+   return ret;
+   }
+
+   if (val <= 0) {
+   pr_err("Invalid max_data_area %d.\n", val);
+   return -EINVAL;
+   }
+
+   mutex_lock(>cmdr_lock);
+   if (udev->data_bitmap) {
+   pr_err("Cannot set max_data_area_mb after it has been 
enabled.\n");
+   ret = -EINVAL;
+   goto unlock;
+   }
+
+   udev->max_blocks = TCMU_MBS_TO_BLOCKS(val);
+   if (udev->max_blocks > tcmu_global_max_blocks) {
+   pr_err("%d is too large. Adjusting max_data_area_mb to global 
limit of %u\n",
+  val, TCMU_BLOCKS_TO_MBS(tcmu_global_max_blocks));
+   udev->max_blocks = tcmu_global_max_blocks;
+   }
+
+unlock:
+   mutex_unlock(>cmdr_lock);
+   return ret;
+}
+
 static ssize_t tcmu_set_configfs_dev_params(struct se_device *dev,
const char *page, ssize_t count)
 {
struct tcmu_dev *udev = TCMU_DEV(dev);
char *orig, *ptr, *opts, *arg_p;
substring_t args[MAX_OPT_ARGS];
-   int ret = 0, token, tmpval;
+   int ret = 0, token;
 
opts = kstrdup(page, GFP_KERNEL);
if (!opts)
@@ -2105,37 +2142,7 @@ static ssize_t tcmu_set_configfs_dev_params(struct 
se_device *dev,
pr_err("kstrtoint() failed for 
nl_reply_supported=\n");
break;
case Opt_max_data_area_mb:
-   if (dev->export_count) {
-   pr_err("Unable to set max_data_area_mb while 
exports exist\n");
-   ret = -EINVAL;
-   break;
-   }
-
-   arg_p = match_strdup([0]);
-   if (!arg_p) {
-   ret = -ENOMEM;
-   break;
-   }
-   ret = kstrtoint(arg_p, 0, );
-   kfree(arg_p);
-   if (ret < 0) {
-   pr_err("kstrtoint() failed for 
max_data_area_mb=\n");
-   break;
-   }
-
-   if (tmpval <= 0) {
-   pr_err("Invalid max_data_area %d\n", tmpval);
-   ret = -EINVAL;
-   break;
-   }
-
-   udev->max_blocks = TCMU_MBS_TO_BLOCKS(tmpval);
-   if (udev->max_blocks > tcmu_global_max_blocks) {
-   pr_err("%d is too large. Adjusting 
max_data_area_mb to global limit of %u\n",
-  tmpval,
-  
TCMU_BLOCKS_TO_MBS(tcmu_global_max_blocks));
-   udev->max_blocks = tcmu_global_max_blocks;
-   }
+   ret = tcmu_set_max_blocks_param(udev, [0]);
break;
default:
break;
-- 
1.8.3.1

[PATCH 0/9] tcmu: configuration fixes and cleanups

[Mike Christie]

The following patches were made over Martin's for-next branch.

The first patch fixes a locking bug in the command setup path. The rest
of the patches fix several bugs and cleanup the setup and configuration
code paths.

[PATCH 2/9] tcmu: initialize list head

[Mike Christie]

Use INIT_LIST_HEAD to initialize node list head.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index fafe65f..b010ed7 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1342,6 +1342,7 @@ static struct se_device *tcmu_alloc_device(struct se_hba 
*hba, const char *name)
udev->max_blocks = DATA_BLOCK_BITS_DEF;
mutex_init(>cmdr_lock);
 
+   INIT_LIST_HEAD(>node);
INIT_LIST_HEAD(>timedout_entry);
INIT_LIST_HEAD(>cmdr_queue);
idr_init(>commands);
-- 
1.8.3.1

[PATCH 6/9] tcmu: unmap if dev is configured

[Mike Christie]

The tcmu dev is added to the list of tcmu devices during configuration.
At this time the tcmu setup has completed, but lio core has not
completed its setup. The device is not yet usable so do not try to unmap
blocks from it

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index d6b4022..31cfe83 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -2581,6 +2581,11 @@ static void find_free_blocks(void)
list_for_each_entry(udev, _udev, node) {
mutex_lock(>cmdr_lock);
 
+   if (!target_dev_configured(>se_dev)) {
+   mutex_unlock(>cmdr_lock);
+   continue;
+   }
+
/* Try to complete the finished commands first */
tcmu_handle_completions(udev);
 
-- 
1.8.3.1

[PATCH 3/9] target: add helper to check if dev is configured

[Mike Christie]

This just adds a helper function to check if a device is configured and
it converts the target users to use it. The next patch will add a
backend module user so those types of modules do not have to know the
lio core details.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_configfs.c| 8 
 drivers/target/target_core_device.c  | 6 +++---
 drivers/target/target_core_fabric_configfs.c | 3 ++-
 include/target/target_core_backend.h | 4 
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_configfs.c 
b/drivers/target/target_core_configfs.c
index 93d3ff3..f6b1549 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -810,7 +810,7 @@ static ssize_t pi_prot_type_store(struct config_item *item,
   dev->transport->name);
return -ENOSYS;
}
-   if (!(dev->dev_flags & DF_CONFIGURED)) {
+   if (!target_dev_configured(dev)) {
pr_err("DIF protection requires device to be configured\n");
return -ENODEV;
}
@@ -859,7 +859,7 @@ static ssize_t pi_prot_format_store(struct config_item 
*item,
   dev->transport->name);
return -ENOSYS;
}
-   if (!(dev->dev_flags & DF_CONFIGURED)) {
+   if (!target_dev_configured(dev)) {
pr_err("DIF protection format requires device to be 
configured\n");
return -ENODEV;
}
@@ -1948,7 +1948,7 @@ static ssize_t target_dev_enable_show(struct config_item 
*item, char *page)
 {
struct se_device *dev = to_device(item);
 
-   return snprintf(page, PAGE_SIZE, "%d\n", !!(dev->dev_flags & 
DF_CONFIGURED));
+   return snprintf(page, PAGE_SIZE, "%d\n", target_dev_configured(dev));
 }
 
 static ssize_t target_dev_enable_store(struct config_item *item,
@@ -2473,7 +2473,7 @@ static ssize_t 
target_tg_pt_gp_alua_access_state_store(struct config_item *item,
" tg_pt_gp ID: %hu\n", tg_pt_gp->tg_pt_gp_valid_id);
return -EINVAL;
}
-   if (!(dev->dev_flags & DF_CONFIGURED)) {
+   if (!target_dev_configured(dev)) {
pr_err("Unable to set alua_access_state while device is"
   " not configured\n");
return -ENODEV;
diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index 73675ee..47b5ef1 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -900,7 +900,7 @@ static int target_devices_idr_iter(int id, void *p, void 
*data)
 * to allow other callers to access partially setup devices,
 * so we skip them here.
 */
-   if (!(dev->dev_flags & DF_CONFIGURED))
+   if (!target_dev_configured(dev))
return 0;
 
iter->prev_item = config_item_get_unless_zero(>dev_group.cg_item);
@@ -940,7 +940,7 @@ int target_configure_device(struct se_device *dev)
struct se_hba *hba = dev->se_hba;
int ret, id;
 
-   if (dev->dev_flags & DF_CONFIGURED) {
+   if (target_dev_configured(dev)) {
pr_err("se_dev->se_dev_ptr already set for storage"
" object\n");
return -EEXIST;
@@ -1045,7 +1045,7 @@ void target_free_device(struct se_device *dev)
 
WARN_ON(!list_empty(>dev_sep_list));
 
-   if (dev->dev_flags & DF_CONFIGURED) {
+   if (target_dev_configured(dev)) {
destroy_workqueue(dev->tmr_wq);
 
dev->transport->destroy_device(dev);
diff --git a/drivers/target/target_core_fabric_configfs.c 
b/drivers/target/target_core_fabric_configfs.c
index 1fa436e..aa2f4f6 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -34,6 +34,7 @@
 #include 
 
 #include 
+#include 
 #include 
 
 #include "target_core_internal.h"
@@ -642,7 +643,7 @@ static int target_fabric_port_link(
}
dev = container_of(to_config_group(se_dev_ci), struct se_device, 
dev_group);
 
-   if (!(dev->dev_flags & DF_CONFIGURED)) {
+   if (!target_dev_configured(dev)) {
pr_err("se_device not configured yet, cannot port link\n");
return -ENODEV;
}
diff --git a/include/target/target_core_backend.h 
b/include/target/target_core_backend.h
index c3ac472..51b6f50 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -111,6 +111,10 @@ sense_reason_t passthrough_parse_cdb(struct se_cmd *cmd,
 bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
   struct request_queue *q);
 
+static inline

[PATCH 1/9] target_core_user: fix double unlock

[Mike Christie]

The caller of queue_cmd_ring grabs and releases the lock, so
the tcmu_setup_cmd_timer failure handling inside queue_cmd_ring
should not call mutex_unlock.

Signed-off-by: Mike Christie 
---
 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 847707a..fafe65f 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1066,7 +1066,6 @@ static sense_reason_t queue_cmd_ring(struct tcmu_cmd 
*tcmu_cmd, int *scsi_err)
   >cmd_timer);
if (ret) {
tcmu_cmd_free_data(tcmu_cmd, tcmu_cmd->dbi_cnt);
-   mutex_unlock(>cmdr_lock);
 
*scsi_err = TCM_OUT_OF_RESOURCES;
return -1;
-- 
1.8.3.1

Re: [PATCH 02/15] target: fix isid copying and comparision

[Mike Christie]

On 07/20/2018 04:08 PM, Mike Christie wrote:
> On 07/19/2018 11:13 AM, Mike Christie wrote:
>> > 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.
> Hey Bart and Christoph,
> 
> Bart, I noticed we basically had what you are requesting but Christoph
> had moved the id code from the fabric drivers to lio core in this commit:
> 
> commit 2650d71e244fb3637b5f58a0080682a8bf9c7091
> Author: Christoph Hellwig 
> Date: Fri May 1 17:47:58 2015 +0200
> 
> target: move transport ID handling to the core
> 
> 
> So what do you guys want to do here? Revert Christoph's patch and go
> back to that style or have lio core do the transport ID processing?

Oh yeah for the latter, we can make it so at least target_core_pr.c does
not have to do any isid conversions. We could just rename sess_bin_isid
to sess_str_isid, store the isid as a string, and then never convert it
between the binary and string format. All the isid tests in
target_core_pr.c would be strcmps.

Re: [PATCH 02/15] target: fix isid copying and comparision

[Mike Christie]

On 07/19/2018 11:13 AM, Mike Christie wrote:
> 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.

Hey Bart and Christoph,

Bart, I noticed we basically had what you are requesting but Christoph
had moved the id code from the fabric drivers to lio core in this commit:

commit 2650d71e244fb3637b5f58a0080682a8bf9c7091
Author: Christoph Hellwig 
Date: Fri May 1 17:47:58 2015 +0200

target: move transport ID handling to the core


So what do you guys want to do here? Revert Christoph's patch and go
back to that style or have lio core do the transport ID processing?


> 
>> of that function is in source file scst/src/scst_targ.c:
>>
>>  res = sess->tgt->tgtt->get_initiator_port_transport_id(
>>  sess->tgt, 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
>>
> 
> --
> 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

[Mike Christie]

On 07/19/2018 07:34 PM, Xiubo Li wrote:
> On 2018/7/19 23:49, Mike Christie wrote:
>> 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?
> Yes, not only the RHEL bug, for the RHEL we have sync the related code
> to the upstream. Before as I remembered I have hit one sudden crash by
> using the SCSI repo code when testing the tcmu-runner PR#402:
> tcmu-runner: add reset netlink support.
> 
> The SCSI repo is :
> git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git
> 
> Since the kdump service was not working for the upstream code in my
> setups that time, so could only get very simple logs. This time I was
> using Prasanna's script and RHEL code with the netlink and UIO crash
> fixing patches to reproduce this. The test script please see the attachment.
> 
>> 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.
> With the tcmu-runner PR#402, when starting tcmu-runner the daemon will do :
> 1, block netlink
> 2, reset netlink events
> 3, block device --> reset ring --> open device --> unblock device

If the device was only created and not yet enabled you will hit the
crash here.

If in your test you kill runner while a enable is in process then this
would happen.

> 4, unblock netlink
> 
> 
> Currently there may has one bug in systemd service, because we hit one
> odd issue by using the test script:
> When executing 'systemctl restart tcmu-runner', this process seems
> waiting one signal from somewhere by stucking itself, if we do nothing
> It will wait for about 6 ~20 minutes(the time comes from our test
> result) to recovery from that. And we can execute 'systemctl restart
> gluster-blockd' in another terminal, which could stop the stuck state
> above immediately.
> 
> [root@localhost ~]# ps -aux | grep -e tcmu -e
> target 
> root  2021  0.0  0.0  0 0 ?S<   15:09   0:00
> [target_completi]   
> root  2386  0.6  0.5 226232 11096 ?Ds   15:11   0:00
> /usr/bin/python /usr/bin/targetctl clear
> root  2400  0.0  0.0 134816  1316 pts/0S+   15:11   0:00
> systemctl restart tcmu-runnerroot  2415 
> 0.0  0.0 112704   988 pts/1R+   15:11   0:00 grep --color=auto -e
> tcmu -e target
> [root@localhost ~]# cat
> /proc/2400/stack  
>
> 
> []
> poll_schedule_timeout+0x55/0xb0   
>   
> 
> []
> do_sys_poll+0x48d/0x590   
>   
> 
> []
> SyS_ppoll+0x1d3/0x1f0 
>   
> 
> []
> system_call_fastpath+0x1c/0x21
>   
> 
> 
> And with IOs going on, when killing the tcmu-runner process, the

Are you just doing a test to kill runner at various times or is your
restart of your service doing this while the targetcli clear operation
is running?

If you re killing it at various times, how do you know you are not
killing it during a enable operation and hitting this bug there?

If you are killing on purpose as part of your restart operation why are
you killing it when the targetcli clear operation needs the backends to
complete their IO as part of the tpg disablement process.


> 'targetctl clear' process will stuck in [1] first. Then we restart the
> tcmu-runner, which will reset ring with accessing the udev->mb_addr,
> currently we still could access the 
> /sys/kernel/config/target/core/user_XX/*. And while the ring reset is
> still going on, the stucked 'targetctl clear' process will be woken up
> and then t

Re: [PATCH 11/15] target: export initiator port values for all sessions

[Mike Christie]

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 11/15] target: export initiator port values for all sessions

[Mike Christie]

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,
>> +   _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

[Mike Christie]

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, >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

[Mike Christie]

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(_out_udevs_lock);
>   if (!list_empty(>timedout_entry))
>   list_del(>timedout_entry);
>   spin_unlock_bh(_out_udevs_lock);
>  
> - /* Upper layer should drain all requests before calling this */
>   mutex_lock(>cmdr_lock);
> + vfree(udev->mb_addr);
> + udev->mb_addr = NULL;
> +
> + /* Upper layer should drain all requests before calling this */
>   idr_for_each_entry(>commands, cmd, i) {
>   if (tcmu_check_and_free_pending_cmd(cmd) != 0)
>   all_expired = false;
> 

Re: [PATCH 04/15] target/iscsi: move session_index to common se_session

[Mike Christie]

On 07/18/2018 07:15 PM, Mike Christie wrote:
> On 07/18/2018 05:19 PM, Bart Van Assche wrote:
>> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
>>> diff --git a/drivers/target/target_core_transport.c 
>>> b/drivers/target/target_core_transport.c
>>> index 75ddbbb..97a1ee5 100644
>>> --- a/drivers/target/target_core_transport.c
>>> +++ b/drivers/target/target_core_transport.c
>>> @@ -55,6 +55,8 @@
>>>  
>>>  static struct workqueue_struct *target_completion_wq;
>>>  static struct kmem_cache *se_sess_cache;
>>> +static DEFINE_SPINLOCK(se_sess_idr_lock);
>>> +static DEFINE_IDR(se_sess_idr);
>>
>> Is it necessary that se_sess_idr_lock and se_sess_idr are global? Could these
>> two data structures be members of the data structure associated with
>> /sys/kernel/config/target/iscsi/$port/$tpg (struct se_portal_group?)?
> 
> For tcmu we have a problem where we pass the scsi commands to userspace
> but then we need to know what I_T nexus it was sent through or what
> target port it was received on.
> 
> I thought I could reuse the sid for tcmu commands where I could embed
> the sid in the tcmu_cmd and then userspace can look up the sid and know
> what session the command came in on. If the device is exported through 2
> tpgs then we need the sid target wide in case sessions on different tpgs
> have the same sid. And, then I thought if you exported the device
> through 2 fabrics then I thought you need it set globally.
> 
> I am still working on that part with Bodo, so I can make it per tpg when
> I resend then do another path to change it later.

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?

Re: [PATCH 11/15] target: export initiator port values for all sessions

[Mike Christie]

On 07/18/2018 06:04 PM, Mike Christie wrote:
> On 07/18/2018 05:41 PM, Bart Van Assche wrote:
>> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
>>> Export the initiator port info in configfs
>>
>> Does configfs support soft links? Can this information be exported as a
>> soft link from the session directory to the struct se_portal_group configfs
>> object?
>>
> 
> If you just needed to export the initiator name or if a single session
> per initiator can be connected to a tpg then it would work ok.
> 
> The problem is for iscsi the scsi initiator port / transport id, is the
> initiator name and isid. The isid is just a 48 bit number and the
> initiator will allocate a new value for every session. So on the
> initiator side if there are multiple nics, then it is common to create a
> session through nic and each session will have the same initiator name
> but different isids. So at some place you need to put multiple files to
> export the different isids or indicate to userspace tools that there is
> more than one session connected to that tpg.
>

Oh wait, I think I know what you mean. Did you want something like this
where the symlink name is the info in the initiator_port file like this:

[tpgt_1]# tree -L 2
.
`-- sessions
`-- 1
|   `-- iqn.2005-03.com.ceph:ini1,i,0x00023d01 -> ../../tpgt_1
 `--2
.   `-- iqn.2005-03.com.ceph:ini1,i,0x00023d02 -> ../../tpgt_1

If that is what you are asking about, I did not get why we want to link
to the tpg object, because we already know the tpg since it is the
parent of the session dir.

Re: [PATCH 02/15] target: fix isid copying and comparision

[Mike Christie]

On 07/18/2018 07:03 PM, Mike Christie wrote:
> On 07/18/2018 05:09 PM, Bart Van Assche wrote:
>> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
>>> The isid is 48 bits, and in hex string format it's 12 bytes.
>>> We are currently copying the 12 byte hex string to a u64
>>> so we can easily compare it, but this has the problem that
>>> only 8 bytes of the 12 bytes are copied.
>>>
>>> The next patches will want to print se_session sess_bin_isid
>>> so this has us use hex2bin to when converting from the hex
>>> sting to the bin value.
>>   ^
>>   string?
>>
>> Which spec defines that an ISID is 48 bits? All I know about SCSI 
>> registrations
> 
> The iscsi spec defines it as 48 bits.
> 
>> 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.

Re: [PATCH 12/15] target: add callout to test a session

[Mike Christie]

On 07/18/2018 05:46 PM, Bart Van Assche wrote:
> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
>> +   int (*test_session)(struct se_session *, u8 timeout);
> 
> Does any of the patches in this series define a test_session callback
> function?

Patch 14 does.

> 
> What is the unit of the timeout parameter? 1/HZ, ms or s?
> 

It is seconds.

I will add comments to document the callout requirements and arguments.

> Thanks,
> 
> Bart.
> 

Re: [PATCH 04/15] target/iscsi: move session_index to common se_session

[Mike Christie]

On 07/18/2018 05:19 PM, Bart Van Assche wrote:
> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
>> diff --git a/drivers/target/target_core_transport.c 
>> b/drivers/target/target_core_transport.c
>> index 75ddbbb..97a1ee5 100644
>> --- a/drivers/target/target_core_transport.c
>> +++ b/drivers/target/target_core_transport.c
>> @@ -55,6 +55,8 @@
>>  
>>  static struct workqueue_struct *target_completion_wq;
>>  static struct kmem_cache *se_sess_cache;
>> +static DEFINE_SPINLOCK(se_sess_idr_lock);
>> +static DEFINE_IDR(se_sess_idr);
> 
> Is it necessary that se_sess_idr_lock and se_sess_idr are global? Could these
> two data structures be members of the data structure associated with
> /sys/kernel/config/target/iscsi/$port/$tpg (struct se_portal_group?)?

For tcmu we have a problem where we pass the scsi commands to userspace
but then we need to know what I_T nexus it was sent through or what
target port it was received on.

I thought I could reuse the sid for tcmu commands where I could embed
the sid in the tcmu_cmd and then userspace can look up the sid and know
what session the command came in on. If the device is exported through 2
tpgs then we need the sid target wide in case sessions on different tpgs
have the same sid. And, then I thought if you exported the device
through 2 fabrics then I thought you need it set globally.

I am still working on that part with Bodo, so I can make it per tpg when
I resend then do another path to change it later.

Re: [PATCH 02/15] target: fix isid copying and comparision

[Mike Christie]

On 07/18/2018 05:09 PM, Bart Van Assche wrote:
> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
>> The isid is 48 bits, and in hex string format it's 12 bytes.
>> We are currently copying the 12 byte hex string to a u64
>> so we can easily compare it, but this has the problem that
>> only 8 bytes of the 12 bytes are copied.
>>
>> The next patches will want to print se_session sess_bin_isid
>> so this has us use hex2bin to when converting from the hex
>> sting to the bin value.
>   ^
>   string?
> 
> Which spec defines that an ISID is 48 bits? All I know about SCSI 
> registrations

The iscsi spec defines it as 48 bits.

> 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.


> 
>>  if (isid != NULL) {
>> -pr_reg->pr_reg_bin_isid = get_unaligned_be64(isid);
>> +if (hex2bin((u8 *)_reg->pr_reg_bin_isid, isid, 6)) {
>> +pr_err("Invalid isid %s\n", isid);
>> +goto free_reg;
>> +}
> 
> Why is it necessary to convert the ISID from hex to binary format? If this
> conversion is necessary, isn't that something that should be handled by the
> iSCSI target driver instead of the target core?
> 

The target drivers get the value as a 48 bit binary value.

The PR code gets it in the string format as describe above.

I had thought the code preferred to store it in the binary format
because it was easier to test than comparing strings or maybe for speed,
so I just fixed that code.

I think we can just keep it in string format in
__transport_register_session then just compare strings in target_core_pr.c.

Re: [PATCH 11/15] target: export initiator port values for all sessions

[Mike Christie]

On 07/18/2018 05:41 PM, Bart Van Assche wrote:
> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote:
>> Export the initiator port info in configfs
> 
> Does configfs support soft links? Can this information be exported as a
> soft link from the session directory to the struct se_portal_group configfs
> object?
> 

If you just needed to export the initiator name or if a single session
per initiator can be connected to a tpg then it would work ok.

The problem is for iscsi the scsi initiator port / transport id, is the
initiator name and isid. The isid is just a 48 bit number and the
initiator will allocate a new value for every session. So on the
initiator side if there are multiple nics, then it is common to create a
session through nic and each session will have the same initiator name
but different isids. So at some place you need to put multiple files to
export the different isids or indicate to userspace tools that there is
more than one session connected to that tpg.

Re: [PATCH 09/15] target: add session dir in configfs

[Mike Christie]

On 07/15/2018 06:16 PM, Mike Christie wrote:
> TODO:
> 1. handle target_fabric_register_session failure in iscsi target.
> 

I fixed this in the updated attached patch. So Bodo and reviewers please
use that instead.
>From 895edd5d20df208c3dfae8d826a3c3fcbd68 Mon Sep 17 00:00:00 2001
From: Mike Christie 
Date: Mon, 16 Jul 2018 13:37:12 -0500
Subject: [PATCH 09/15] target: add session dir in configfs

Drivers like iscsi will dump one of the session's info in
a info file under the acl. The problem with this is that
it only prints one session and only sessions with an acl.

Then there is a dynamic_sessions file which dumps the initiator
name of each session without an acl. The problem with this
is that the info printed is limited to only the node name.
For iscsi we do not get the entire initiator port string
(name + isid). Also all dynamic sessions are printed in one
file so setups that do a target per lun quickly run out of
space.

To handle all these issues, this patch adds a per session
dir under the tpg. We can then export info like initiator
name and if needed isid. These patches will then also add
some files to test sessions.

Signed-off-by: Mike Christie 
---
 drivers/target/iscsi/iscsi_target.c  |  3 ++
 drivers/target/iscsi/iscsi_target_login.c|  6 ++-
 drivers/target/iscsi/iscsi_target_nego.c | 24 ++--
 drivers/target/target_core_fabric_configfs.c | 47 +++
 drivers/target/target_core_internal.h|  2 +
 drivers/target/target_core_transport.c   | 57 
 include/target/iscsi/iscsi_target_core.h |  1 +
 include/target/target_core_base.h|  3 ++
 include/target/target_core_fabric.h  |  5 ++-
 9 files changed, 127 insertions(+), 21 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 6d81a59..f350c68 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4324,6 +4324,9 @@ int iscsit_close_session(struct iscsi_session *sess)
 	iscsit_stop_time2retain_timer(sess);
 	spin_unlock_bh(_tpg->session_lock);
 
+	if (sess->fabric_registered)
+		target_fabric_unregister_session(sess->se_sess);
+
 	/*
 	 * transport_deregister_session_configfs() will clear the
 	 * struct se_node_acl->nacl_sess pointer now as a iscsi_np process context
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index d6a4b93..4f80965 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -1145,8 +1145,12 @@ void iscsi_target_login_sess_out(struct iscsi_conn *conn,
    ISCSI_LOGIN_STATUS_INIT_ERR);
 	if (!zero_tsih || !conn->sess)
 		goto old_sess_out;
-	if (conn->sess->se_sess)
+	if (conn->sess->se_sess) {
+		if (conn->sess->fabric_registered)
+			target_fabric_unregister_session(conn->sess->se_sess);
+
 		transport_free_session(conn->sess->se_sess);
+	}
 	kfree(conn->sess->sess_ops);
 	kfree(conn->sess);
 	conn->sess = NULL;
diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index 8a5e8d1..fe08e40 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -1279,11 +1279,27 @@ int iscsi_target_locate_portal(
 	tag_size = sizeof(struct iscsi_cmd) + conn->conn_transport->priv_size;
 
 	ret = transport_alloc_session_tags(sess->se_sess, tag_num, tag_size);
-	if (ret < 0) {
-		iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
-ISCSI_LOGIN_STATUS_NO_RESOURCES);
-		ret = -1;
+	if (ret < 0)
+		goto return_oom;
+
+	if (conn->tpg != iscsit_global->discovery_tpg) {
+		ret = target_fabric_register_session(>tpg->tpg_se_tpg,
+		 sess->se_sess);
+		if (ret) {
+			pr_err("Could not register session to configfs. Err %d.\n",
+			   ret);
+			/* tags and nacl released when session is freed */
+			goto return_oom;
+		}
+		sess->fabric_registered = true;
 	}
+
+	goto out;
+
+return_oom:
+	iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
+			ISCSI_LOGIN_STATUS_NO_RESOURCES);
+	ret = -1;
 out:
 	kfree(tmpbuf);
 	return ret;
diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 1fa436e..497fa01 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -878,6 +878,11 @@ static struct config_group *target_fabric_make_tpg(
 	configfs_add_default_group(_tpg->tpg_param_group,
 			_tpg->tpg_group);
 
+	config_group_init_type_name(_tpg->tpg_session_group, "sessions",
+			>tf_tpg_session_cit);
+	configfs_add_default_group(_tpg->tpg_session_group,
+			_tpg->tpg_group);
+
 	return _tpg->tpg_group;
 }
 
@@ -892,6 +897,48 @@ static void target_fabric_drop_tpg(
 	config_item_put(item);
 }
 
+static voi

[PATCH 11/15] target: export initiator port values for all sessions

[Mike Christie]

Export the initiator port info in configfs

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_fabric_configfs.c | 48 
 1 file changed, 48 insertions(+)

diff --git a/drivers/target/target_core_fabric_configfs.c 
b/drivers/target/target_core_fabric_configfs.c
index 497fa01..2deda28 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -897,6 +897,53 @@ static void target_fabric_drop_tpg(
config_item_put(item);
 }
 
+static ssize_t target_fabric_session_initiator_port_show(
+   struct config_item *item, char *page)
+{
+   struct se_session *se_sess = container_of(to_config_group(item),
+ struct se_session, group);
+   ssize_t len;
+   int isid_len = 0;
+   unsigned long flags;
+
+   spin_lock_irqsave(_sess->configfs_lock, flags);
+   if (!se_sess->se_tpg) {
+   len = -ENOTCONN;
+   goto unlock;
+   }
+
+   if (se_sess->se_tpg->se_tpg_tfo->sess_get_initiator_sid)
+   isid_len = 15;
+
+   if (strlen(se_sess->se_node_acl->initiatorname) + isid_len + 1 >
+   PAGE_SIZE) {
+   len = -EINVAL;
+   goto unlock;
+   }
+
+   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,
+  _sess->sess_bin_isid);
+   } else {
+   len = snprintf(page, PAGE_SIZE, "%s\n",
+  se_sess->se_node_acl->initiatorname);
+   }
+   len += 1; /* Include NULL terminator */
+
+unlock:
+   spin_unlock_irqrestore(_sess->configfs_lock, flags);
+
+   return len;
+}
+
+CONFIGFS_ATTR_RO(target_fabric_session_, initiator_port);
+
+static struct configfs_attribute *target_fabric_session_attrs[] = {
+   _fabric_session_attr_initiator_port,
+   NULL,
+};
+
 static void target_fabric_session_release(struct config_item *item)
 {
struct se_session *se_sess = container_of(to_config_group(item),
@@ -918,6 +965,7 @@ int target_fabric_init_session(struct se_session *se_sess)
 
se_sess->cit.ct_owner = THIS_MODULE;
se_sess->cit.ct_item_ops = _session_item_ops;
+   se_sess->cit.ct_attrs = target_fabric_session_attrs;
config_group_init_type_name(_sess->group, name, _sess->cit);
kfree(name);
return 0;
-- 
2.7.2

[PATCH 12/15] target: add callout to test a session

[Mike Christie]

This adds a callout and configfs file so userspace apps can
test a session. It is useful for apps that do not want
to enable target nops that may disrupt normal traffic
and only want to test it during specific events like
failover/failbacks when there might be multiple sessions
to the same target port.

Signed-off-by: Mike Christie 
---
 drivers/target/iscsi/iscsi_target_login.c|  2 +-
 drivers/target/target_core_fabric_configfs.c | 71 ++--
 drivers/target/target_core_transport.c   | 20 +---
 include/target/target_core_base.h|  1 +
 include/target/target_core_fabric.h  |  7 ++-
 5 files changed, 88 insertions(+), 13 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_login.c 
b/drivers/target/iscsi/iscsi_target_login.c
index 48a9522..8f219b9 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -353,7 +353,7 @@ static int iscsi_login_zero_tsih_s1(
return -ENOMEM;
}
 
-   sess->se_sess = transport_alloc_session(TARGET_PROT_NORMAL);
+   sess->se_sess = transport_alloc_session(_ops, TARGET_PROT_NORMAL);
if (IS_ERR(sess->se_sess)) {
iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
ISCSI_LOGIN_STATUS_NO_RESOURCES);
diff --git a/drivers/target/target_core_fabric_configfs.c 
b/drivers/target/target_core_fabric_configfs.c
index 2deda28..a5bb9a1 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -937,7 +937,48 @@ static ssize_t target_fabric_session_initiator_port_show(
return len;
 }
 
+static ssize_t target_fabric_session_test_store(
+   struct config_item *item, const char *page, size_t count)
+{
+   struct se_session *se_sess = container_of(to_config_group(item),
+ struct se_session, group);
+   struct se_portal_group *se_tpg;
+   unsigned long flags;
+
+   int ret;
+   u8 val;
+
+   ret = kstrtou8(page, 0, );
+   if (ret < 0)
+   return ret;
+
+   spin_lock_irqsave(_sess->configfs_lock, flags);
+   se_tpg = se_sess->se_tpg;
+   if (!se_tpg) {
+   ret = -ENOTCONN;
+   goto unlock;
+   }
+
+   if (!config_item_get_unless_zero(_tpg->tpg_group.cg_item)) {
+   ret = -ENOTCONN;
+   goto unlock;
+   }
+   spin_unlock_irqrestore(_sess->configfs_lock, flags);
+
+   ret = se_tpg->se_tpg_tfo->test_session(se_sess, val);
+   if (!ret)
+   ret = count;
+
+   config_item_put(_tpg->tpg_group.cg_item);
+   return ret;
+
+unlock:
+   spin_unlock_irqrestore(_sess->configfs_lock, flags);
+   return ret;
+}
+
 CONFIGFS_ATTR_RO(target_fabric_session_, initiator_port);
+CONFIGFS_ATTR_WO(target_fabric_session_, test);
 
 static struct configfs_attribute *target_fabric_session_attrs[] = {
_fabric_session_attr_initiator_port,
@@ -955,8 +996,11 @@ static struct configfs_item_operations 
target_session_item_ops = {
.release= target_fabric_session_release,
 };
 
-int target_fabric_init_session(struct se_session *se_sess)
+int target_fabric_init_session(const struct target_core_fabric_ops *tfo,
+  struct se_session *se_sess)
 {
+   struct configfs_attribute **attrs;
+   int len = 0, i, ret;
char *name;
 
name = kasprintf(GFP_KERNEL, "%d", se_sess->sid);
@@ -965,10 +1009,31 @@ int target_fabric_init_session(struct se_session 
*se_sess)
 
se_sess->cit.ct_owner = THIS_MODULE;
se_sess->cit.ct_item_ops = _session_item_ops;
-   se_sess->cit.ct_attrs = target_fabric_session_attrs;
+
+   for (i = 0; target_fabric_session_attrs[i] != NULL; i++)
+   len += sizeof(struct configfs_attribute *);
+   if (tfo->test_session)
+   len += sizeof(struct configfs_attribute *);
+
+   attrs = kzalloc(len, GFP_KERNEL);
+   if (!attrs) {
+   ret = -ENOMEM;
+   goto free_name;
+   }
+
+   for (i = 0; target_fabric_session_attrs[i] != NULL; i++)
+   attrs[i] = target_fabric_session_attrs[i];
+
+   if (tfo->test_session)
+   attrs[i] = _fabric_session_attr_test;
+
+   se_sess->cit.ct_attrs = se_sess->attrs = attrs;
+
config_group_init_type_name(_sess->group, name, _sess->cit);
+   ret = 0;
+free_name:
kfree(name);
-   return 0;
+   return ret;
 }
 EXPORT_SYMBOL(target_fabric_init_session);
 
diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 04ef13c..5f0f7a4 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -245,9 +245,11 @@ EXPORT_SYMBOL(transport_init_session);
 
 /**
  * transport_alloc_sessi

[PATCH 14/15] iscsi target: add test_session callout

[Mike Christie]

Send a iscsi nop as a ping for the test_session callout.

Signed-off-by: Mike Christie 
---
 drivers/target/iscsi/iscsi_target.c  |  8 ++--
 drivers/target/iscsi/iscsi_target_configfs.c | 47 ++
 drivers/target/iscsi/iscsi_target_util.c | 60 +---
 drivers/target/iscsi/iscsi_target_util.h |  3 +-
 include/target/iscsi/iscsi_target_core.h |  2 +
 5 files changed, 111 insertions(+), 9 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index 793ee98..9338f9b 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1795,11 +1795,13 @@ int iscsit_process_nop_out(struct iscsi_conn *conn, 
struct iscsi_cmd *cmd,
 * This was a response to a unsolicited NOPIN ping.
 */
if (hdr->ttt != cpu_to_be32(0x)) {
-   cmd_p = iscsit_find_cmd_from_ttt(conn, be32_to_cpu(hdr->ttt));
+   u32 ttt = be32_to_cpu(hdr->ttt);
+
+   cmd_p = iscsit_find_cmd_from_ttt(conn, ttt);
if (!cmd_p)
return -EINVAL;
 
-   iscsit_stop_nopin_response_timer(conn);
+   iscsit_stop_nopin_response_timer(conn, ttt);
 
cmd_p->i_state = ISTATE_REMOVE;
iscsit_add_cmd_to_immediate_queue(cmd_p, conn, cmd_p->i_state);
@@ -4107,7 +4109,7 @@ int iscsit_close_connection(
spin_unlock(_global->ts_bitmap_lock);
 
iscsit_stop_timers_for_cmds(conn);
-   iscsit_stop_nopin_response_timer(conn);
+   iscsit_stop_nopin_response_timer(conn, conn->test_nop_ttt);
iscsit_stop_nopin_timer(conn);
 
if (conn->conn_transport->iscsit_wait_conn)
diff --git a/drivers/target/iscsi/iscsi_target_configfs.c 
b/drivers/target/iscsi/iscsi_target_configfs.c
index d58a6f9..cedbe29 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1510,6 +1510,52 @@ static void lio_tpg_close_session(struct se_session 
*se_sess)
iscsit_close_session(sess);
 }
 
+static int lio_test_session(struct se_session *se_sess, u8 timeout)
+{
+   struct iscsi_session *sess = se_sess->fabric_sess_ptr;
+   struct iscsi_conn *conn;
+   int ret;
+   DECLARE_COMPLETION_ONSTACK(nop_done);
+
+   spin_lock_bh(>conn_lock);
+   if (sess->session_state != TARG_SESS_STATE_LOGGED_IN) {
+   ret = -ENOTCONN;
+   goto unlock;
+   }
+
+   /*
+* If the session state is still logged in, but all the
+* connections are in the process of going up/down then
+* tell caller to retry later.
+*/
+   ret = -EAGAIN;
+   list_for_each_entry(conn, >sess_conn_list, conn_list) {
+   if (conn->conn_state == TARG_CONN_STATE_LOGGED_IN) {
+   ret = 0;
+   break;
+   }
+   }
+   if (ret)
+   goto unlock;
+
+   iscsit_inc_conn_usage_count(conn);
+   spin_unlock_bh(>conn_lock);
+
+   ret = iscsit_sync_nopin(conn, timeout);
+   spin_lock_bh(>conn_lock);
+   if (sess->session_state != TARG_SESS_STATE_LOGGED_IN ||
+   conn->conn_state != TARG_CONN_STATE_LOGGED_IN)
+   ret = -ENOTCONN;
+   spin_unlock_bh(>conn_lock);
+
+   iscsit_dec_conn_usage_count(conn);
+   return ret;
+
+unlock:
+   spin_unlock_bh(>conn_lock);
+   return ret;
+}
+
 static u32 lio_tpg_get_inst_index(struct se_portal_group *se_tpg)
 {
return iscsi_tpg(se_tpg)->tpg_tiqn->tiqn_index;
@@ -1560,6 +1606,7 @@ const struct target_core_fabric_ops iscsi_ops = {
.check_stop_free= lio_check_stop_free,
.release_cmd= lio_release_cmd,
.close_session  = lio_tpg_close_session,
+   .test_session   = lio_test_session,
.sess_get_initiator_sid = lio_sess_get_initiator_sid,
.write_pending  = lio_write_pending,
.write_pending_status   = lio_write_pending_status,
diff --git a/drivers/target/iscsi/iscsi_target_util.c 
b/drivers/target/iscsi/iscsi_target_util.c
index c51c14e..549ad74 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -884,14 +884,15 @@ void iscsit_inc_conn_usage_count(struct iscsi_conn *conn)
spin_unlock_bh(>conn_usage_lock);
 }
 
-static int iscsit_add_nopin(struct iscsi_conn *conn, int want_response)
+static int iscsit_add_nopin(struct iscsi_conn *conn, int want_response,
+   bool for_test)
 {
u8 state;
struct iscsi_cmd *cmd;
 
cmd = iscsit_allocate_cmd(conn, TASK_RUNNING);
if (!cmd)
-   return -1;
+   return -ENOMEM;
 
cmd->iscsi_opcode = ISCSI_OP_NOOP_IN;
state

[PATCH 10/15] target: add lock around session fields exported in configfs

[Mike Christie]

The next patches will export the initiator port info and
add a file to test a session. This patch adds locking
around the fields that will be exported in the session's
configfs dir.

It also moves the setting of se_tpg/fabric_sess_ptr to the
end of registration so we know that when they are set
the session has been full registered.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_transport.c | 13 +++--
 include/target/target_core_base.h  |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 0efb3dc..04ef13c 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -238,6 +238,7 @@ void transport_init_session(struct se_session *se_sess)
INIT_LIST_HEAD(_sess->sess_acl_list);
INIT_LIST_HEAD(_sess->sess_cmd_list);
spin_lock_init(_sess->sess_cmd_lock);
+   spin_lock_init(_sess->configfs_lock);
init_waitqueue_head(_sess->cmd_list_wq);
 }
 EXPORT_SYMBOL(transport_init_session);
@@ -377,7 +378,6 @@ void __transport_register_session(
unsigned char buf[PR_REG_ISID_LEN];
unsigned long flags;
 
-   se_sess->se_tpg = se_tpg;
se_sess->fabric_sess_ptr = fabric_sess_ptr;
/*
 * Used by struct se_node_acl's under ConfigFS to locate active 
se_session-t
@@ -426,6 +426,10 @@ void __transport_register_session(
}
list_add_tail(_sess->sess_list, _tpg->tpg_sess_list);
 
+   spin_lock(_sess->configfs_lock);
+   se_sess->se_tpg = se_tpg;
+   spin_unlock(_sess->configfs_lock);
+
pr_debug("TARGET_CORE[%s]: Registered fabric_sess_ptr: %p\n",
se_tpg->se_tpg_tfo->get_fabric_name(), 
se_sess->fabric_sess_ptr);
 }
@@ -603,7 +607,9 @@ void transport_free_session(struct se_session *se_sess)
struct se_portal_group *se_tpg = se_nacl->se_tpg;
const struct target_core_fabric_ops *se_tfo = 
se_tpg->se_tpg_tfo;
 
+   spin_lock_irqsave(_sess->configfs_lock, flags);
se_sess->se_node_acl = NULL;
+   spin_unlock_irqrestore(_sess->configfs_lock, flags);
 
/*
 * Also determine if we need to drop the extra ->cmd_kref if
@@ -647,8 +653,11 @@ void transport_deregister_session(struct se_session 
*se_sess)
 
spin_lock_irqsave(_tpg->session_lock, flags);
list_del(_sess->sess_list);
-   se_sess->se_tpg = NULL;
se_sess->fabric_sess_ptr = NULL;
+
+   spin_lock(_sess->configfs_lock);
+   se_sess->se_tpg = NULL;
+   spin_unlock(_sess->configfs_lock);
spin_unlock_irqrestore(_tpg->session_lock, flags);
 
pr_debug("TARGET_CORE[%s]: Deregistered fabric_sess\n",
diff --git a/include/target/target_core_base.h 
b/include/target/target_core_base.h
index 01caaa3..d3bb76c 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -606,6 +606,7 @@ struct se_session {
struct list_headsess_acl_list;
struct list_headsess_cmd_list;
spinlock_t  sess_cmd_lock;
+   spinlock_t  configfs_lock;
wait_queue_head_t   cmd_list_wq;
void*sess_cmd_map;
struct sbitmap_queuesess_tag_pool;
-- 
2.7.2

[PATCH 13/15] iscsi target: check nopin_response_timeout before starting timer

[Mike Christie]

The next patch will use a iscsi nop as a ping/test IO. For
that nop we will allow the user to specify a timeout value
that may be different from the hard coded or acl
nopin_response_timeout value.

This patch adds checks so if the nopin_response_timeout is
turned off we do not accidentally turn it on for the
user initiated test nop.

Signed-off-by: Mike Christie 
---
 drivers/target/iscsi/iscsi_target_util.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_util.c 
b/drivers/target/iscsi/iscsi_target_util.c
index 5a645b5..c51c14e 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -955,7 +955,8 @@ void iscsit_mod_nopin_response_timer(struct iscsi_conn 
*conn)
struct iscsi_node_attrib *na = iscsit_tpg_get_node_attrib(sess);
 
spin_lock_bh(>nopin_timer_lock);
-   if (!(conn->nopin_response_timer_flags & ISCSI_TF_RUNNING)) {
+   if (!na->nopin_response_timeout ||
+   !(conn->nopin_response_timer_flags & ISCSI_TF_RUNNING)) {
spin_unlock_bh(>nopin_timer_lock);
return;
}
@@ -974,7 +975,8 @@ void iscsit_start_nopin_response_timer(struct iscsi_conn 
*conn)
struct iscsi_node_attrib *na = iscsit_tpg_get_node_attrib(sess);
 
spin_lock_bh(>nopin_timer_lock);
-   if (conn->nopin_response_timer_flags & ISCSI_TF_RUNNING) {
+   if (!na->nopin_response_timeout ||
+   conn->nopin_response_timer_flags & ISCSI_TF_RUNNING) {
spin_unlock_bh(>nopin_timer_lock);
return;
}
-- 
2.7.2

[PATCH 15/15] iscsi target: merge iscsit_start_nopin_timer and __iscsit_start_nopin_timer

[Mike Christie]

Just have iscsit_start_nopin_timer grab the lock and
call __iscsit_start_nopin_timer.

Signed-off-by: Mike Christie 
---
 drivers/target/iscsi/iscsi_target_util.c | 20 +---
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_util.c 
b/drivers/target/iscsi/iscsi_target_util.c
index 549ad74..0d56318 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -1103,26 +1103,8 @@ void __iscsit_start_nopin_timer(struct iscsi_conn *conn)
 
 void iscsit_start_nopin_timer(struct iscsi_conn *conn)
 {
-   struct iscsi_session *sess = conn->sess;
-   struct iscsi_node_attrib *na = iscsit_tpg_get_node_attrib(sess);
-   /*
-* NOPIN timeout is disabled..
-*/
-   if (!na->nopin_timeout)
-   return;
-
spin_lock_bh(>nopin_timer_lock);
-   if (conn->nopin_timer_flags & ISCSI_TF_RUNNING) {
-   spin_unlock_bh(>nopin_timer_lock);
-   return;
-   }
-
-   conn->nopin_timer_flags &= ~ISCSI_TF_STOP;
-   conn->nopin_timer_flags |= ISCSI_TF_RUNNING;
-   mod_timer(>nopin_timer, jiffies + na->nopin_timeout * HZ);
-
-   pr_debug("Started NOPIN Timer on CID: %d at %u second"
-   " interval\n", conn->cid, na->nopin_timeout);
+   __iscsit_start_nopin_timer(conn);
spin_unlock_bh(>nopin_timer_lock);
 }
 
-- 
2.7.2

[PATCH 09/15] target: add session dir in configfs

[Mike Christie]

Drivers like iscsi will dump one of the session's info in
a info file under the acl. The problem with this is that
it only prints one session and only sessions with an acl.

Then there is a dynamic_sessions file which dumps the initiator
name of each session without an acl. The problem with this
is that the info printed is limited to only the node name.
For iscsi we do not get the entire initiator port string
(name + isid). Also all dynamic sessions are printed in one
file so setups that do a target per lun quickly run out of
space.

To handle all these issues, this patch adds a per session
dir under the tpg. We can then export info like initiator
name and if needed isid. These patches will then also add
some files to test sessions.

TODO:
1. handle target_fabric_register_session failure in iscsi target.

Signed-off-by: Mike Christie 
---
 drivers/target/iscsi/iscsi_target.c  |  4 ++
 drivers/target/iscsi/iscsi_target_login.c|  3 ++
 drivers/target/target_core_fabric_configfs.c | 47 +++
 drivers/target/target_core_internal.h|  2 +
 drivers/target/target_core_transport.c   | 57 
 include/target/target_core_base.h|  3 ++
 include/target/target_core_fabric.h  |  5 ++-
 7 files changed, 105 insertions(+), 16 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index 6d81a59..793ee98 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4324,6 +4324,10 @@ int iscsit_close_session(struct iscsi_session *sess)
iscsit_stop_time2retain_timer(sess);
spin_unlock_bh(_tpg->session_lock);
 
+
+   if (tpg != iscsit_global->discovery_tpg)
+   target_fabric_unregister_session(sess->se_sess);
+
/*
 * transport_deregister_session_configfs() will clear the
 * struct se_node_acl->nacl_sess pointer now as a iscsi_np process 
context
diff --git a/drivers/target/iscsi/iscsi_target_login.c 
b/drivers/target/iscsi/iscsi_target_login.c
index d6a4b93..48a9522 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -778,6 +778,9 @@ void iscsi_post_login_handler(
 
iscsit_determine_maxcmdsn(sess);
 
+   if (tpg != iscsit_global->discovery_tpg)
+   target_fabric_register_session(se_tpg, se_sess);
+
spin_lock_bh(_tpg->session_lock);
__transport_register_session(>tpg->tpg_se_tpg,
se_sess->se_node_acl, se_sess, sess);
diff --git a/drivers/target/target_core_fabric_configfs.c 
b/drivers/target/target_core_fabric_configfs.c
index 1fa436e..497fa01 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -878,6 +878,11 @@ static struct config_group *target_fabric_make_tpg(
configfs_add_default_group(_tpg->tpg_param_group,
_tpg->tpg_group);
 
+   config_group_init_type_name(_tpg->tpg_session_group, "sessions",
+   >tf_tpg_session_cit);
+   configfs_add_default_group(_tpg->tpg_session_group,
+   _tpg->tpg_group);
+
return _tpg->tpg_group;
 }
 
@@ -892,6 +897,48 @@ static void target_fabric_drop_tpg(
config_item_put(item);
 }
 
+static void target_fabric_session_release(struct config_item *item)
+{
+   struct se_session *se_sess = container_of(to_config_group(item),
+ struct se_session, group);
+   target_release_session(se_sess);
+}
+
+static struct configfs_item_operations target_session_item_ops = {
+   .release= target_fabric_session_release,
+};
+
+int target_fabric_init_session(struct se_session *se_sess)
+{
+   char *name;
+
+   name = kasprintf(GFP_KERNEL, "%d", se_sess->sid);
+   if (!name)
+   return -ENOMEM;
+
+   se_sess->cit.ct_owner = THIS_MODULE;
+   se_sess->cit.ct_item_ops = _session_item_ops;
+   config_group_init_type_name(_sess->group, name, _sess->cit);
+   kfree(name);
+   return 0;
+}
+EXPORT_SYMBOL(target_fabric_init_session);
+
+int target_fabric_register_session(
+   struct se_portal_group *se_tpg,
+   struct se_session *se_sess)
+{
+   return configfs_register_group(_tpg->tpg_session_group,
+  _sess->group);
+}
+EXPORT_SYMBOL(target_fabric_register_session);
+
+void target_fabric_unregister_session(struct se_session *se_sess)
+{
+   configfs_unregister_group(_sess->group);
+}
+EXPORT_SYMBOL(target_fabric_unregister_session);
+
 static void target_fabric_release_wwn(struct config_item *item)
 {
struct se_wwn *wwn = container_of(to_config_group(item),
diff --git a/drivers/target/target_core_internal.h 
b/drivers/target/target_core_internal.h
index 0c66355..73c90ae 10

[PATCH 06/15] target: make transport_init_session_tags static

[Mike Christie]

transport_init_session_tags is only called from target_core_transport.c
so make it static.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_transport.c | 7 +++
 include/target/target_core_fabric.h| 2 --
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 97a1ee5..586f5a6b 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -322,9 +322,9 @@ EXPORT_SYMBOL(transport_alloc_session_tags);
  *   each command.
  * @sup_prot_ops: bitmask that defines which T10-PI modes are supported.
  */
-struct se_session *transport_init_session_tags(unsigned int tag_num,
-  unsigned int tag_size,
-  enum target_prot_op sup_prot_ops)
+static struct se_session *
+transport_init_session_tags(unsigned int tag_num, unsigned int tag_size,
+   enum target_prot_op sup_prot_ops)
 {
struct se_session *se_sess;
int rc;
@@ -352,7 +352,6 @@ struct se_session *transport_init_session_tags(unsigned int 
tag_num,
 
return se_sess;
 }
-EXPORT_SYMBOL(transport_init_session_tags);
 
 /*
  * Called with spin_lock_irqsave( se_portal_group->session_lock called.
diff --git a/include/target/target_core_fabric.h 
b/include/target/target_core_fabric.h
index 899da38..5964bc4 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -118,8 +118,6 @@ void transport_init_session(struct se_session *);
 struct se_session *transport_alloc_session(enum target_prot_op);
 int transport_alloc_session_tags(struct se_session *, unsigned int,
unsigned int);
-struct se_session *transport_init_session_tags(unsigned int, unsigned int,
-   enum target_prot_op);
 void   __transport_register_session(struct se_portal_group *,
struct se_node_acl *, struct se_session *, void *);
 void   transport_register_session(struct se_portal_group *,
-- 
2.7.2

[PATCH 05/15] target: remove sess_get_index

[Mike Christie]

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.

Signed-off-by: Mike Christie 
---
 Documentation/target/tcm_mod_builder.py  |  8 
 drivers/infiniband/ulp/srpt/ib_srpt.c| 15 ---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c |  6 --
 drivers/scsi/qla2xxx/tcm_qla2xxx.c   |  7 ---
 drivers/target/iscsi/iscsi_target_configfs.c |  6 --
 drivers/target/loopback/tcm_loop.c   |  6 --
 drivers/target/sbp/sbp_target.c  |  6 --
 drivers/target/target_core_configfs.c|  4 
 drivers/target/target_core_stat.c|  3 +--
 drivers/target/tcm_fc/tcm_fc.h   |  1 -
 drivers/target/tcm_fc/tfc_conf.c |  1 -
 drivers/target/tcm_fc/tfc_sess.c |  7 ---
 drivers/usb/gadget/function/f_tcm.c  |  6 --
 drivers/vhost/scsi.c |  6 --
 drivers/xen/xen-scsiback.c   |  6 --
 include/target/target_core_fabric.h  |  1 -
 16 files changed, 1 insertion(+), 88 deletions(-)

diff --git a/Documentation/target/tcm_mod_builder.py 
b/Documentation/target/tcm_mod_builder.py
index 94bf694..1befc88 100755
--- a/Documentation/target/tcm_mod_builder.py
+++ b/Documentation/target/tcm_mod_builder.py
@@ -294,7 +294,6 @@ def tcm_mod_build_configfs(proto_ident, fabric_mod_dir_var, 
fabric_mod_name):
buf += ".tpg_check_prod_mode_write_protect = " + 
fabric_mod_name + "_check_false,\n"
buf += ".tpg_get_inst_index = " + fabric_mod_name + 
"_tpg_get_inst_index,\n"
buf += ".release_cmd= " + fabric_mod_name + 
"_release_cmd,\n"
-   buf += ".sess_get_index = " + fabric_mod_name + 
"_sess_get_index,\n"
buf += ".sess_get_initiator_sid = NULL,\n"
buf += ".write_pending  = " + fabric_mod_name + 
"_write_pending,\n"
buf += ".write_pending_status   = " + fabric_mod_name + 
"_write_pending_status,\n"
@@ -465,13 +464,6 @@ def tcm_mod_dump_fabric_ops(proto_ident, 
fabric_mod_dir_var, fabric_mod_name):
buf += "}\n\n"
bufi += "void " + fabric_mod_name + 
"_release_cmd(struct se_cmd *);\n"
 
-   if re.search('sess_get_index\)\(', fo):
-   buf += "u32 " + fabric_mod_name + 
"_sess_get_index(struct se_session *se_sess)\n"
-   buf += "{\n"
-   buf += "return 0;\n"
-   buf += "}\n\n"
-   bufi += "u32 " + fabric_mod_name + 
"_sess_get_index(struct se_session *);\n"
-
if re.search('write_pending\)\(', fo):
buf += "int " + fabric_mod_name + 
"_write_pending(struct se_cmd *se_cmd)\n"
buf += "{\n"
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 07b3e1c..73fee6a 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -3198,20 +3198,6 @@ static void srpt_close_session(struct se_session 
*se_sess)
srpt_disconnect_ch_sync(ch);
 }
 
-/**
- * srpt_sess_get_index - return the value of scsiAttIntrPortIndex (SCSI-MIB)
- * @se_sess: SCSI target session.
- *
- * A quote from RFC 4455 (SCSI-MIB) about this MIB object:
- * This object represents an arbitrary integer used to uniquely identify a
- * particular attached remote initiator port to a particular SCSI target port
- * within a particular SCSI target device within a particular SCSI instance.
- */
-static u32 srpt_sess_get_index(struct se_session *se_sess)
-{
-   return 0;
-}
-
 static void srpt_set_default_node_attrs(struct se_node_acl *nacl)
 {
 }
@@ -3676,7 +3662,6 @@ static const struct target_core_fabric_ops srpt_template 
= {
.release_cmd= srpt_release_cmd,
.check_stop_free= srpt_check_stop_free,
.close_session  = srpt_close_session,
-   .sess_get_index = srpt_sess_get_index,
.sess_get_initiator_sid = NULL,
.write_pending  = srpt_write_pending,
.write_pending_status   = srpt_write_pending_status,
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index fdda04e..c04e4bd 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmv

[PATCH 02/15] target: fix isid copying and comparision

[Mike Christie]

The isid is 48 bits, and in hex string format it's 12 bytes.
We are currently copying the 12 byte hex string to a u64
so we can easily compare it, but this has the problem that
only 8 bytes of the 12 bytes are copied.

The next patches will want to print se_session sess_bin_isid
so this has us use hex2bin to when converting from the hex
sting to the bin value.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_pr.c| 20 
 drivers/target/target_core_transport.c |  3 ++-
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 01ac306..65e5253 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -662,8 +662,7 @@ static struct t10_pr_registration 
*__core_scsi3_do_alloc_registration(
rcu_read_unlock();
pr_err("Unable to locate PR deve %s mapped_lun: %llu\n",
nacl->initiatorname, mapped_lun);
-   kmem_cache_free(t10_pr_reg_cache, pr_reg);
-   return NULL;
+   goto free_reg;
}
kref_get(_reg->pr_reg_deve->pr_kref);
rcu_read_unlock();
@@ -679,12 +678,20 @@ static struct t10_pr_registration 
*__core_scsi3_do_alloc_registration(
 * save it to the registration now.
 */
if (isid != NULL) {
-   pr_reg->pr_reg_bin_isid = get_unaligned_be64(isid);
+   if (hex2bin((u8 *)_reg->pr_reg_bin_isid, isid, 6)) {
+   pr_err("Invalid isid %s\n", isid);
+   goto free_reg;
+   }
+
snprintf(pr_reg->pr_reg_isid, PR_REG_ISID_LEN, "%s", isid);
pr_reg->isid_present_at_reg = 1;
}
 
return pr_reg;
+
+free_reg:
+   kmem_cache_free(t10_pr_reg_cache, pr_reg);
+   return NULL;
 }
 
 static int core_scsi3_lunacl_depend_item(struct se_dev_entry *);
@@ -873,7 +880,12 @@ int core_scsi3_alloc_aptpl_registration(
 * SCSI Initiator Port, restore it now.
 */
if (isid != NULL) {
-   pr_reg->pr_reg_bin_isid = get_unaligned_be64(isid);
+   if (hex2bin((u8 *)_reg->pr_reg_bin_isid, isid, 6)) {
+   pr_err("Invalid isid %s\n", isid);
+   kmem_cache_free(t10_pr_reg_cache, pr_reg);
+   return -EINVAL;
+   }
+
snprintf(pr_reg->pr_reg_isid, PR_REG_ISID_LEN, "%s", isid);
pr_reg->isid_present_at_reg = 1;
}
diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 7261561..6324743 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -380,7 +380,8 @@ void __transport_register_session(
memset([0], 0, PR_REG_ISID_LEN);
se_tpg->se_tpg_tfo->sess_get_initiator_sid(se_sess,
[0], PR_REG_ISID_LEN);
-   se_sess->sess_bin_isid = get_unaligned_be64([0]);
+
+   WARN_ON(hex2bin((u8 *)_sess->sess_bin_isid, buf, 6));
}
 
spin_lock_irq(_nacl->nacl_sess_lock);
-- 
2.7.2

[PATCH 08/15] target: add session removal function

[Mike Christie]

This adds a function to remove a session which should be used by
drivers that use target_setup_session.

All the drivers but iscsi and tcm_fc were doing calling
transport_deregister_session_configfs and then immediately calling
transport_deregister_session or just calling
transport_deregisteir_session.

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.

iscsi does not use the setup/remove session functions and we
let it to continue to go wild with it calling the lower level
functions directly.

Signed-off-by: Mike Christie 
---
 drivers/infiniband/ulp/srpt/ib_srpt.c| 3 +--
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 3 +--
 drivers/scsi/qla2xxx/tcm_qla2xxx.c   | 3 +--
 drivers/target/loopback/tcm_loop.c   | 2 +-
 drivers/target/sbp/sbp_target.c  | 3 +--
 drivers/target/target_core_transport.c   | 7 +++
 drivers/target/tcm_fc/tfc_sess.c | 3 +--
 drivers/usb/gadget/function/f_tcm.c  | 2 +-
 drivers/vhost/scsi.c | 2 +-
 drivers/xen/xen-scsiback.c   | 2 +-
 include/target/target_core_fabric.h  | 1 +
 11 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 496ee6b..ba53245c 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2029,8 +2029,7 @@ static void srpt_release_channel_work(struct work_struct 
*w)
target_sess_cmd_list_set_waiting(se_sess);
target_wait_for_sess_cmds(se_sess);
 
-   transport_deregister_session_configfs(se_sess);
-   transport_deregister_session(se_sess);
+   target_remove_session(se_sess);
ch->sess = NULL;
 
if (ch->using_rdma_cm)
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index 7e2d2c0..99df92e 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -2267,8 +2267,7 @@ static int ibmvscsis_drop_nexus(struct ibmvscsis_tport 
*tport)
 * Release the SCSI I_T Nexus to the emulated ibmvscsis Target Port
 */
target_wait_for_sess_cmds(se_sess);
-   transport_deregister_session_configfs(se_sess);
-   transport_deregister_session(se_sess);
+   target_remove_session(se_sess);
tport->ibmv_nexus = NULL;
kfree(nexus);
 
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index c0e1921..e25c15d 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1456,8 +1456,7 @@ static void tcm_qla2xxx_free_session(struct fc_port *sess)
}
target_wait_for_sess_cmds(se_sess);
 
-   transport_deregister_session_configfs(sess->se_sess);
-   transport_deregister_session(sess->se_sess);
+   target_remove_session(se_sess);
 }
 
 static int tcm_qla2xxx_session_cb(struct se_portal_group *se_tpg,
diff --git a/drivers/target/loopback/tcm_loop.c 
b/drivers/target/loopback/tcm_loop.c
index 3bb2236..f3398db 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -800,7 +800,7 @@ static int tcm_loop_drop_nexus(
/*
 * Release the SCSI I_T Nexus to the emulated Target Port
 */
-   transport_deregister_session(tl_nexus->se_sess);
+   target_remove_session(se_sess);
tpg->tl_nexus = NULL;
kfree(tl_nexus);
return 0;
diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index 14244bf..2087d5f 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -235,8 +235,7 @@ static void sbp_session_release(struct sbp_session *sess, 
bool cancel_work)
if (cancel_work)
cancel_delayed_work_sync(>maint_work);
 
-   transport_deregister_session_configfs(sess->se_sess);
-   transport_deregister_session(sess->se_sess);
+   target_remove_session(sess->se_sess);
 
if (sess->card)
fw_card_put(sess->card);
diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 77c5954..57279fe 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -640,6 +640,13 @@ void transport_deregister_session(struct se_session 
*se_sess)
 }
 EXPORT_SYMBOL(transport_deregister_session);
 
+void target_remove_session(struct se_session *se_sess)
+{
+  

[PATCH 01/15] configfs: fix registered group removal

[Mike Christie]

This patch fixes a bug where configfs_register_group had added
a group in a tree, and userspace has done a rmdir on a dir somewhere
above that group and we hit a kernel crash. The problem is configfs_rmdir
will detach everything under it and unlink groups on the default_groups
list. It will not unlink groups added with configfs_register_group so when
configfs_unregister_group is called to drop its references to the group/items
we crash when we try to access the freed dentrys.

The patch just adds a check for if a rmdir has been done above
us and if so just does the unlink part of unregistration.

Sorry if you are getting this multiple times. I thouhgt I sent
this to some of you and lkml, but I do not see it.

Signed-off-by: Mike Christie 
Cc: Christoph Hellwig 
Cc: Joel Becker 
---
 fs/configfs/dir.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 577cff2..45cdbb5 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1777,6 +1777,15 @@ void configfs_unregister_group(struct config_group 
*group)
struct dentry *dentry = group->cg_item.ci_dentry;
struct dentry *parent = group->cg_item.ci_parent->ci_dentry;
 
+   mutex_lock(>su_mutex);
+   if (!group->cg_item.ci_parent->ci_group)
+   /*
+* The parent has already been unlinked and detached
+* due to a rmdir.
+*/
+   goto unlink_group;
+   mutex_unlock(>su_mutex);
+
inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
spin_lock(_dirent_lock);
configfs_detach_prep(dentry, NULL);
@@ -1791,6 +1800,7 @@ void configfs_unregister_group(struct config_group *group)
dput(dentry);
 
mutex_lock(>su_mutex);
+unlink_group:
unlink_group(group);
mutex_unlock(>su_mutex);
 }
-- 
2.7.2

[PATCH 07/15] target: rename target_alloc_session

[Mike Christie]

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.

iscsi will continue to be the odd driver.

Signed-off-by: Mike Christie 
---
 drivers/infiniband/ulp/srpt/ib_srpt.c| 6 +++---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 2 +-
 drivers/scsi/qla2xxx/tcm_qla2xxx.c   | 2 +-
 drivers/target/loopback/tcm_loop.c   | 2 +-
 drivers/target/sbp/sbp_target.c  | 2 +-
 drivers/target/target_core_transport.c   | 4 ++--
 drivers/target/tcm_fc/tfc_sess.c | 2 +-
 drivers/usb/gadget/function/f_tcm.c  | 2 +-
 drivers/vhost/scsi.c | 2 +-
 drivers/xen/xen-scsiback.c   | 2 +-
 include/target/target_core_fabric.h  | 2 +-
 11 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 73fee6a..496ee6b 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2221,16 +2221,16 @@ static int srpt_cm_req_recv(struct srpt_device *const 
sdev,
pr_debug("registering session %s\n", ch->sess_name);
 
if (sport->port_guid_tpg.se_tpg_wwn)
-   ch->sess = target_alloc_session(>port_guid_tpg, 0, 0,
+   ch->sess = target_setup_session(>port_guid_tpg, 0, 0,
TARGET_PROT_NORMAL,
ch->sess_name, ch, NULL);
if (sport->port_gid_tpg.se_tpg_wwn && IS_ERR_OR_NULL(ch->sess))
-   ch->sess = target_alloc_session(>port_gid_tpg, 0, 0,
+   ch->sess = target_setup_session(>port_gid_tpg, 0, 0,
TARGET_PROT_NORMAL, i_port_id, ch,
NULL);
/* Retry without leading "0x" */
if (sport->port_gid_tpg.se_tpg_wwn && IS_ERR_OR_NULL(ch->sess))
-   ch->sess = target_alloc_session(>port_gid_tpg, 0, 0,
+   ch->sess = target_setup_session(>port_gid_tpg, 0, 0,
TARGET_PROT_NORMAL,
i_port_id + 2, ch, NULL);
if (IS_ERR_OR_NULL(ch->sess)) {
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c 
b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index c04e4bd..7e2d2c0 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -2233,7 +2233,7 @@ static int ibmvscsis_make_nexus(struct ibmvscsis_tport 
*tport)
return -ENOMEM;
}
 
-   nexus->se_sess = target_alloc_session(>se_tpg, 0, 0,
+   nexus->se_sess = target_setup_session(>se_tpg, 0, 0,
  TARGET_PROT_NORMAL, name, nexus,
  NULL);
if (IS_ERR(nexus->se_sess)) {
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c 
b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 2516216..c0e1921 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -1534,7 +1534,7 @@ static int tcm_qla2xxx_check_initiator_node_acl(
 * Locate our struct se_node_acl either from an explict NodeACL created
 * via ConfigFS, or via running in TPG demo mode.
 */
-   se_sess = target_alloc_session(>se_tpg, num_tags,
+   se_sess = target_setup_session(>se_tpg, num_tags,
   sizeof(struct qla_tgt_cmd),
   TARGET_PROT_ALL, port_name,
   qlat_sess, tcm_qla2xxx_session_cb);
diff --git a/drivers/target/loopback/tcm_loop.c 
b/drivers/target/loopback/tcm_loop.c
index c3b39b6..3bb2236 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -760,7 +760,7 @@ static int tcm_loop_make_nexus(
if (!tl_nexus)
return -ENOMEM;
 
-   tl_nexus->se_sess = target_alloc_session(_tpg->tl_se_tpg, 0, 0,
+   tl_nexus->se_sess = target_setup_session(_tpg->tl_se_tpg, 0, 0,
TARGET_PROT_DIN_PASS | 
TARGET_PROT_DOUT_PASS,
name, tl_nexus, tcm_loop_alloc_sess_cb);
if (IS_ERR(tl_nexus->se_sess)) {
diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index 33b3b4b..14244bf 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -209,7 +209,7 @@ static

[RFC PATCH 00/15] target: export session info

[Mike Christie]

Currently, session info is in 3 places:

1. iscsi has the info file under initiator acls.

2. In the tpg dir iscsi and qla have the dynamic sessions.

3. Some lun stat files return some initiator info.

The problem with these is for:

1 and 3 - They only display 1 session's info per target port.
In multipath and HA setups you will have multiple initiator ports
connected to each target port, so for iscsi the isid will be different
for each session.

2 - We only get the initiator name. For iscsi we want the isid. Also
if we have a lot of sessions we are limited by the PAGE_SIZE file limit.

To handle all these issues these patches add session info under

/sys/kernel/config/target/iscsi/$target_name/tpgt_$N/sessions

This only a RFC right now. There is still a bug in the iscsi code
where if configfs registration fails we have to cleanup the
session.

[PATCH 04/15] target/iscsi: move session_index to common se_session

[Mike Christie]

The next patches will make session$SID dir for each session
so move the iscsi session session_index to the se_session.

This differs from the previous code in that it now uses
idr_alloc_cyclic to help prevent apps from confusing
a reused sid with a previous session.

Signed-off-by: Mike Christie 
---
 drivers/target/iscsi/iscsi_target.c  |  8 
 drivers/target/iscsi/iscsi_target_configfs.c |  4 +---
 drivers/target/iscsi/iscsi_target_login.c| 21 -
 drivers/target/iscsi/iscsi_target_stat.c |  3 +--
 drivers/target/target_core_transport.c   | 25 -
 include/target/iscsi/iscsi_target_core.h |  2 --
 include/target/target_core_base.h|  1 +
 7 files changed, 27 insertions(+), 37 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index d547dcd..6d81a59 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -57,9 +57,7 @@ static DEFINE_SPINLOCK(tiqn_lock);
 static DEFINE_MUTEX(np_lock);
 
 static struct idr tiqn_idr;
-struct idr sess_idr;
 struct mutex auth_id_lock;
-spinlock_t sess_idr_lock;
 
 struct iscsit_global *iscsit_global;
 
@@ -698,9 +696,7 @@ static int __init iscsi_target_init_module(void)
 
spin_lock_init(_global->ts_bitmap_lock);
mutex_init(_id_lock);
-   spin_lock_init(_idr_lock);
idr_init(_idr);
-   idr_init(_idr);
 
ret = target_register_template(_ops);
if (ret)
@@ -4373,10 +4369,6 @@ int iscsit_close_session(struct iscsi_session *sess)
pr_debug("Decremented number of active iSCSI Sessions on"
" iSCSI TPG: %hu to %u\n", tpg->tpgt, tpg->nsessions);
 
-   spin_lock(_idr_lock);
-   idr_remove(_idr, sess->session_index);
-   spin_unlock(_idr_lock);
-
kfree(sess->sess_ops);
sess->sess_ops = NULL;
spin_unlock_bh(_tpg->session_lock);
diff --git a/drivers/target/iscsi/iscsi_target_configfs.c 
b/drivers/target/iscsi/iscsi_target_configfs.c
index 1fcd9bc..39af194 100644
--- a/drivers/target/iscsi/iscsi_target_configfs.c
+++ b/drivers/target/iscsi/iscsi_target_configfs.c
@@ -1357,9 +1357,7 @@ static int iscsi_get_cmd_state(struct se_cmd *se_cmd)
 
 static u32 lio_sess_get_index(struct se_session *se_sess)
 {
-   struct iscsi_session *sess = se_sess->fabric_sess_ptr;
-
-   return sess->session_index;
+   return se_sess->sid;
 }
 
 static u32 lio_sess_get_initiator_sid(
diff --git a/drivers/target/iscsi/iscsi_target_login.c 
b/drivers/target/iscsi/iscsi_target_login.c
index 923b1a9..d6a4b93 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -336,22 +336,6 @@ static int iscsi_login_zero_tsih_s1(
timer_setup(>time2retain_timer,
iscsit_handle_time2retain_timeout, 0);
 
-   idr_preload(GFP_KERNEL);
-   spin_lock_bh(_idr_lock);
-   ret = idr_alloc(_idr, NULL, 0, 0, GFP_NOWAIT);
-   if (ret >= 0)
-   sess->session_index = ret;
-   spin_unlock_bh(_idr_lock);
-   idr_preload_end();
-
-   if (ret < 0) {
-   pr_err("idr_alloc() for sess_idr failed\n");
-   iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
-   ISCSI_LOGIN_STATUS_NO_RESOURCES);
-   kfree(sess);
-   return -ENOMEM;
-   }
-
sess->creation_time = get_jiffies_64();
/*
 * The FFP CmdSN window values will be allocated from the TPG's
@@ -1163,11 +1147,6 @@ void iscsi_target_login_sess_out(struct iscsi_conn *conn,
goto old_sess_out;
if (conn->sess->se_sess)
transport_free_session(conn->sess->se_sess);
-   if (conn->sess->session_index != 0) {
-   spin_lock_bh(_idr_lock);
-   idr_remove(_idr, conn->sess->session_index);
-   spin_unlock_bh(_idr_lock);
-   }
kfree(conn->sess->sess_ops);
kfree(conn->sess);
conn->sess = NULL;
diff --git a/drivers/target/iscsi/iscsi_target_stat.c 
b/drivers/target/iscsi/iscsi_target_stat.c
index df0a398..b1a41bf5 100644
--- a/drivers/target/iscsi/iscsi_target_stat.c
+++ b/drivers/target/iscsi/iscsi_target_stat.c
@@ -638,8 +638,7 @@ static ssize_t iscsi_stat_sess_indx_show(struct config_item 
*item, char *page)
if (se_sess) {
sess = se_sess->fabric_sess_ptr;
if (sess)
-   ret = snprintf(page, PAGE_SIZE, "%u\n",
-   sess->session_index);
+   ret = snprintf(page, PAGE_SIZE, "%u\n", se_sess->sid);
}
spin_unlock_bh(_nacl->nacl_sess_lock);
 
diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 75ddb

[PATCH 03/15] target: fix __transport_register_session locking

[Mike Christie]

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.

This has __transport_register_session use the save/restore
call.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_transport.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_transport.c 
b/drivers/target/target_core_transport.c
index 6324743..75ddbbb 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -347,6 +347,7 @@ void __transport_register_session(
 {
const struct target_core_fabric_ops *tfo = se_tpg->se_tpg_tfo;
unsigned char buf[PR_REG_ISID_LEN];
+   unsigned long flags;
 
se_sess->se_tpg = se_tpg;
se_sess->fabric_sess_ptr = fabric_sess_ptr;
@@ -384,7 +385,7 @@ void __transport_register_session(
WARN_ON(hex2bin((u8 *)_sess->sess_bin_isid, buf, 6));
}
 
-   spin_lock_irq(_nacl->nacl_sess_lock);
+   spin_lock_irqsave(_nacl->nacl_sess_lock, flags);
/*
 * The se_nacl->nacl_sess pointer will be set to the
 * last active I_T Nexus for each struct se_node_acl.
@@ -393,7 +394,7 @@ void __transport_register_session(
 
list_add_tail(_sess->sess_acl_list,
  _nacl->acl_sess_list);
-   spin_unlock_irq(_nacl->nacl_sess_lock);
+   spin_unlock_irqrestore(_nacl->nacl_sess_lock, flags);
}
list_add_tail(_sess->sess_list, _tpg->tpg_sess_list);
 
-- 
2.7.2

[PATCH 1/1] tcmu: Don't pass KERN_ERR to pr_err

[Mike Christie]

Fix warning:

smatch warnings:
drivers/target/target_core_user.c:301 tcmu_genl_cmd_done() warn: KERN_*
level not at start of string

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 9555981..847707a 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -383,7 +383,7 @@ static int tcmu_genl_cmd_done(struct genl_info *info, int 
completed_cmd)
}
 
if (!udev) {
-   pr_err(KERN_ERR "tcmu nl cmd %u/%d completion could not find 
device with dev id %u.\n",
+   pr_err("tcmu nl cmd %u/%d completion could not find device with 
dev id %u.\n",
   completed_cmd, rc, dev_id);
ret = -ENODEV;
goto unlock;
-- 
2.7.2

[PATCH 4/6] tcmu: misc nl code cleanup

[Mike Christie]

Some misc cleanup of the nl rework patches.

1. Fix space instead of tabs use and extra newline.
2. Drop initializing variables to 0 when not needed
3. Just pass the skb_buff and msg_header pointers to
tcmu_netlink_event_send.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 21 +
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 9835ea3..2a2e9e4e 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1628,11 +1628,9 @@ static int tcmu_netlink_event_init(struct tcmu_dev *udev,
 
 static int tcmu_netlink_event_send(struct tcmu_dev *udev,
   enum tcmu_genl_cmd cmd,
-  struct sk_buff **buf, void **hdr)
+  struct sk_buff *skb, void *msg_header)
 {
-   int ret = 0;
-   struct sk_buff *skb = *buf;
-   void *msg_header = *hdr;
+   int ret;
 
genlmsg_end(skb, msg_header);
 
@@ -1644,7 +1642,7 @@ static int tcmu_netlink_event_send(struct tcmu_dev *udev,
 
ret = genlmsg_multicast_allns(_genl_family, skb, 0,
  TCMU_MCGRP_CONFIG, GFP_KERNEL);
-   /* We don't care if no one is listening */
+   /* We don't care if no one is listening */
if (ret == -ESRCH)
ret = 0;
if (!ret)
@@ -1662,9 +1660,8 @@ static int tcmu_send_dev_add_event(struct tcmu_dev *udev)
  _header);
if (ret < 0)
return ret;
-   return tcmu_netlink_event_send(udev, TCMU_CMD_ADDED_DEVICE, ,
-  _header);
-
+   return tcmu_netlink_event_send(udev, TCMU_CMD_ADDED_DEVICE, skb,
+  msg_header);
 }
 
 static int tcmu_send_dev_remove_event(struct tcmu_dev *udev)
@@ -1678,7 +1675,7 @@ static int tcmu_send_dev_remove_event(struct tcmu_dev 
*udev)
if (ret < 0)
return ret;
return tcmu_netlink_event_send(udev, TCMU_CMD_REMOVED_DEVICE,
-  , _header);
+  skb, msg_header);
 }
 
 static int tcmu_update_uio_info(struct tcmu_dev *udev)
@@ -2197,7 +2194,7 @@ static int tcmu_send_dev_config_event(struct tcmu_dev 
*udev,
return ret;
}
return tcmu_netlink_event_send(udev, TCMU_CMD_RECONFIG_DEVICE,
-  , _header);
+  skb, msg_header);
 }
 
 
@@ -2259,7 +2256,7 @@ static int tcmu_send_dev_size_event(struct tcmu_dev 
*udev, u64 size)
return ret;
}
return tcmu_netlink_event_send(udev, TCMU_CMD_RECONFIG_DEVICE,
-  , _header);
+  skb, msg_header);
 }
 
 static ssize_t tcmu_dev_size_store(struct config_item *item, const char *page,
@@ -2341,7 +2338,7 @@ static int tcmu_send_emulate_write_cache(struct tcmu_dev 
*udev, u8 val)
return ret;
}
return tcmu_netlink_event_send(udev, TCMU_CMD_RECONFIG_DEVICE,
-  , _header);
+  skb, msg_header);
 }
 
 static ssize_t tcmu_emulate_write_cache_store(struct config_item *item,
-- 
2.7.2

[PATCH 3/6] tcmu: simplify nl interface

[Mike Christie]

Just return EBUSY if a nl request comes in while processing
one. The upper layers do not support sending multiple create/remove
requests at the same time (you cannot have a create and remove at the
same time or do multiple creates or removes at the same time) and doing
a reconfig while a create/remove is still executing does not make sense.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 26 --
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 73a5768..9835ea3 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -165,8 +165,6 @@ struct tcmu_dev {
struct list_head timedout_entry;
 
struct tcmu_nl_cmd curr_nl_cmd;
-   /* wake up threads waiting on curr_nl_cmd */
-   wait_queue_head_t nl_cmd_wq;
 
char dev_config[TCMU_CONFIG_LEN];
 
@@ -1264,8 +1262,6 @@ static struct se_device *tcmu_alloc_device(struct se_hba 
*hba, const char *name)
timer_setup(>qfull_timer, tcmu_qfull_timedout, 0);
timer_setup(>cmd_timer, tcmu_cmd_timedout, 0);
 
-   init_waitqueue_head(>nl_cmd_wq);
-
INIT_RADIX_TREE(>data_blocks, GFP_KERNEL);
 
return >se_dev;
@@ -1539,24 +1535,23 @@ static int tcmu_release(struct uio_info *info, struct 
inode *inode)
return 0;
 }
 
-static void tcmu_init_genl_cmd_reply(struct tcmu_dev *udev, int cmd)
+static int tcmu_init_genl_cmd_reply(struct tcmu_dev *udev, int cmd)
 {
struct tcmu_nl_cmd *nl_cmd = >curr_nl_cmd;
 
if (!tcmu_kern_cmd_reply_supported)
-   return;
+   return 0;
 
if (udev->nl_reply_supported <= 0)
-   return;
+   return 0;
 
-relock:
mutex_lock(_nl_cmd_mutex);
 
if (nl_cmd->cmd != TCMU_CMD_UNSPEC) {
mutex_unlock(_nl_cmd_mutex);
-   pr_debug("sleeping for open nl cmd\n");
-   wait_event(udev->nl_cmd_wq, (nl_cmd->cmd == TCMU_CMD_UNSPEC));
-   goto relock;
+   pr_warn("netlink cmd %d already executing on %s\n",
+nl_cmd->cmd, udev->name);
+   return -EBUSY;
}
 
memset(nl_cmd, 0, sizeof(*nl_cmd));
@@ -1568,6 +1563,7 @@ static void tcmu_init_genl_cmd_reply(struct tcmu_dev 
*udev, int cmd)
list_add_tail(_cmd->nl_list, _nl_cmd_list);
 
mutex_unlock(_nl_cmd_mutex);
+   return 0;
 }
 
 static int tcmu_wait_genl_cmd_reply(struct tcmu_dev *udev)
@@ -1590,8 +1586,6 @@ static int tcmu_wait_genl_cmd_reply(struct tcmu_dev *udev)
nl_cmd->status = 0;
mutex_unlock(_nl_cmd_mutex);
 
-   wake_up_all(>nl_cmd_wq);
-
return ret;
 }
 
@@ -1642,7 +1636,11 @@ static int tcmu_netlink_event_send(struct tcmu_dev *udev,
 
genlmsg_end(skb, msg_header);
 
-   tcmu_init_genl_cmd_reply(udev, cmd);
+   ret = tcmu_init_genl_cmd_reply(udev, cmd);
+   if (ret) {
+   nlmsg_free(skb);
+   return ret;
+   }
 
ret = genlmsg_multicast_allns(_genl_family, skb, 0,
  TCMU_MCGRP_CONFIG, GFP_KERNEL);
-- 
2.7.2

[PATCH 6/6] target: remove target_find_device

[Mike Christie]

target_find_device is no longer used, so remove it.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_device.c  | 24 
 include/target/target_core_backend.h |  2 --
 2 files changed, 26 deletions(-)

diff --git a/drivers/target/target_core_device.c 
b/drivers/target/target_core_device.c
index e27db4d..a9ad6ec 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -879,30 +879,6 @@ sector_t target_to_linux_sector(struct se_device *dev, 
sector_t lb)
 }
 EXPORT_SYMBOL(target_to_linux_sector);
 
-/**
- * target_find_device - find a se_device by its dev_index
- * @id: dev_index
- * @do_depend: true if caller needs target_depend_item to be done
- *
- * If do_depend is true, the caller must do a target_undepend_item
- * when finished using the device.
- *
- * If do_depend is false, the caller must be called in a configfs
- * callback or during removal.
- */
-struct se_device *target_find_device(int id, bool do_depend)
-{
-   struct se_device *dev;
-
-   mutex_lock(_mutex);
-   dev = idr_find(_idr, id);
-   if (dev && do_depend && target_depend_item(>dev_group.cg_item))
-   dev = NULL;
-   mutex_unlock(_mutex);
-   return dev;
-}
-EXPORT_SYMBOL(target_find_device);
-
 struct devices_idr_iter {
int (*fn)(struct se_device *dev, void *data);
void *data;
diff --git a/include/target/target_core_backend.h 
b/include/target/target_core_backend.h
index 34a15d5..c3ac472 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -106,8 +106,6 @@ booltarget_lun_is_rdonly(struct se_cmd *);
 sense_reason_t passthrough_parse_cdb(struct se_cmd *cmd,
sense_reason_t (*exec_cmd)(struct se_cmd *cmd));
 
-struct se_device *target_find_device(int id, bool do_depend);
-
 bool target_sense_desc_format(struct se_device *dev);
 sector_t target_to_linux_sector(struct se_device *dev, sector_t lb);
 bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
-- 
2.7.2

[PATCH 5/6] tcmu: add module wide block/reset_netlink support

[Mike Christie]

This patch based on Xiubo's patches adds 2 tcmu attr to block and
resetup the netlink interface. It's used during userspace daemon
reinitialization after the daemon has crashed while there is
outstanding nl requests. The daemon can block the nl interface,
kill outstanding requests in the kernel and then reopen the
netlink socket and unblock it to allow new requests.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 100 --
 1 file changed, 97 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 2a2e9e4e..9555981 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -94,6 +94,7 @@
 #define TCMU_GLOBAL_MAX_BLOCKS_DEF (512 * 1024)
 
 static u8 tcmu_kern_cmd_reply_supported;
+static u8 tcmu_netlink_blocked;
 
 static struct device *tcmu_root_device;
 
@@ -255,6 +256,92 @@ MODULE_PARM_DESC(global_max_data_area_mb,
 "Max MBs allowed to be allocated to all the tcmu device's "
 "data areas.");
 
+static int tcmu_get_block_netlink(char *buffer,
+ const struct kernel_param *kp)
+{
+   return sprintf(buffer, "%s\n", tcmu_netlink_blocked ?
+  "blocked" : "unblocked");
+}
+
+static int tcmu_set_block_netlink(const char *str,
+ const struct kernel_param *kp)
+{
+   int ret;
+   u8 val;
+
+   ret = kstrtou8(str, 0, );
+   if (ret < 0)
+   return ret;
+
+   if (val > 1) {
+   pr_err("Invalid block netlink value %u\n", val);
+   return -EINVAL;
+   }
+
+   tcmu_netlink_blocked = val;
+   return 0;
+}
+
+static const struct kernel_param_ops tcmu_block_netlink_op = {
+   .set = tcmu_set_block_netlink,
+   .get = tcmu_get_block_netlink,
+};
+
+module_param_cb(block_netlink, _block_netlink_op, NULL, S_IWUSR | 
S_IRUGO);
+MODULE_PARM_DESC(block_netlink, "Block new netlink commands.");
+
+static int tcmu_fail_netlink_cmd(struct tcmu_nl_cmd *nl_cmd)
+{
+   struct tcmu_dev *udev = nl_cmd->udev;
+
+   if (!tcmu_netlink_blocked) {
+   pr_err("Could not reset device's netlink interface. Netlink is 
not blocked.\n");
+   return -EBUSY;
+   }
+
+   if (nl_cmd->cmd != TCMU_CMD_UNSPEC) {
+   pr_debug("Aborting nl cmd %d on %s\n", nl_cmd->cmd, udev->name);
+   nl_cmd->status = -EINTR;
+   list_del(_cmd->nl_list);
+   complete(_cmd->complete);
+   }
+   return 0;
+}
+
+static int tcmu_set_reset_netlink(const char *str,
+ const struct kernel_param *kp)
+{
+   struct tcmu_nl_cmd *nl_cmd, *tmp_cmd;
+   int ret;
+   u8 val;
+
+   ret = kstrtou8(str, 0, );
+   if (ret < 0)
+   return ret;
+
+   if (val != 1) {
+   pr_err("Invalid reset netlink value %u\n", val);
+   return -EINVAL;
+   }
+
+   mutex_lock(_nl_cmd_mutex);
+   list_for_each_entry_safe(nl_cmd, tmp_cmd, _nl_cmd_list, nl_list) {
+   ret = tcmu_fail_netlink_cmd(nl_cmd);
+   if (ret)
+   break;
+   }
+   mutex_unlock(_nl_cmd_mutex);
+
+   return ret;
+}
+
+static const struct kernel_param_ops tcmu_reset_netlink_op = {
+   .set = tcmu_set_reset_netlink,
+};
+
+module_param_cb(reset_netlink, _reset_netlink_op, NULL, S_IWUSR);
+MODULE_PARM_DESC(reset_netlink, "Reset netlink commands.");
+
 /* multicast group */
 enum tcmu_multicast_groups {
TCMU_MCGRP_CONFIG,
@@ -303,8 +390,9 @@ static int tcmu_genl_cmd_done(struct genl_info *info, int 
completed_cmd)
}
list_del(_cmd->nl_list);
 
-   pr_debug("%s genl cmd done got id %d curr %d done %d rc %d\n",
-udev->name, dev_id, nl_cmd->cmd, completed_cmd, rc);
+   pr_debug("%s genl cmd done got id %d curr %d done %d rc %d stat %d\n",
+udev->name, dev_id, nl_cmd->cmd, completed_cmd, rc,
+nl_cmd->status);
 
if (nl_cmd->cmd != completed_cmd) {
pr_err("Mismatched commands on %s (Expecting reply for %d. 
Current %d).\n",
@@ -1547,6 +1635,13 @@ static int tcmu_init_genl_cmd_reply(struct tcmu_dev 
*udev, int cmd)
 
mutex_lock(_nl_cmd_mutex);
 
+   if (tcmu_netlink_blocked) {
+   mutex_unlock(_nl_cmd_mutex);
+   pr_warn("Failing nl cmd %d on %s. Interface is blocked.\n", cmd,
+   udev->name);
+   return -EAGAIN;
+   }
+
if (nl_cmd->cmd != TCMU_CMD_UNSPEC) {
mutex_unlock(_nl_cmd_mutex);
pr_warn("netlink cmd %d already executing on %s

tcmu: fix hung netlink requests and nl related cleanup V2

[Mike Christie]

The following patches fix the issues where the userspace daemon
has crashed and left netlink requests dangling. The daemon
can now block the interface, kill outstanding requests, reopen
the netlink socket and then unblock and execute new requests.

The patches were made over Martin's for-next branch.

v2:
- Return BUSY error immediately when there is a running command.
- Fix nl skb leak when device is blocked.
- Fix check for valid block_netlink input.
- Break out nl code cleanup into separate patch.

[PATCH 2/6] tcmu: track nl commands

[Mike Christie]

The next patch is going to fix the hung nl command issue
so this adds a list of outstanding nl commands that we
can later abort when the daemon is restarted.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 68 ++-
 1 file changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 898a561..73a5768 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -103,9 +103,16 @@ struct tcmu_hba {
 
 #define TCMU_CONFIG_LEN 256
 
+static DEFINE_MUTEX(tcmu_nl_cmd_mutex);
+static LIST_HEAD(tcmu_nl_cmd_list);
+
+struct tcmu_dev;
+
 struct tcmu_nl_cmd {
/* wake up thread waiting for reply */
struct completion complete;
+   struct list_head nl_list;
+   struct tcmu_dev *udev;
int cmd;
int status;
 };
@@ -157,7 +164,6 @@ struct tcmu_dev {
 
struct list_head timedout_entry;
 
-   spinlock_t nl_cmd_lock;
struct tcmu_nl_cmd curr_nl_cmd;
/* wake up threads waiting on curr_nl_cmd */
wait_queue_head_t nl_cmd_wq;
@@ -270,11 +276,9 @@ static struct nla_policy tcmu_attr_policy[TCMU_ATTR_MAX+1] 
= {
 
 static int tcmu_genl_cmd_done(struct genl_info *info, int completed_cmd)
 {
-   struct se_device *dev;
-   struct tcmu_dev *udev;
+   struct tcmu_dev *udev = NULL;
struct tcmu_nl_cmd *nl_cmd;
int dev_id, rc, ret = 0;
-   bool is_removed = (completed_cmd == TCMU_CMD_REMOVED_DEVICE);
 
if (!info->attrs[TCMU_ATTR_CMD_STATUS] ||
!info->attrs[TCMU_ATTR_DEVICE_ID]) {
@@ -285,33 +289,36 @@ static int tcmu_genl_cmd_done(struct genl_info *info, int 
completed_cmd)
dev_id = nla_get_u32(info->attrs[TCMU_ATTR_DEVICE_ID]);
rc = nla_get_s32(info->attrs[TCMU_ATTR_CMD_STATUS]);
 
-   dev = target_find_device(dev_id, !is_removed);
-   if (!dev) {
-   printk(KERN_ERR "tcmu nl cmd %u/%u completion could not find 
device with dev id %u.\n",
-  completed_cmd, rc, dev_id);
-   return -ENODEV;
+   mutex_lock(_nl_cmd_mutex);
+   list_for_each_entry(nl_cmd, _nl_cmd_list, nl_list) {
+   if (nl_cmd->udev->se_dev.dev_index == dev_id) {
+   udev = nl_cmd->udev;
+   break;
+   }
}
-   udev = TCMU_DEV(dev);
 
-   spin_lock(>nl_cmd_lock);
-   nl_cmd = >curr_nl_cmd;
+   if (!udev) {
+   pr_err(KERN_ERR "tcmu nl cmd %u/%d completion could not find 
device with dev id %u.\n",
+  completed_cmd, rc, dev_id);
+   ret = -ENODEV;
+   goto unlock;
+   }
+   list_del(_cmd->nl_list);
 
-   pr_debug("genl cmd done got id %d curr %d done %d rc %d\n", dev_id,
-nl_cmd->cmd, completed_cmd, rc);
+   pr_debug("%s genl cmd done got id %d curr %d done %d rc %d\n",
+udev->name, dev_id, nl_cmd->cmd, completed_cmd, rc);
 
if (nl_cmd->cmd != completed_cmd) {
-   printk(KERN_ERR "Mismatched commands (Expecting reply for %d. 
Current %d).\n",
-  completed_cmd, nl_cmd->cmd);
+   pr_err("Mismatched commands on %s (Expecting reply for %d. 
Current %d).\n",
+  udev->name, completed_cmd, nl_cmd->cmd);
ret = -EINVAL;
-   } else {
-   nl_cmd->status = rc;
+   goto unlock;
}
 
-   spin_unlock(>nl_cmd_lock);
-   if (!is_removed)
-   target_undepend_item(>dev_group.cg_item);
-   if (!ret)
-   complete(_cmd->complete);
+   nl_cmd->status = rc;
+   complete(_cmd->complete);
+unlock:
+   mutex_unlock(_nl_cmd_mutex);
return ret;
 }
 
@@ -1258,7 +1265,6 @@ static struct se_device *tcmu_alloc_device(struct se_hba 
*hba, const char *name)
timer_setup(>cmd_timer, tcmu_cmd_timedout, 0);
 
init_waitqueue_head(>nl_cmd_wq);
-   spin_lock_init(>nl_cmd_lock);
 
INIT_RADIX_TREE(>data_blocks, GFP_KERNEL);
 
@@ -1544,10 +1550,10 @@ static void tcmu_init_genl_cmd_reply(struct tcmu_dev 
*udev, int cmd)
return;
 
 relock:
-   spin_lock(>nl_cmd_lock);
+   mutex_lock(_nl_cmd_mutex);
 
if (nl_cmd->cmd != TCMU_CMD_UNSPEC) {
-   spin_unlock(>nl_cmd_lock);
+   mutex_unlock(_nl_cmd_mutex);
pr_debug("sleeping for open nl cmd\n");
wait_event(udev->nl_cmd_wq, (nl_cmd->cmd == TCMU_CMD_UNSPEC));
goto relock;
@@ -1555,9 +1561,13 @@ static void tcmu_init_genl_cmd_reply(struct tcmu_dev 
*udev, int cmd)
 
memset(nl_cmd, 0, sizeof(*nl_cmd));
nl_cmd->cmd = cmd;
+   nl_cmd->udev = ud

[PATCH 1/6] tcmu: delete unused __wait

[Mike Christie]

When this code change this was never cleaned up.

Signed-off-by: Mike Christie 
---
 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 e4a76f9..898a561 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1564,7 +1564,6 @@ static int tcmu_wait_genl_cmd_reply(struct tcmu_dev *udev)
 {
struct tcmu_nl_cmd *nl_cmd = >curr_nl_cmd;
int ret;
-   DEFINE_WAIT(__wait);
 
if (!tcmu_kern_cmd_reply_supported)
return 0;
-- 
2.7.2

Re: [PATCH 3/3] tcmu: add module wide block/reset_netlink support

[Mike Christie]

On 06/21/2018 10:52 PM, Mike Christie wrote:
> +static int tcmu_set_block_netlink(const char *str,
> +   const struct kernel_param *kp)
> +{
> + int ret;
> + u8 val;
> +
> + ret = kstrtou8(str, 0, );
> + if (ret < 0)
> + return ret;
> +
> + if (val > 1 || val < 0) {

Darn. I meant to send a different version of the patch where it did

if (val != 1) {

since its a unsigned value.

I will resend this patch after you guys review it.

[PATCH 2/3] tcmu: track nl commands

[Mike Christie]

The next patch is going to fix the hung nl command issue
so this adds a list of outstanding nl commands that we
can later abort when the daemon is restarted.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 68 ++-
 1 file changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 898a561..73a5768 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -103,9 +103,16 @@ struct tcmu_hba {
 
 #define TCMU_CONFIG_LEN 256
 
+static DEFINE_MUTEX(tcmu_nl_cmd_mutex);
+static LIST_HEAD(tcmu_nl_cmd_list);
+
+struct tcmu_dev;
+
 struct tcmu_nl_cmd {
/* wake up thread waiting for reply */
struct completion complete;
+   struct list_head nl_list;
+   struct tcmu_dev *udev;
int cmd;
int status;
 };
@@ -157,7 +164,6 @@ struct tcmu_dev {
 
struct list_head timedout_entry;
 
-   spinlock_t nl_cmd_lock;
struct tcmu_nl_cmd curr_nl_cmd;
/* wake up threads waiting on curr_nl_cmd */
wait_queue_head_t nl_cmd_wq;
@@ -270,11 +276,9 @@ static struct nla_policy tcmu_attr_policy[TCMU_ATTR_MAX+1] 
= {
 
 static int tcmu_genl_cmd_done(struct genl_info *info, int completed_cmd)
 {
-   struct se_device *dev;
-   struct tcmu_dev *udev;
+   struct tcmu_dev *udev = NULL;
struct tcmu_nl_cmd *nl_cmd;
int dev_id, rc, ret = 0;
-   bool is_removed = (completed_cmd == TCMU_CMD_REMOVED_DEVICE);
 
if (!info->attrs[TCMU_ATTR_CMD_STATUS] ||
!info->attrs[TCMU_ATTR_DEVICE_ID]) {
@@ -285,33 +289,36 @@ static int tcmu_genl_cmd_done(struct genl_info *info, int 
completed_cmd)
dev_id = nla_get_u32(info->attrs[TCMU_ATTR_DEVICE_ID]);
rc = nla_get_s32(info->attrs[TCMU_ATTR_CMD_STATUS]);
 
-   dev = target_find_device(dev_id, !is_removed);
-   if (!dev) {
-   printk(KERN_ERR "tcmu nl cmd %u/%u completion could not find 
device with dev id %u.\n",
-  completed_cmd, rc, dev_id);
-   return -ENODEV;
+   mutex_lock(_nl_cmd_mutex);
+   list_for_each_entry(nl_cmd, _nl_cmd_list, nl_list) {
+   if (nl_cmd->udev->se_dev.dev_index == dev_id) {
+   udev = nl_cmd->udev;
+   break;
+   }
}
-   udev = TCMU_DEV(dev);
 
-   spin_lock(>nl_cmd_lock);
-   nl_cmd = >curr_nl_cmd;
+   if (!udev) {
+   pr_err(KERN_ERR "tcmu nl cmd %u/%d completion could not find 
device with dev id %u.\n",
+  completed_cmd, rc, dev_id);
+   ret = -ENODEV;
+   goto unlock;
+   }
+   list_del(_cmd->nl_list);
 
-   pr_debug("genl cmd done got id %d curr %d done %d rc %d\n", dev_id,
-nl_cmd->cmd, completed_cmd, rc);
+   pr_debug("%s genl cmd done got id %d curr %d done %d rc %d\n",
+udev->name, dev_id, nl_cmd->cmd, completed_cmd, rc);
 
if (nl_cmd->cmd != completed_cmd) {
-   printk(KERN_ERR "Mismatched commands (Expecting reply for %d. 
Current %d).\n",
-  completed_cmd, nl_cmd->cmd);
+   pr_err("Mismatched commands on %s (Expecting reply for %d. 
Current %d).\n",
+  udev->name, completed_cmd, nl_cmd->cmd);
ret = -EINVAL;
-   } else {
-   nl_cmd->status = rc;
+   goto unlock;
}
 
-   spin_unlock(>nl_cmd_lock);
-   if (!is_removed)
-   target_undepend_item(>dev_group.cg_item);
-   if (!ret)
-   complete(_cmd->complete);
+   nl_cmd->status = rc;
+   complete(_cmd->complete);
+unlock:
+   mutex_unlock(_nl_cmd_mutex);
return ret;
 }
 
@@ -1258,7 +1265,6 @@ static struct se_device *tcmu_alloc_device(struct se_hba 
*hba, const char *name)
timer_setup(>cmd_timer, tcmu_cmd_timedout, 0);
 
init_waitqueue_head(>nl_cmd_wq);
-   spin_lock_init(>nl_cmd_lock);
 
INIT_RADIX_TREE(>data_blocks, GFP_KERNEL);
 
@@ -1544,10 +1550,10 @@ static void tcmu_init_genl_cmd_reply(struct tcmu_dev 
*udev, int cmd)
return;
 
 relock:
-   spin_lock(>nl_cmd_lock);
+   mutex_lock(_nl_cmd_mutex);
 
if (nl_cmd->cmd != TCMU_CMD_UNSPEC) {
-   spin_unlock(>nl_cmd_lock);
+   mutex_unlock(_nl_cmd_mutex);
pr_debug("sleeping for open nl cmd\n");
wait_event(udev->nl_cmd_wq, (nl_cmd->cmd == TCMU_CMD_UNSPEC));
goto relock;
@@ -1555,9 +1561,13 @@ static void tcmu_init_genl_cmd_reply(struct tcmu_dev 
*udev, int cmd)
 
memset(nl_cmd, 0, sizeof(*nl_cmd));
nl_cmd->cmd = cmd;
+   nl_cmd->udev = ud

[PATCH 1/3] tcmu: delete unused __wait

[Mike Christie]

When this code change this was never cleaned up.

Signed-off-by: Mike Christie 
---
 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 e4a76f9..898a561 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1564,7 +1564,6 @@ static int tcmu_wait_genl_cmd_reply(struct tcmu_dev *udev)
 {
struct tcmu_nl_cmd *nl_cmd = >curr_nl_cmd;
int ret;
-   DEFINE_WAIT(__wait);
 
if (!tcmu_kern_cmd_reply_supported)
return 0;
-- 
2.7.2

[PATCH 3/3] tcmu: add module wide block/reset_netlink support

[Mike Christie]

This patch based on Xiubo's patches adds 2 tcmu attr to block and
resetup the netlink interface. It's used during userspace daemon
reinitialization after the daemon has crashed while there is
outstanding nl requests. The daemon can block the nl interface,
kill outstanding requests in the kernel and then reopen the
netlink socket and unblock it to allow new requests.

Signed-off-by: Mike Christie 
---
 drivers/target/target_core_user.c | 115 +++---
 1 file changed, 107 insertions(+), 8 deletions(-)

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 73a5768..0b2491d 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -94,6 +94,7 @@
 #define TCMU_GLOBAL_MAX_BLOCKS_DEF (512 * 1024)
 
 static u8 tcmu_kern_cmd_reply_supported;
+static u8 tcmu_netlink_blocked;
 
 static struct device *tcmu_root_device;
 
@@ -257,6 +258,92 @@ MODULE_PARM_DESC(global_max_data_area_mb,
 "Max MBs allowed to be allocated to all the tcmu device's "
 "data areas.");
 
+static int tcmu_get_block_netlink(char *buffer,
+ const struct kernel_param *kp)
+{
+   return sprintf(buffer, "%s\n", tcmu_netlink_blocked ?
+  "blocked" : "unblocked");
+}
+
+static int tcmu_set_block_netlink(const char *str,
+ const struct kernel_param *kp)
+{
+   int ret;
+   u8 val;
+
+   ret = kstrtou8(str, 0, );
+   if (ret < 0)
+   return ret;
+
+   if (val > 1 || val < 0) {
+   pr_err("Invalid block netlink value %u\n", val);
+   return -EINVAL;
+   }
+
+   tcmu_netlink_blocked = val;
+   return 0;
+}
+
+static const struct kernel_param_ops tcmu_block_netlink_op = {
+   .set = tcmu_set_block_netlink,
+   .get = tcmu_get_block_netlink,
+};
+
+module_param_cb(block_netlink, _block_netlink_op, NULL, S_IWUSR | 
S_IRUGO);
+MODULE_PARM_DESC(block_netlink, "Block new netlink commands.");
+
+static int tcmu_fail_netlink_cmd(struct tcmu_nl_cmd *nl_cmd)
+{
+   struct tcmu_dev *udev = nl_cmd->udev;
+
+   if (!tcmu_netlink_blocked) {
+   pr_err("Could not reset device's netlink interface. Netlink is 
not blocked.\n");
+   return -EBUSY;
+   }
+
+   if (nl_cmd->cmd != TCMU_CMD_UNSPEC) {
+   pr_debug("Aborting nl cmd %d on %s\n", nl_cmd->cmd, udev->name);
+   nl_cmd->status = -EINTR;
+   list_del(_cmd->nl_list);
+   complete(_cmd->complete);
+   }
+   return 0;
+}
+
+static int tcmu_set_reset_netlink(const char *str,
+ const struct kernel_param *kp)
+{
+   struct tcmu_nl_cmd *nl_cmd, *tmp_cmd;
+   int ret;
+   u8 val;
+
+   ret = kstrtou8(str, 0, );
+   if (ret < 0)
+   return ret;
+
+   if (val != 1) {
+   pr_err("Invalid reset netlink value %u\n", val);
+   return -EINVAL;
+   }
+
+   mutex_lock(_nl_cmd_mutex);
+   list_for_each_entry_safe(nl_cmd, tmp_cmd, _nl_cmd_list, nl_list) {
+   ret = tcmu_fail_netlink_cmd(nl_cmd);
+   if (ret)
+   break;
+   }
+   mutex_unlock(_nl_cmd_mutex);
+
+   return ret;
+}
+
+static const struct kernel_param_ops tcmu_reset_netlink_op = {
+   .set = tcmu_set_reset_netlink,
+};
+
+module_param_cb(reset_netlink, _reset_netlink_op, NULL, S_IWUSR);
+MODULE_PARM_DESC(reset_netlink, "Reset netlink commands.");
+
 /* multicast group */
 enum tcmu_multicast_groups {
TCMU_MCGRP_CONFIG,
@@ -305,8 +392,9 @@ static int tcmu_genl_cmd_done(struct genl_info *info, int 
completed_cmd)
}
list_del(_cmd->nl_list);
 
-   pr_debug("%s genl cmd done got id %d curr %d done %d rc %d\n",
-udev->name, dev_id, nl_cmd->cmd, completed_cmd, rc);
+   pr_debug("%s genl cmd done got id %d curr %d done %d rc %d stat %d\n",
+udev->name, dev_id, nl_cmd->cmd, completed_cmd, rc,
+nl_cmd->status);
 
if (nl_cmd->cmd != completed_cmd) {
pr_err("Mismatched commands on %s (Expecting reply for %d. 
Current %d).\n",
@@ -1539,19 +1627,26 @@ static int tcmu_release(struct uio_info *info, struct 
inode *inode)
return 0;
 }
 
-static void tcmu_init_genl_cmd_reply(struct tcmu_dev *udev, int cmd)
+static int tcmu_init_genl_cmd_reply(struct tcmu_dev *udev, int cmd)
 {
struct tcmu_nl_cmd *nl_cmd = >curr_nl_cmd;
 
if (!tcmu_kern_cmd_reply_supported)
-   return;
+   return 0;
 
if (udev->nl_reply_supported <= 0)
-   return;
+   return 0;
 
 re

[PATCH 0/3] tcmu: fix hung netlink requests during restarts

[Mike Christie]

The following patches fix the issues where the userspace daemon
has crashed and left netlink requests dangling. The daemon
can block the interface, kill outstanding requests, reopen
the netlink socket and then unblock and execute new requests.

The patches were made over Martin's for-next branch.

Re: [PATCH 01/33] TCMU PR: first commit to implement TCMU PR

[Mike Christie]

On 06/16/2018 02:20 PM, Mike Christie wrote:
> Adding Bodo who is working on a alternative approach.
> 
> On 06/16/2018 12:22 AM, Christoph Hellwig wrote:
>> > On Sat, Jun 16, 2018 at 02:23:10AM +0800, Zhu Lingshan wrote:
>>> >> These commits and the following intend to implement Persistent
>>> >> Reservation operations for TCMU devices.
>> > 
>> > Err, hell no.
>> > 
>> > If you are that tightly integrated with the target code that you can
>> > implement persistent reservation you need to use kernel code.
>> > Everything else just creates a way too complex interface.
> Hey Christoph,
> 
> Just wanted to make sure I understood this comment. In Lingshan's
> patches I think he was going to end up calling out to
> userspace/tcmu-runner and there he was going to make ceph calls which
> basically translate PGR operations to ceph requests. Are you saying we
> should just add some kernel module that makes the ceph calls? This would
> then avoid the mess of having the split PGR processing design in this
> patchset?

Oh yeah, I meant to also ask if I understood you correctly above, then
did you also just want us to add the target_core_rbd module that did
READ/WRITE/VAAI commands too?

Re: [PATCH 01/33] TCMU PR: first commit to implement TCMU PR

[Mike Christie]

Adding Bodo who is working on a alternative approach.

On 06/16/2018 12:22 AM, Christoph Hellwig wrote:
> On Sat, Jun 16, 2018 at 02:23:10AM +0800, Zhu Lingshan wrote:
>> These commits and the following intend to implement Persistent
>> Reservation operations for TCMU devices.
> 
> Err, hell no.
> 
> If you are that tightly integrated with the target code that you can
> implement persistent reservation you need to use kernel code.
> Everything else just creates a way too complex interface.

Hey Christoph,

Just wanted to make sure I understood this comment. In Lingshan's
patches I think he was going to end up calling out to
userspace/tcmu-runner and there he was going to make ceph calls which
basically translate PGR operations to ceph requests. Are you saying we
should just add some kernel module that makes the ceph calls? This would
then avoid the mess of having the split PGR processing design in this
patchset?


Also just to make it a little more fun :) There is another person
working on a completely different design.

Bodo's design is for tcmu only and allows userspace to just handle
everything. PGR commands and related checks for conflicts are all
handled in the userspace daemon that is receiving commands from the
target_core_user kernel module.

The one hiccup is this design requires a change where the I_T Nexus info
is passed to userspace so it can handle certain PGR registration
commands correction and also for each command check if a nexus has been
registered and what reservation there is for it. We could:

1. Just pass that info to userspace in each tcmu request.

2. Do something like the old kernel tgt code did where when a I_T nexus
was established in the kernel it sent an event to userspace. Each SCSI
command passed to userspace had some tag that allowed userspace to match
the tag with the I_T Nexus.

Re: [PATCH] tcmu: remove useless code and clean up the code style.

[Mike Christie]

On 06/07/2018 01:31 AM, xiu...@redhat.com wrote:
> From: Xiubo Li 
> 
> Since the TCMU_RING_SIZE macro is not using here will discard it
> and at the same time clean up the code style.
> 
> Signed-off-by: Xiubo Li 
> ---
>  drivers/target/target_core_user.c | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/target/target_core_user.c 
> b/drivers/target/target_core_user.c
> index 4f26bdc..2c63936 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -79,14 +79,10 @@
>  #define DATA_BLOCK_SIZE PAGE_SIZE
>  #define DATA_BLOCK_SHIFT PAGE_SHIFT
>  #define DATA_BLOCK_BITS_DEF (256 * 1024)
> -#define DATA_SIZE (DATA_BLOCK_BITS * DATA_BLOCK_SIZE)
>  
>  #define TCMU_MBS_TO_BLOCKS(_mbs) (_mbs << (20 - DATA_BLOCK_SHIFT))
>  #define TCMU_BLOCKS_TO_MBS(_blocks) (_blocks >> (20 - DATA_BLOCK_SHIFT))
>  
> -/* The total size of the ring is 8M + 256K * PAGE_SIZE */
> -#define TCMU_RING_SIZE (CMDR_SIZE + DATA_SIZE)
> -
>  /*
>   * Default number of global data blocks(512K * PAGE_SIZE)
>   * when the unmap thread will be started.
> @@ -279,7 +275,7 @@ static int tcmu_genl_cmd_done(struct genl_info *info, int 
> completed_cmd)
>   if (!info->attrs[TCMU_ATTR_CMD_STATUS] ||
>   !info->attrs[TCMU_ATTR_DEVICE_ID]) {
>   printk(KERN_ERR "TCMU_ATTR_CMD_STATUS or TCMU_ATTR_DEVICE_ID 
> not set, doing nothing\n");
> -return -EINVAL;
> + return -EINVAL;
>  }
>  
>   dev_id = nla_get_u32(info->attrs[TCMU_ATTR_DEVICE_ID]);
> @@ -309,7 +305,7 @@ static int tcmu_genl_cmd_done(struct genl_info *info, int 
> completed_cmd)
>  
>   spin_unlock(>nl_cmd_lock);
>   if (!is_removed)
> -  target_undepend_item(>dev_group.cg_item);
> + target_undepend_item(>dev_group.cg_item);
>   if (!ret)
>   complete(_cmd->complete);
>   return ret;
> 

Acked-by: Mike Christie 

Re: [PATCH v2] target: transport should allow st ILI reads

[Mike Christie]

On 05/10/2018 01:51 PM, Mike Christie wrote:
> On 05/09/2018 03:59 PM, Lee Duncan wrote:
>> When a tape drive is exported via LIO using the
>> pscsi module, a read that requests more bytes per block
>> than the tape can supply returns an empty buffer. This
>> is because the pscsi pass-through target module sees
>> the "ILI" illegal length bit set and thinks there
>> is no reason to return the data.
>>
>> This is a long-standing transport issue, since it
>> assume that no data need be returned under a check
>> condition, which isn't always the case for tape.
>>
>> Add in a check for tape reads with the the ILI, EOM,
>> or FM bits set, with a sense code of NO_SENSE,
>> treating such cases as if there is no sense data
>> and the read succeeded. The layered tape driver then
>> "does the right thing" when it gets such a response.
>>
>> Changes from RFC:
>>  - Moved ugly code from transport to pscsi module
>>  - Added checking EOM and FM bits, as well as ILI
>>  - fixed malformed patch
>>  - Clarified description a bit
>>
>> Signed-off-by: Lee Duncan <ldun...@suse.com>
>> ---
>>  drivers/target/target_core_pscsi.c | 22 +-
>>  drivers/target/target_core_transport.c |  6 ++
>>  include/target/target_core_base.h  |  1 +
>>  3 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/target/target_core_pscsi.c 
>> b/drivers/target/target_core_pscsi.c
>> index 0d99b242e82e..b237104af81c 100644
>> --- a/drivers/target/target_core_pscsi.c
>> +++ b/drivers/target/target_core_pscsi.c
>> @@ -689,8 +689,28 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8 
>> scsi_status,
>>  }
>>  after_mode_select:
>>  
>> -if (scsi_status == SAM_STAT_CHECK_CONDITION)
>> +if (scsi_status == SAM_STAT_CHECK_CONDITION) {
>>  transport_copy_sense_to_cmd(cmd, req_sense);
>> +
>> +/*
>> + * hack to check for TAPE device reads with
>> + * FM/EOM/ILI set, so that we can get data
>> + * back despite framework assumption that a
>> + * check condition means there is no data
>> + */
>> +if ((sd->type == TYPE_TAPE) &&
>> +(cmd->data_direction == DMA_FROM_DEVICE)) {
>> +/*
>> + * is sense data valid, fixed format,
>> + * and have FM, EOM, or ILI set?
>> + */
>> +if ((req_sense[0] == 0xf0) &&   /* valid, fixed format 
>> */
>> +(req_sense[2] & 0xe0) &&/* FM, EOM, or ILI */
>> +((req_sense[2] & 0xf) == 0)) { /* key==NO_SENSE */
>> +cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;
>> +}
>> +}
>> +}
>>  }
>>  
>>  enum {
>> diff --git a/drivers/target/target_core_transport.c 
>> b/drivers/target/target_core_transport.c
>> index 74b646f165d4..56661a824266 100644
>> --- a/drivers/target/target_core_transport.c
>> +++ b/drivers/target/target_core_transport.c
>> @@ -2192,6 +2192,11 @@ static void target_complete_ok_work(struct 
>> work_struct *work)
>>  if (atomic_read(>se_dev->dev_qf_count) != 0)
>>  schedule_work(>se_dev->qf_work_queue);
>>  
>> +if (cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) {
>> +pr_debug("Tape FM/EOM/ILI status detected. Treating as OK\n");
>> +goto treat_as_normal_read;
>> +}
>> +
>>  /*
>>   * Check if we need to send a sense buffer from
>>   * the struct se_cmd in question.
>> @@ -2241,6 +2246,7 @@ static void target_complete_ok_work(struct work_struct 
>> *work)
>>  if (cmd->scsi_status)
>>  goto queue_status;
>>  
>> +treat_as_normal_read:
>>  atomic_long_add(cmd->data_length,
>>  >se_lun->lun_stats.tx_data_octets);
> 
> 
> Do you want to return both the data and sense or just one or the other?
> Both right? In this code path we would return both the data and sense
> for drivers like iscsi.
> 
> If the queue_data_in call a little below this line returns
> -ENOMEM/-EAGAIN then I think the queue full handling is going to end up
> only returning the sense like in your original bug. You would need a
> similar change to transport_complete_qf so it returns the data.
> 

Oh yeah, one other question/comment. The above code is bypassing the
normal sense sending code so SCF_SENT_CHECK_CONDITION is not going to be
set. For iscsi could you end up where 2 paths complete the same command
because a reassign could race with one of these completions?

Re: [PATCH v2] target: transport should allow st ILI reads

[Mike Christie]

On 05/09/2018 03:59 PM, Lee Duncan wrote:
> When a tape drive is exported via LIO using the
> pscsi module, a read that requests more bytes per block
> than the tape can supply returns an empty buffer. This
> is because the pscsi pass-through target module sees
> the "ILI" illegal length bit set and thinks there
> is no reason to return the data.
> 
> This is a long-standing transport issue, since it
> assume that no data need be returned under a check
> condition, which isn't always the case for tape.
> 
> Add in a check for tape reads with the the ILI, EOM,
> or FM bits set, with a sense code of NO_SENSE,
> treating such cases as if there is no sense data
> and the read succeeded. The layered tape driver then
> "does the right thing" when it gets such a response.
> 
> Changes from RFC:
>  - Moved ugly code from transport to pscsi module
>  - Added checking EOM and FM bits, as well as ILI
>  - fixed malformed patch
>  - Clarified description a bit
> 
> Signed-off-by: Lee Duncan 
> ---
>  drivers/target/target_core_pscsi.c | 22 +-
>  drivers/target/target_core_transport.c |  6 ++
>  include/target/target_core_base.h  |  1 +
>  3 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/target/target_core_pscsi.c 
> b/drivers/target/target_core_pscsi.c
> index 0d99b242e82e..b237104af81c 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -689,8 +689,28 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8 
> scsi_status,
>   }
>  after_mode_select:
>  
> - if (scsi_status == SAM_STAT_CHECK_CONDITION)
> + if (scsi_status == SAM_STAT_CHECK_CONDITION) {
>   transport_copy_sense_to_cmd(cmd, req_sense);
> +
> + /*
> +  * hack to check for TAPE device reads with
> +  * FM/EOM/ILI set, so that we can get data
> +  * back despite framework assumption that a
> +  * check condition means there is no data
> +  */
> + if ((sd->type == TYPE_TAPE) &&
> + (cmd->data_direction == DMA_FROM_DEVICE)) {
> + /*
> +  * is sense data valid, fixed format,
> +  * and have FM, EOM, or ILI set?
> +  */
> + if ((req_sense[0] == 0xf0) &&   /* valid, fixed format 
> */
> + (req_sense[2] & 0xe0) &&/* FM, EOM, or ILI */
> + ((req_sense[2] & 0xf) == 0)) { /* key==NO_SENSE */
> + cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;
> + }
> + }
> + }
>  }
>  
>  enum {
> diff --git a/drivers/target/target_core_transport.c 
> b/drivers/target/target_core_transport.c
> index 74b646f165d4..56661a824266 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2192,6 +2192,11 @@ static void target_complete_ok_work(struct work_struct 
> *work)
>   if (atomic_read(>se_dev->dev_qf_count) != 0)
>   schedule_work(>se_dev->qf_work_queue);
>  
> + if (cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) {
> + pr_debug("Tape FM/EOM/ILI status detected. Treating as OK\n");
> + goto treat_as_normal_read;
> + }
> +
>   /*
>* Check if we need to send a sense buffer from
>* the struct se_cmd in question.
> @@ -2241,6 +2246,7 @@ static void target_complete_ok_work(struct work_struct 
> *work)
>   if (cmd->scsi_status)
>   goto queue_status;
>  
> +treat_as_normal_read:
>   atomic_long_add(cmd->data_length,
>   >se_lun->lun_stats.tx_data_octets);


Do you want to return both the data and sense or just one or the other?
Both right? In this code path we would return both the data and sense
for drivers like iscsi.

If the queue_data_in call a little below this line returns
-ENOMEM/-EAGAIN then I think the queue full handling is going to end up
only returning the sense like in your original bug. You would need a
similar change to transport_complete_qf so it returns the data.

Re: [RESEND] tcmu: fix error resetting qfull_time_out to default

[Mike Christie]

On 05/10/2018 08:42 AM, Prasanna Kumar Kalever wrote:
> Problem:
> ---
> $ cat /sys/kernel/config/target/core/user_0/block/attrib/qfull_time_out
> -1
> 
> $ echo "-1" > 
> /sys/kernel/config/target/core/user_0/block/attrib/qfull_time_out
> -bash: echo: write error: Invalid argument
> 
> Fix:
> ---
> This patch will help reset qfull_time_out to its default i.e. 
> qfull_time_out=-1
> 
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kale...@redhat.com>
> ---
>  drivers/target/target_core_user.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/target/target_core_user.c 
> b/drivers/target/target_core_user.c
> index 4ad89ea71a70..4f26bdc3d1dc 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -2121,6 +2121,8 @@ static ssize_t tcmu_qfull_time_out_store(struct 
> config_item *item,
>  
>   if (val >= 0) {
>   udev->qfull_time_out = val * MSEC_PER_SEC;
> + } else if (val == -1) {
> + udev->qfull_time_out = val;
>   } else {
>   printk(KERN_ERR "Invalid qfull timeout value %d\n", val);
>   return -EINVAL;
> 

Look ok.

Acked-by: Mike Christie <mchri...@redhat.com>