re: libiscsi: Use scsi helper to set information descriptor

re: libiscsi: Use scsi helper to set information descriptor

[Dan Carpenter]

Hello Sagi Grimberg,

The patch a73c2a2f9123: "libiscsi: Use scsi helper to set information
descriptor" from Jul 15, 2015, leads to the following static checker
warning:

	drivers/scsi/libiscsi.c:858 iscsi_scsi_cmd_rsp()
	error: XXX uninitialized symbol 'sector'.

drivers/scsi/libiscsi.c
   850                  ascq = session->tt->check_protection(task, &sector);

If "ascq" is 0x1 then there sector might not be initialized.  The
documentation is not clear on how that works.  Har dee har har.  The
oldest jokes are still the best...  :P

   851                  if (ascq) {
   852                          sc->result = DRIVER_SENSE << 24 |
   853                                       SAM_STAT_CHECK_CONDITION;
   854                          scsi_build_sense_buffer(1, sc->sense_buffer,
   855                                                  ILLEGAL_REQUEST, 0x10, ascq);
   856                          scsi_set_sense_information(sc->sense_buffer,
   857                                                     SCSI_SENSE_BUFFERSIZE,
   858                                                     sector);
   859                          goto out;
   860                  }

regards,
dan carpenter

Re: libiscsi: Use scsi helper to set information descriptor

[Sagi Grimberg]

Hey Dan,

> Hello Sagi Grimberg,
>
> The patch a73c2a2f9123: "libiscsi: Use scsi helper to set information
> descriptor" from Jul 15, 2015, leads to the following static checker
> warning:
>
> 	drivers/scsi/libiscsi.c:858 iscsi_scsi_cmd_rsp()
> 	error: XXX uninitialized symbol 'sector'.
>
> drivers/scsi/libiscsi.c
>     850                  ascq = session->tt->check_protection(task, &sector);
>
> If "ascq" is 0x1 then there sector might not be initialized.  The
> documentation is not clear on how that works.  Har dee har har.  The
> oldest jokes are still the best...  :P

iscsi transports that implement this callout are expected
to set the sector which is passed by reference.

would it make the checker happy if we set sector to 0 before
calling check_protection (although it's not needed by no means)?

Re: libiscsi: Use scsi helper to set information descriptor

[Dan Carpenter]

On Wed, Apr 13, 2016 at 05:53:08PM +0300, Sagi Grimberg wrote:
> Hey Dan,
> 
> >Hello Sagi Grimberg,
> >
> >The patch a73c2a2f9123: "libiscsi: Use scsi helper to set information
> >descriptor" from Jul 15, 2015, leads to the following static checker
> >warning:
> >
> >	drivers/scsi/libiscsi.c:858 iscsi_scsi_cmd_rsp()
> >	error: XXX uninitialized symbol 'sector'.
> >
> >drivers/scsi/libiscsi.c
> >    850                  ascq = session->tt->check_protection(task, &sector);
> >
> >If "ascq" is 0x1 then there sector might not be initialized.  The
> >documentation is not clear on how that works.  Har dee har har.  The
> >oldest jokes are still the best...  :P
> 
> iscsi transports that implement this callout are expected
> to set the sector which is passed by reference.

There is only iscsi_iser_check_protection() I think. It behaves how I
described in my original email.  If ib_check_mr_status() fails it
returns without setting sector.

> 
> would it make the checker happy if we set sector to 0 before
> calling check_protection (although it's not needed by no means)?

It looks for if there is any possible way that it could be uninitialized
so that would solve the problem.

regards,
dan carpenter

Re: libiscsi: Use scsi helper to set information descriptor

[Dan Carpenter]

You saw the bug in the end right?

  1112  u8 iser_check_task_pi_status(struct iscsi_iser_task *iser_task,
  1113                               enum iser_data_dir cmd_dir, sector_t *sector)
                                                                           ^^^^^^
The caller assumes sector is initialized on error.

  1114  {
  1115          struct iser_mem_reg *reg = &iser_task->rdma_reg[cmd_dir];
  1116          struct iser_fr_desc *desc = reg->mem_h;
  1117          unsigned long sector_size = iser_task->sc->device->sector_size;
  1118          struct ib_mr_status mr_status;
  1119          int ret;
  1120  
  1121          if (desc && desc->pi_ctx->sig_protected) {
  1122                  desc->pi_ctx->sig_protected = 0;
  1123                  ret = ib_check_mr_status(desc->pi_ctx->sig_mr,
  1124                                           IB_MR_CHECK_SIG_STATUS, &mr_status);
  1125                  if (ret) {
  1126                          pr_err("ib_check_mr_status failed, ret %d\n", ret);
  1127                          goto err;

But we have an error path where it's not initialized.

  1128                  }
  1129  
  1130                  if (mr_status.fail_status & IB_MR_CHECK_SIG_STATUS) {
  1131                          sector_t sector_off = mr_status.sig_err.sig_err_offset;
  1132  
  1133                          sector_div(sector_off, sector_size + 8);
  1134                          *sector = scsi_get_lba(iser_task->sc) + sector_off;
  1135  
  1136                          pr_err("PI error found type %d at sector %llx "
  1137                                 "expected %x vs actual %x\n",
  1138                                 mr_status.sig_err.err_type,
  1139                                 (unsigned long long)*sector,
  1140                                 mr_status.sig_err.expected,
  1141                                 mr_status.sig_err.actual);
  1142  
  1143                          switch (mr_status.sig_err.err_type) {
  1144                          case IB_SIG_BAD_GUARD:
  1145                                  return 0x1;
  1146                          case IB_SIG_BAD_REFTAG:
  1147                                  return 0x3;
  1148                          case IB_SIG_BAD_APPTAG:
  1149                                  return 0x2;
  1150                          }
  1151                  }
  1152          }
  1153  
  1154          return 0;
  1155  err:
  1156          /* Not alot we can do here, return ambiguous guard error */
  1157          return 0x1;
  1158  }

regards,
dan carpenter

-- 
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.