[PATCH] scsi: Fix a harmless double shift bug

[Dan Carpenter]

Smatch generates a warning:

drivers/scsi/scsi_lib.c:1656 scsi_mq_done() warn: test_bit() takes a bit 
number

The problem is that SCMD_STATE_COMPLETE is supposed to be bit number 0
and not a mask like "(1 << 0)".  It is used like this:

if (test_and_set_bit(SCMD_STATE_COMPLETE, >state))

The test_and_set_bit() has a shift built in so it's a double left shift
and uses bit number 1 instead of number 0.  This bug is harmless because
it's done consistently and it doesn't clash with any other flags.

Fixes: f1342709d18a ("scsi: Do not rely on blk-mq for double completions")
Signed-off-by: Dan Carpenter 
---
 include/scsi/scsi_cmnd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 3de905e205ce..d85e6befa26b 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -62,7 +62,7 @@ struct scsi_pointer {
 #define SCMD_PRESERVED_FLAGS   (SCMD_UNCHECKED_ISA_DMA | SCMD_INITIALIZED)
 
 /* for scmd->state */
-#define SCMD_STATE_COMPLETE(1 << 0)
+#define SCMD_STATE_COMPLETE0
 
 struct scsi_cmnd {
struct scsi_request req;
-- 
2.11.0

[PATCH] scsi: bnx2fc: Fix NULL dereference in error handling

[Dan Carpenter]

If "interface" is NULL then we can't release it and trying to will only
lead to an Oops.

Fixes: aea71a024914 ("[SCSI] bnx2fc: Introduce interface structure for each 
vlan interface")
Signed-off-by: Dan Carpenter 
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c 
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index cd160f2ec75d..bcd30e2374f1 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -2364,7 +2364,7 @@ static int _bnx2fc_create(struct net_device *netdev,
if (!interface) {
printk(KERN_ERR PFX "bnx2fc_interface_create failed\n");
rc = -ENOMEM;
-   goto ifput_err;
+   goto netdev_err;
}
 
if (is_vlan_dev(netdev)) {
-- 
2.11.0

Re: [PATCH] scsi: lpfc: Uninitialized variable in lpfc_debugfs_nodelist_data()

[Dan Carpenter]

On Mon, Oct 22, 2018 at 08:25:49AM +0100, James Bottomley wrote:
> On Mon, 2018-10-22 at 09:50 +0300, Dan Carpenter wrote:
> > There was a merge problem and we accidentally removed the "nrport"
> > initialization.
> > 
> > Fixes: 77c5bf5647b5 ("Merge branch 'misc' into for-next")
> > Signed-off-by: Dan Carpenter 
> > ---
> >  drivers/scsi/lpfc/lpfc_debugfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/lpfc/lpfc_debugfs.c
> > b/drivers/scsi/lpfc/lpfc_debugfs.c
> > index c6912394b56d..0c8005bb0f53 100644
> > --- a/drivers/scsi/lpfc/lpfc_debugfs.c
> > +++ b/drivers/scsi/lpfc/lpfc_debugfs.c
> > @@ -550,7 +550,7 @@ lpfc_debugfs_nodelist_data(struct lpfc_vport
> > *vport, char *buf, int size)
> > struct lpfc_nodelist *ndlp;
> > unsigned char *statep;
> > struct nvme_fc_local_port *localport;
> > -   struct nvme_fc_remote_port *nrport;
> > +   struct nvme_fc_remote_port *nrport = NULL;
> 
> Really? that's not the way I did the merge in my for-next:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git/commit/?h=for-next=1f86cc958e2a60cef07546b15bdce90713a05e80
> 
> 77c5bf5647b5 looks to be the orphaned residue of a failed merge ... how
> did you find it because it's not in any of the public branches?

It was in Friday's linux-next.  Anyway, so long as it was fixed now.

regards,
dan carpenter

[PATCH] scsi: lpfc: Uninitialized variable in lpfc_debugfs_nodelist_data()

[Dan Carpenter]

There was a merge problem and we accidentally removed the "nrport"
initialization.

Fixes: 77c5bf5647b5 ("Merge branch 'misc' into for-next")
Signed-off-by: Dan Carpenter 
---
 drivers/scsi/lpfc/lpfc_debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/lpfc/lpfc_debugfs.c b/drivers/scsi/lpfc/lpfc_debugfs.c
index c6912394b56d..0c8005bb0f53 100644
--- a/drivers/scsi/lpfc/lpfc_debugfs.c
+++ b/drivers/scsi/lpfc/lpfc_debugfs.c
@@ -550,7 +550,7 @@ lpfc_debugfs_nodelist_data(struct lpfc_vport *vport, char 
*buf, int size)
struct lpfc_nodelist *ndlp;
unsigned char *statep;
struct nvme_fc_local_port *localport;
-   struct nvme_fc_remote_port *nrport;
+   struct nvme_fc_remote_port *nrport = NULL;
struct lpfc_nvme_rport *rport;
 
cnt = (LPFC_NODELIST_SIZE / LPFC_NODELIST_ENTRY_SIZE);
-- 
2.11.0

Re: [PATCH 3/3] scsi: myrs: prevent negatives in disable_enclosure_messages_store()

[Dan Carpenter]

On Fri, Oct 19, 2018 at 12:19:09PM +0300, Dan Carpenter wrote:
> We only want the value to be zero or one.
> 
> It's not a big deal, but say we passed set value to INT_MIN, then
> disable_enclosure_messages_show() would return that 12 bytes of "buf"
> are initialized but actually only 3 are.  I think there are tools like
> KASAN which will trigger an info leak warning when that happens.
> 
> Fixes: 77266186397c ("scsi: myrs: Add Mylex RAID controller (SCSI interface)")
> Signed-off-by: Dan Carpenter 
> ---
>  drivers/scsi/myrs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
> index 07e5a3f54e31..55842ed54231 100644
> --- a/drivers/scsi/myrs.c
> +++ b/drivers/scsi/myrs.c
> @@ -1501,7 +1501,7 @@ static ssize_t disable_enclosure_messages_store(struct 
> device *dev,
>   if (ret)
>   return ret;
>  
> - if (value > 2)
> + if (value < 0 || value > 2)
>   return -EINVAL;

It's not actually clear to me why we allow 2.  Shouldn't we just use
kstrtobool()?

regards,
dan carpenter

[bug report] scsi: myrb: Add Mylex RAID controller (block interface)

[Dan Carpenter]

Hello Hannes Reinecke,

The patch 081ff398c56c: "scsi: myrb: Add Mylex RAID controller (block
interface)" from Oct 17, 2018, leads to the following static checker
warning:

drivers/scsi/myrb.c:1614 myrb_ldev_queuecommand()
warn: assigning signed to unsigned: 'mbox->type5.sg_count = nsge' 
'(-12),0,2-s32max'

drivers/scsi/myrb.c
  1579  if (scmd->sc_data_direction == DMA_NONE)
  1580  goto submit;
  1581  nsge = scsi_dma_map(scmd);
^
nsge can be -ENOMEM;

  1582  if (nsge == 1) {
  1583  sgl = scsi_sglist(scmd);
  1584  if (scmd->sc_data_direction == DMA_FROM_DEVICE)
  1585  mbox->type5.opcode = MYRB_CMD_READ;
  1586  else
  1587  mbox->type5.opcode = MYRB_CMD_WRITE;
  1588  
  1589  mbox->type5.ld.xfer_len = block_cnt;
  1590  mbox->type5.ld.ldev_num = sdev->id;
  1591  mbox->type5.lba = lba;
  1592  mbox->type5.addr = (u32)sg_dma_address(sgl);
  1593  } else {
  1594  struct myrb_sge *hw_sgl;
  1595  dma_addr_t hw_sgl_addr;
  1596  int i;
  1597  
  1598  hw_sgl = dma_pool_alloc(cb->sg_pool, GFP_ATOMIC, 
_sgl_addr);
  1599  if (!hw_sgl)
  1600  return SCSI_MLQUEUE_HOST_BUSY;
  1601  
  1602  cmd_blk->sgl = hw_sgl;
  1603  cmd_blk->sgl_addr = hw_sgl_addr;
  1604  
  1605  if (scmd->sc_data_direction == DMA_FROM_DEVICE)
  1606  mbox->type5.opcode = MYRB_CMD_READ_SG;
  1607  else
  1608  mbox->type5.opcode = MYRB_CMD_WRITE_SG;
  1609  
  1610  mbox->type5.ld.xfer_len = block_cnt;
  1611  mbox->type5.ld.ldev_num = sdev->id;
  1612  mbox->type5.lba = lba;
  1613  mbox->type5.addr = hw_sgl_addr;
  1614  mbox->type5.sg_count = nsge;
  1615  
  1616  scsi_for_each_sg(scmd, sgl, nsge, i) {
  1617  hw_sgl->sge_addr = (u32)sg_dma_address(sgl);
  1618  hw_sgl->sge_count = (u32)sg_dma_len(sgl);
  1619  hw_sgl++;
  1620  }
  1621  }
  1622  submit:
  1623  spin_lock_irqsave(>queue_lock, flags);

regards,
dan carpenter

[PATCH 3/3] scsi: myrs: prevent negatives in disable_enclosure_messages_store()

[Dan Carpenter]

We only want the value to be zero or one.

It's not a big deal, but say we passed set value to INT_MIN, then
disable_enclosure_messages_show() would return that 12 bytes of "buf"
are initialized but actually only 3 are.  I think there are tools like
KASAN which will trigger an info leak warning when that happens.

Fixes: 77266186397c ("scsi: myrs: Add Mylex RAID controller (SCSI interface)")
Signed-off-by: Dan Carpenter 
---
 drivers/scsi/myrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
index 07e5a3f54e31..55842ed54231 100644
--- a/drivers/scsi/myrs.c
+++ b/drivers/scsi/myrs.c
@@ -1501,7 +1501,7 @@ static ssize_t disable_enclosure_messages_store(struct 
device *dev,
if (ret)
return ret;
 
-   if (value > 2)
+   if (value < 0 || value > 2)
return -EINVAL;
 
cs->disable_enc_msg = value;
-- 
2.11.0

[PATCH 2/3] scsi: myrs: Fix the processor absent message in processor_show()

[Dan Carpenter]

If both processors are absent then it's supposed to print that, but
instead we print that just the second processor is absent.

Fixes: 77266186397c ("scsi: myrs: Add Mylex RAID controller (SCSI interface)")
Signed-off-by: Dan Carpenter 
---
 drivers/scsi/myrs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
index b02ee0b0dd55..07e5a3f54e31 100644
--- a/drivers/scsi/myrs.c
+++ b/drivers/scsi/myrs.c
@@ -1366,11 +1366,11 @@ static ssize_t processor_show(struct device *dev,
   first_processor, info->cpu[0].cpu_count,
   info->cpu[1].cpu_name,
   second_processor, info->cpu[1].cpu_count);
-   else if (!second_processor)
+   else if (first_processor && !second_processor)
ret = snprintf(buf, 64, "1: %s (%s, %d cpus)\n2: absent\n",
   info->cpu[0].cpu_name,
   first_processor, info->cpu[0].cpu_count);
-   else if (!first_processor)
+   else if (!first_processor && second_processor)
ret = snprintf(buf, 64, "1: absent\n2: %s (%s, %d cpus)\n",
   info->cpu[1].cpu_name,
   second_processor, info->cpu[1].cpu_count);
-- 
2.11.0

[PATCH 1/3] scsi: myrs: Fix a logical vs bitwise bug

[Dan Carpenter]

The || was supposed to be |.  The original code just sets ->result to 1.

Fixes: 77266186397c ("scsi: myrs: Add Mylex RAID controller (SCSI interface)")
Signed-off-by: Dan Carpenter 
---
 drivers/scsi/myrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
index b02ee0b0dd55..17e691bde485 100644
--- a/drivers/scsi/myrs.c
+++ b/drivers/scsi/myrs.c
@@ -2085,7 +2085,7 @@ static void myrs_handle_scsi(struct myrs_hba *cs, struct 
myrs_cmdblk *cmd_blk,
status == MYRS_STATUS_DEVICE_NON_RESPONSIVE2)
scmd->result = (DID_BAD_TARGET << 16);
else
-   scmd->result = (DID_OK << 16) || status;
+   scmd->result = (DID_OK << 16) | status;
scmd->scsi_done(scmd);
 }
 
-- 
2.11.0

[PATCH] scsi: qla2xxx: don't allow negative thresholds

[Dan Carpenter]

We shouldn't allow negative thresholds.  I don't know what it would do
but it can't be good.

Fixes: 8b4673ba3a1b ("scsi: qla2xxx: Add support for ZIO6 interrupt threshold")
Signed-off-by: Dan Carpenter 

diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
index a31d23905753..b28f159fdaee 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -1228,7 +1228,7 @@ qla_zio_threshold_store(struct device *dev, struct 
device_attribute *attr,
return -EINVAL;
if (sscanf(buf, "%d", ) != 1)
return -EINVAL;
-   if (val > 256)
+   if (val < 0 || val > 256)
return -ERANGE;
 
atomic_set(>hw->zio_threshold, val);

Re: [PATCH 4/6] qla2xxx_nvmet: Add FC-NVMe Target handling

[Dan Carpenter]

Hi Anil,

I love your patch! Perhaps something to improve:

url:
https://github.com/0day-ci/linux/commits/Himanshu-Madhani/qla2xxx-Add-FC-NVMe-Target-support/20180916-090108
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next

smatch warnings:
drivers/scsi/qla2xxx/qla_target.c:891 qlt_queue_purex() warn: taking sizeof 
binop
drivers/scsi/qla2xxx/qla_target.c:902 qlt_queue_purex() error: memcpy() 
'p->purex_pyld' too small (4 vs 44)

# 
https://github.com/0day-ci/linux/commit/51867b7ad96cb9b1d5a96effc476a2e5a48293ae
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 51867b7ad96cb9b1d5a96effc476a2e5a48293ae
vim +891 drivers/scsi/qla2xxx/qla_target.c

51867b7a Anil Gurumurthy 2018-09-14  868  
51867b7a Anil Gurumurthy 2018-09-14  869  static void 
qlt_queue_purex(scsi_qla_host_t *vha,
51867b7a Anil Gurumurthy 2018-09-14  870struct atio_from_isp *atio)
51867b7a Anil Gurumurthy 2018-09-14  871  {
51867b7a Anil Gurumurthy 2018-09-14  872struct qla_tgt_purex_op *p;
51867b7a Anil Gurumurthy 2018-09-14  873unsigned long flags;
51867b7a Anil Gurumurthy 2018-09-14  874struct purex_entry_24xx *purex =
51867b7a Anil Gurumurthy 2018-09-14  875(struct 
purex_entry_24xx *)>u.raw;
51867b7a Anil Gurumurthy 2018-09-14  876uint16_t len = 
purex->frame_size;
51867b7a Anil Gurumurthy 2018-09-14  877uint8_t *purex_pyld_tmp;
51867b7a Anil Gurumurthy 2018-09-14  878  
51867b7a Anil Gurumurthy 2018-09-14  879p = kzalloc(sizeof(*p), 
GFP_ATOMIC);
51867b7a Anil Gurumurthy 2018-09-14  880if (p == NULL)
51867b7a Anil Gurumurthy 2018-09-14  881goto out;
51867b7a Anil Gurumurthy 2018-09-14  882  
51867b7a Anil Gurumurthy 2018-09-14  883p->vha = vha;
51867b7a Anil Gurumurthy 2018-09-14  884memcpy(>atio, atio, 
sizeof(*atio));
51867b7a Anil Gurumurthy 2018-09-14  885  
51867b7a Anil Gurumurthy 2018-09-14  886ql_dbg(ql_dbg_disc + 
ql_dbg_buffer, vha, 0xff11,
51867b7a Anil Gurumurthy 2018-09-14  887"Dumping the Purex IOCB 
received\n");
51867b7a Anil Gurumurthy 2018-09-14  888ql_dump_buffer(ql_dbg_disc + 
ql_dbg_buffer, vha, 0xe012,
51867b7a Anil Gurumurthy 2018-09-14  889(uint8_t *)purex, 64);
51867b7a Anil Gurumurthy 2018-09-14  890  
51867b7a Anil Gurumurthy 2018-09-14 @891p->purex_pyld = 
kzalloc(sizeof(purex->entry_count * 64), GFP_ATOMIC);

^^^
The parens are wrong so

51867b7a Anil Gurumurthy 2018-09-14  892if (p->purex_pyld == NULL) {
51867b7a Anil Gurumurthy 2018-09-14  893kfree(p);
51867b7a Anil Gurumurthy 2018-09-14  894goto out;
51867b7a Anil Gurumurthy 2018-09-14  895}
51867b7a Anil Gurumurthy 2018-09-14  896purex_pyld_tmp = (uint8_t 
*)p->purex_pyld;
51867b7a Anil Gurumurthy 2018-09-14  897p->purex_pyld_len = len;
51867b7a Anil Gurumurthy 2018-09-14  898  
51867b7a Anil Gurumurthy 2018-09-14  899if (len < PUREX_PYLD_SIZE)
51867b7a Anil Gurumurthy 2018-09-14  900len = PUREX_PYLD_SIZE;
51867b7a Anil Gurumurthy 2018-09-14  901  
51867b7a Anil Gurumurthy 2018-09-14 @902memcpy(p->purex_pyld, 
>d_id, PUREX_PYLD_SIZE);
   ^
it leads to a memory corruption warning as well.

51867b7a Anil Gurumurthy 2018-09-14  903purex_pyld_tmp += 
PUREX_PYLD_SIZE;
51867b7a Anil Gurumurthy 2018-09-14  904len -= PUREX_PYLD_SIZE;
51867b7a Anil Gurumurthy 2018-09-14  905  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

[PATCH] scsi: qla2xxx: Fix an endian bug in fcpcmd_is_corrupted()

[Dan Carpenter]

We should first do the le16_to_cpu endian conversion and then apply
the FCP_CMD_LENGTH_MASK mask.

Fixes: 5f35509db179 ("qla2xxx: Terminate exchange if corrupted")
Signed-off-by: Dan Carpenter 
---
I just happened to spot this when I was reviewing something unrelated.
I don't have the hardware to test it, so please review carefully.

diff --git a/drivers/scsi/qla2xxx/qla_target.h 
b/drivers/scsi/qla2xxx/qla_target.h
index fecf96f0225c..199d3ba1916d 100644
--- a/drivers/scsi/qla2xxx/qla_target.h
+++ b/drivers/scsi/qla2xxx/qla_target.h
@@ -374,8 +374,8 @@ struct atio_from_isp {
 static inline int fcpcmd_is_corrupted(struct atio *atio)
 {
if (atio->entry_type == ATIO_TYPE7 &&
-   (le16_to_cpu(atio->attr_n_length & FCP_CMD_LENGTH_MASK) <
-   FCP_CMD_LENGTH_MIN))
+   ((le16_to_cpu(atio->attr_n_length) & FCP_CMD_LENGTH_MASK) <
+FCP_CMD_LENGTH_MIN))
return 1;
else
return 0;

[bug report] Linux-2.6.12-rc2

[Dan Carpenter]

Hi SCSI devs,

The patch 1da177e4c3f4: "Linux-2.6.12-rc2" from Apr 16, 2005, leads
to the following static checker warning:

drivers/scsi/aic7xxx/aic79xx_core.c:6440 ahd_init_scbdata() warn: integer 
overflow (literal): u32max + 1
drivers/scsi/aic7xxx/aic79xx_core.c:6454 ahd_init_scbdata() warn: integer 
overflow (literal): u32max + 1
drivers/scsi/aic7xxx/aic79xx_core.c:6473 ahd_init_scbdata() warn: integer 
overflow (literal): u32max + 1
drivers/scsi/aic7xxx/aic79xx_core.c:7128 ahd_init() warn: integer overflow 
(literal): u32max + 1
drivers/scsi/aic7xxx/aic7xxx_core.c:4807 ahc_init_scbdata() warn: integer 
overflow (literal): u32max + 1
drivers/scsi/aic7xxx/aic7xxx_core.c:4839 ahc_init_scbdata() warn: integer 
overflow (literal): u32max + 1
drivers/scsi/aic7xxx/aic7xxx_core.c:4871 ahc_init_scbdata() warn: integer 
overflow (literal): u32max + 1
drivers/scsi/aic7xxx/aic7xxx_core.c:5362 ahc_init() warn: integer overflow 
(literal): u32max + 1

drivers/scsi/aic7xxx/aic79xx_core.c
  6419  /* Determine the number of hardware SCBs and initialize them */
  6420  scb_data->maxhscbs = ahd_probe_scbs(ahd);
  6421  if (scb_data->maxhscbs == 0) {
  6422  printk("%s: No SCB space found\n", ahd_name(ahd));
  6423  return (ENXIO);
  6424  }
  6425  
  6426  ahd_initialize_hscbs(ahd);
  6427  
  6428  /*
  6429   * Create our DMA tags.  These tags define the kinds of device
  6430   * accessible memory allocations and memory mappings we will
  6431   * need to perform during normal operation.
  6432   *
  6433   * Unless we need to further restrict the allocation, we rely
  6434   * on the restrictions of the parent dmat, hence the common
  6435   * use of MAXADDR and MAXSIZE.
  6436   */
  6437  
  6438  /* DMA tag for our hardware scb structures */
  6439  if (ahd_dma_tag_create(ahd, ahd->parent_dmat, /*alignment*/1,
  6440 /*boundary*/BUS_SPACE_MAXADDR_32BIT + 1,
   ^^^
This is UINT_MAX + 1, and we're passing it to a unsigned int type.  I
have no idea what's going on...  Probably no one cares, but I just
thought I would throw it out there in case anyone knows what was
intended.

  6441 /*lowaddr*/BUS_SPACE_MAXADDR_32BIT,
  6442 /*highaddr*/BUS_SPACE_MAXADDR,
  6443 /*filter*/NULL, /*filterarg*/NULL,
  6444 PAGE_SIZE, /*nsegments*/1,
  6445 /*maxsegsz*/BUS_SPACE_MAXSIZE_32BIT,
  6446 /*flags*/0, _data->hscb_dmat) != 0) {
  6447  goto error_exit;
  6448  }
  6449  
  6450  scb_data->init_level++;
  6451  
  6452  /* DMA tag for our S/G structures. */
  6453  if (ahd_dma_tag_create(ahd, ahd->parent_dmat, /*alignment*/8,
  6454 /*boundary*/BUS_SPACE_MAXADDR_32BIT + 1,
  6455 /*lowaddr*/BUS_SPACE_MAXADDR_32BIT,
  6456 /*highaddr*/BUS_SPACE_MAXADDR,
  6457 /*filter*/NULL, /*filterarg*/NULL,
  6458 ahd_sglist_allocsize(ahd), 
/*nsegments*/1,
  6459 /*maxsegsz*/BUS_SPACE_MAXSIZE_32BIT,
  6460 /*flags*/0, _data->sg_dmat) != 0) {
  6461  goto error_exit;
  6462  }

regards,
dan carpenter

[PATCH] scsi: aacraid: fix a signednes bug

[Dan Carpenter]

The problem is that ->reset_state is a u8 but it can be set to -1 or -2
in aac_tmf_callback() and the error handling in aac_eh_target_reset()
relies on it to be signed.

Fixes: 0d643ff3c353 ("scsi: aacraid: use aac_tmf_callback for reset fib")
Signed-off-by: Dan Carpenter 

diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 29bf1e60f542..39eb415987fc 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -1346,7 +1346,7 @@ struct fib {
 struct aac_hba_map_info {
__le32  rmw_nexus;  /* nexus for native HBA devices */
u8  devtype;/* device type */
-   u8  reset_state;/* 0 - no reset, 1..x - */
+   s8  reset_state;/* 0 - no reset, 1..x - */
/* after xth TM LUN reset */
u16 qd_limit;
u32 scan_counter;

[PATCH] scsi: lpfc: remove an unnecessary NULL check

[Dan Carpenter]

Smatch complains about this code:

drivers/scsi/lpfc/lpfc_scsi.c:1053 lpfc_get_scsi_buf_s4()
warn: variable dereferenced before check 'lpfc_cmd' (see line 1039)

Fortunately the NULL check isn't required so I have removed it.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
index 5c7858e735c9..ef20d37e44db 100644
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -1050,7 +1050,7 @@ lpfc_get_scsi_buf_s4(struct lpfc_hba *phba, struct 
lpfc_nodelist *ndlp)
if (!found)
return NULL;
 
-   if (lpfc_ndlp_check_qdepth(phba, ndlp) && lpfc_cmd) {
+   if (lpfc_ndlp_check_qdepth(phba, ndlp)) {
atomic_inc(>cmd_pending);
lpfc_cmd->flags |= LPFC_SBUF_BUMP_QDEPTH;
}

[PATCH v2] scsi: aic94xx: fix an error code in aic94xx_init()

[Dan Carpenter]

We accidentally return success instead of -ENOMEM on this error path.

Fixes: 2908d778ab3e ("[SCSI] aic94xx: new driver")
Signed-off-by: Dan Carpenter 
---
v2: return -ENOMEM instead of -ENODEV

diff --git a/drivers/scsi/aic94xx/aic94xx_init.c 
b/drivers/scsi/aic94xx/aic94xx_init.c
index 80e5b283fd81..cb8191afc1dc 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -1030,8 +1030,10 @@ static int __init aic94xx_init(void)
 
aic94xx_transport_template =
sas_domain_attach_transport(_transport_functions);
-   if (!aic94xx_transport_template)
+   if (!aic94xx_transport_template) {
+   err = -ENOMEM;
goto out_destroy_caches;
+   }
 
err = pci_register_driver(_pci_driver);
if (err)

Re: [PATCH] scsi: aic94xx: fix an error code in aic94xx_init()

[Dan Carpenter]

On Wed, Aug 08, 2018 at 03:16:57PM +0100, John Garry wrote:
> On 08/08/2018 12:56, Dan Carpenter wrote:
> > We accidentally return success instead of -ENODEV on this error path.
> 
> Sorry to nitpick, but - as I see - the only way for
> sas_domain_attach_transport() to fail is if the kzalloc() in
> sas_attach_transport() fails, so should this be -ENOMEM? Other drivers
> return this error code for this scenario.
> 

Huh...  I was so sure I looked up other callers to see what people
normally return...  Anyway, you're right.  Let me resend.

regards,
dan carpenter

[PATCH] scsi: aic94xx: fix an error code in aic94xx_init()

[Dan Carpenter]

We accidentally return success instead of -ENODEV on this error path.

Fixes: 2908d778ab3e ("[SCSI] aic94xx: new driver")
Signed-off-by: Dan Carpenter 

diff --git a/drivers/scsi/aic94xx/aic94xx_init.c 
b/drivers/scsi/aic94xx/aic94xx_init.c
index 80e5b283fd81..cb8191afc1dc 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -1030,8 +1030,10 @@ static int __init aic94xx_init(void)
 
aic94xx_transport_template =
sas_domain_attach_transport(_transport_functions);
-   if (!aic94xx_transport_template)
+   if (!aic94xx_transport_template) {
+   err = -ENODEV;
goto out_destroy_caches;
+   }
 
err = pci_register_driver(_pci_driver);
if (err)

[bug report] scsi: qla2xxx: Migrate NVME N2N handling into state machine

[Dan Carpenter]

Hello Quinn Tran,

This is a semi-automatic email about new static checker warnings.

The patch 8777e4314d39: "scsi: qla2xxx: Migrate NVME N2N handling 
into state machine" from Aug 2, 2018, leads to the following Smatch 
complaint:

drivers/scsi/qla2xxx/qla_iocb.c:2647 qla2x00_els_dcmd2_sp_done()
 error: we previously assumed 'e' could be null (see line 2631)

drivers/scsi/qla2xxx/qla_iocb.c
  2630  e = qla2x00_alloc_work(vha, QLA_EVT_UNMAP);
  2631  if (!e) {
^
New check for NULL

  2632  struct srb_iocb *elsio = >u.iocb_cmd;
  2633  
  2634  if (elsio->u.els_plogi.els_plogi_pyld)
  2635  
dma_free_coherent(>vha->hw->pdev->dev,
  2636  elsio->u.els_plogi.tx_size,
  2637  elsio->u.els_plogi.els_plogi_pyld,
  2638  
elsio->u.els_plogi.els_plogi_pyld_dma);
  2639  
  2640  if (elsio->u.els_plogi.els_resp_pyld)
  2641  
dma_free_coherent(>vha->hw->pdev->dev,
  2642  elsio->u.els_plogi.rx_size,
  2643  elsio->u.els_plogi.els_resp_pyld,
  2644  
elsio->u.els_plogi.els_resp_pyld_dma);
  2645  sp->free(sp);
  2646  }
  2647  e->u.iosb.sp = sp;

Dereference without checking

  2648      qla2x00_post_work(vha, e);
  2649  }

regards,
dan carpenter

[bug report] scsi: libfc: Add lockdep annotations

[Dan Carpenter]

Hello Hannes Reinecke,

The patch ee35624e1e4e: "scsi: libfc: Add lockdep annotations" from
Jul 4, 2018, leads to the following static checker warning:

drivers/scsi/libfc/fc_rport.c:1838 fc_rport_recv_plogi_req()
error: uninitialized symbol 'rdata'.

drivers/scsi/libfc/fc_rport.c
  1828  static void fc_rport_recv_plogi_req(struct fc_lport *lport,
  1829  struct fc_frame *rx_fp)
  1830  {
  1831  struct fc_disc *disc;
  1832  struct fc_rport_priv *rdata;
  ^
  1833  struct fc_frame *fp = rx_fp;
  1834  struct fc_els_flogi *pl;
  1835  struct fc_seq_els_data rjt_data;
  1836  u32 sid;
  1837  
  1838  lockdep_assert_held(>rp_mutex);
 ^^^
This isn't initialized.

  1839  
  1840  sid = fc_frame_sid(fp);
  1841  
  1842  FC_RPORT_ID_DBG(lport, sid, "Received PLOGI request\n");
  1843  
  1844  pl = fc_frame_payload_get(fp, sizeof(*pl));
  1845  if (!pl) {
  1846  FC_RPORT_ID_DBG(lport, sid, "Received PLOGI too 
short\n");
  1847  rjt_data.reason = ELS_RJT_PROT;
  1848  rjt_data.explan = ELS_EXPL_INV_LEN;
  1849  goto reject;
  1850  }
  1851  
  1852  disc = >disc;
  1853  mutex_lock(>disc_mutex);
  1854  rdata = fc_rport_create(lport, sid);
^^^
  1855  if (!rdata) {
^^
  1856  mutex_unlock(>disc_mutex);
  1857  rjt_data.reason = ELS_RJT_UNAB;
  1858  rjt_data.explan = ELS_EXPL_INSUF_RES;
  1859  goto reject;
  1860  }

regards,
dan carpenter

[PATCH] scsi: qedi: tidy up a size caculation

[Dan Carpenter]

The id_tbl->table pointer points to unsigned long so static checkers
complain that instead of 4 we should be allocating sizeof(long) bytes.

We're trying to allocate enough bits for the bitmap.  The size variable
is always 1024.  (1024 / 32 * 4) is the same as (1024 / 64 * 8) so this
doesn't change runtime, but this is the more idiomatic way to do it and
makes the static checker happy.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
index cf274a79e77a..682f3ce31014 100644
--- a/drivers/scsi/qedi/qedi_main.c
+++ b/drivers/scsi/qedi/qedi_main.c
@@ -524,7 +524,7 @@ static int qedi_init_id_tbl(struct qedi_portid_tbl *id_tbl, 
u16 size,
id_tbl->max = size;
id_tbl->next = next;
spin_lock_init(_tbl->lock);
-   id_tbl->table = kcalloc(DIV_ROUND_UP(size, 32), 4, GFP_KERNEL);
+   id_tbl->table = kcalloc(BITS_TO_LONGS(size), sizeof(long), GFP_KERNEL);
if (!id_tbl->table)
return -ENOMEM;
 

Re: [PATCH 2/3] tcmu: track nl commands

[Dan Carpenter]

Hi Mike,

Thank you for the patch! Perhaps something to improve:

url:
https://github.com/0day-ci/linux/commits/Mike-Christie/tcmu-fix-hung-netlink-requests-during-restarts/20180622-115832

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

# 
https://github.com/0day-ci/linux/commit/0921da9c695fe2502a0d25b7758f4c93249148d7
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 0921da9c695fe2502a0d25b7758f4c93249148d7
vim +301 drivers/target/target_core_user.c

b3af66e2 Mike Christie 2017-06-23  276  
b3af66e2 Mike Christie 2017-06-23  277  static int tcmu_genl_cmd_done(struct 
genl_info *info, int completed_cmd)
b3af66e2 Mike Christie 2017-06-23  278  {
0921da9c Mike Christie 2018-06-21  279  struct tcmu_dev *udev = NULL;
b3af66e2 Mike Christie 2017-06-23  280  struct tcmu_nl_cmd *nl_cmd;
b3af66e2 Mike Christie 2017-06-23  281  int dev_id, rc, ret = 0;
b3af66e2 Mike Christie 2017-06-23  282  
b3af66e2 Mike Christie 2017-06-23  283  if 
(!info->attrs[TCMU_ATTR_CMD_STATUS] ||
b3af66e2 Mike Christie 2017-06-23  284  
!info->attrs[TCMU_ATTR_DEVICE_ID]) {
b3af66e2 Mike Christie 2017-06-23  285  printk(KERN_ERR 
"TCMU_ATTR_CMD_STATUS or TCMU_ATTR_DEVICE_ID not set, doing nothing\n");
b3af66e2 Mike Christie 2017-06-23  286  return -EINVAL;
b3af66e2 Mike Christie 2017-06-23  287  }
b3af66e2 Mike Christie 2017-06-23  288  
b3af66e2 Mike Christie 2017-06-23  289  dev_id = 
nla_get_u32(info->attrs[TCMU_ATTR_DEVICE_ID]);
b3af66e2 Mike Christie 2017-06-23  290  rc = 
nla_get_s32(info->attrs[TCMU_ATTR_CMD_STATUS]);
b3af66e2 Mike Christie 2017-06-23  291  
0921da9c Mike Christie 2018-06-21  292  mutex_lock(_nl_cmd_mutex);
0921da9c Mike Christie 2018-06-21  293  list_for_each_entry(nl_cmd, 
_nl_cmd_list, nl_list) {
0921da9c Mike Christie 2018-06-21  294  if 
(nl_cmd->udev->se_dev.dev_index == dev_id) {
0921da9c Mike Christie 2018-06-21  295  udev = 
nl_cmd->udev;
0921da9c Mike Christie 2018-06-21  296  break;
0921da9c Mike Christie 2018-06-21  297  }
b3af66e2 Mike Christie 2017-06-23  298  }
b3af66e2 Mike Christie 2017-06-23  299  
0921da9c Mike Christie 2018-06-21  300  if (!udev) {
0921da9c Mike Christie 2018-06-21 @301  pr_err(KERN_ERR "tcmu 
nl cmd %u/%d completion could not find device with dev id %u.\n",
   
Not required since this is already pr_err().

0921da9c Mike Christie 2018-06-21  302 completed_cmd, 
rc, dev_id);
0921da9c Mike Christie 2018-06-21  303  ret = -ENODEV;
0921da9c Mike Christie 2018-06-21  304  goto unlock;
0921da9c Mike Christie 2018-06-21  305  }
0921da9c Mike Christie 2018-06-21  306  list_del(_cmd->nl_list);
b3af66e2 Mike Christie 2017-06-23  307  
0921da9c Mike Christie 2018-06-21  308  pr_debug("%s genl cmd done got 
id %d curr %d done %d rc %d\n",
0921da9c Mike Christie 2018-06-21  309   udev->name, dev_id, 
nl_cmd->cmd, completed_cmd, rc);
b3af66e2 Mike Christie 2017-06-23  310  
b3af66e2 Mike Christie 2017-06-23  311  if (nl_cmd->cmd != 
completed_cmd) {
0921da9c Mike Christie 2018-06-21  312  pr_err("Mismatched 
commands on %s (Expecting reply for %d. Current %d).\n",
0921da9c Mike Christie 2018-06-21  313 udev->name, 
completed_cmd, nl_cmd->cmd);
b3af66e2 Mike Christie 2017-06-23  314  ret = -EINVAL;
0921da9c Mike Christie 2018-06-21  315  goto unlock;
b3af66e2 Mike Christie 2017-06-23  316  }
b3af66e2 Mike Christie 2017-06-23  317  
0921da9c Mike Christie 2018-06-21  318  nl_cmd->status = rc;
b3af66e2 Mike Christie 2017-06-23  319  complete(_cmd->complete);
0921da9c Mike Christie 2018-06-21  320  unlock:
0921da9c Mike Christie 2018-06-21  321  
mutex_unlock(_nl_cmd_mutex);
b3af66e2 Mike Christie 2017-06-23  322  return ret;
b3af66e2 Mike Christie 2017-06-23  323  }
b3af66e2 Mike Christie 2017-06-23  324  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

Re: qla2xxx and smatch warnings about uninitialized variables

[Dan Carpenter]

On Fri, Jun 22, 2018 at 08:43:21AM -0700, Bart Van Assche wrote:
> Hello Himanshu,
> 
> The latest smatch version reports a large number of warnings about
> uninitialized variables for the qla2xxx driver. I think these warnings
> indicate real bugs. Can you have a look at these warnings?
> 

Or we could silence a lot of them by adding "qla8044_rd_reg_indirect 2"
to the smatch_data/kernel.ignore_uninitialized_param file.

regards,
dan carpenter

[bug report] qedi: Add support for populating ethernet TLVs.

[Dan Carpenter]

Hello Manish Rangankar,

The patch 534bbdf8832a: "qedi: Add support for populating ethernet
TLVs." from May 22, 2018, leads to the following static checker
warning:

drivers/scsi/qedi/qedi_main.c:891 qedi_get_boot_tgt_info()
error: snprintf() is printing too much 256 vs 255

drivers/scsi/qedi/qedi_main.c
   883  static void qedi_get_boot_tgt_info(struct nvm_iscsi_block *block,
   884 struct qedi_boot_target *tgt, u8 
index)
   885  {
   886  u32 ipv6_en;
   887  
   888  ipv6_en = !!(block->generic.ctrl_flags &
   889   NVM_ISCSI_CFG_GEN_IPV6_ENABLED);
   890  
   891  snprintf(tgt->iscsi_name, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN, 
"%s\n",
 ^^^  
The buffer is one char smaller than NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN.

   892   block->target[index].target_name.byte);
   893  
   894  tgt->ipv6_en = ipv6_en;
   895  
   896  if (ipv6_en)
   897  snprintf(tgt->ip_addr, IPV6_LEN, "%pI6\n",
   898   block->target[index].ipv6_addr.byte);
   899  else
   900  snprintf(tgt->ip_addr, IPV4_LEN, "%pI4\n",
   901       block->target[index].ipv4_addr.byte);
   902  }

regards,
dan carpenter

[bug report] csiostor:T5 Firmware fix and cleanup.

[Dan Carpenter]

Hello Praveen Madhavan,

The patch 216ce69c7ff5: "csiostor:T5 Firmware fix and cleanup." from
Jan 27, 2015, leads to the following static checker warning:

drivers/scsi/csiostor/csio_hw.c:2006 csio_hw_use_fwconfig()
warn: was negative '-ENOENT' intended?

drivers/scsi/csiostor/csio_hw.c:2433 csio_hw_configure()
warn: 'rv' is never (-2)

drivers/scsi/csiostor/csio_hw.c
  1994  if (csio_mb_issue(hw, mbp)) {
  1995  rv = -EINVAL;
  1996  goto bye;
  1997  }
  1998  
  1999  rv = csio_mb_fw_retval(mbp);
^^^
The first warning is a false positive because for some reason Smatch
thinks that csio_mb_fw_retval() always returns zero.  But actually it
returns enum fw_retval.

  2000   /* If the CAPS_CONFIG failed with an ENOENT (for a Firmware
  ^^
So this should be FW_ENOENT.

  2001* Configuration File in FLASH), our last gasp effort is to 
use the
  2002* Firmware Configuration File which is embedded in the
  2003* firmware.  A very few early versions of the firmware didn't
  2004* have one embedded but we can ignore those.
  2005*/
  2006  if (rv == ENOENT) {

And this as well.  It doesn't affect runtime because ENOENT AND FW_ENOENT
are both 2.

  2007  CSIO_INIT_MBP(mbp, caps_cmd, CSIO_MB_DEFAULT_TMO, hw, 
NULL, 1);
  2008  caps_cmd->op_to_write = 
htonl(FW_CMD_OP_V(FW_CAPS_CONFIG_CMD) |
  2009FW_CMD_REQUEST_F |
  2010FW_CMD_READ_F);
  2011  caps_cmd->cfvalid_to_len16 = htonl(FW_LEN16(*caps_cmd));
  2012  
  2013  if (csio_mb_issue(hw, mbp)) {
  2014  rv = -EINVAL;
  2015  goto bye;
  2016  }
  2017  
  2018  rv = csio_mb_fw_retval(mbp);
^^^
  2019  config_name = "Firmware Default";
  2020  }
  2021  if (rv != FW_SUCCESS)

But then this function seems to return a mix of negative kernel error
codes and positive FW_ error codes.  There is a static checker warning
in the caller because we're checking for -ENOENT and Smatch thinks that
isn't possible.  Perhaps we should be checking for positive FW_ENOENT?

It's not super harmful because it only affects which warning is printed.

  2022  goto bye;
  2023  

regards,
dan carpenter

Re: [PATCH] scsi: qlogicpti: Fix an error handling path in 'qpti_sbus_probe()'

[Dan Carpenter]

On Thu, May 10, 2018 at 01:45:58PM +0200, Christophe JAILLET wrote:
> The 'free_irq()' call is not at the right place in the error handling path.
> The changed order has been introduced in commit 3d4253d9afab
> ("[SCSI] qlogicpti: Convert to new SBUS device framework.")
> 
> Fixes: 3d4253d9afab ("[SCSI] qlogicpti: Convert to new SBUS device 
> framework.")
> Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr>
> ---
> Please review carefully.  This patch is proposed because it triggers one of
> my coccinelle scripts. I'm not 100% sure if correct.

Looks good to me.

Reviewed-by: Dan Carpenter <dan.carpen...@oracle.com>

regards,
dan carpenter

[bug report] scsi: qlogicfas: move bus_reset to host_reset

[Dan Carpenter]

[ This one is not really your fault but you renamed things so it's
  showing up as a new warning.  - dan ]

Hello Hannes Reinecke,

The patch 4a56c1c166b6: "scsi: qlogicfas: move bus_reset to
host_reset" from Aug 25, 2017, leads to the following static checker
warning:

drivers/scsi/pcmcia/qlogic_stub.c:267 qlogic_resume()
error: NULL dereference inside function 'qlogicfas408_host_reset((0))()'

drivers/scsi/pcmcia/qlogic_stub.c
   254  static int qlogic_resume(struct pcmcia_device *link)
   255  {
   256  scsi_info_t *info = link->priv;
   257  
   258  pcmcia_enable_device(link);
   259  if ((info->manf_id == MANFID_MACNICA) ||
   260  (info->manf_id == MANFID_PIONEER) ||
   261  (info->manf_id == 0x0098)) {
   262  outb(0x80, link->resource[0]->start + 0xd);
   263  outb(0x24, link->resource[0]->start + 0x9);
   264  outb(0x04, link->resource[0]->start + 0xd);
   265  }
   266  /* Ulll!!! */
   267  qlogicfas408_host_reset(NULL);

qlogicfas408_host_reset() doesn't take NULL pointers, it's just going to
crash.

   268  
   269      return 0;
   270  }

regards,
dan carpenter

[PATCH] scsi: megaraid: silence a static checker bug

[Dan Carpenter]

If we had more than 32 megaraid cards then it would cause memory
corruption.  That's not likely, of course, but it's handy to enforce it
and make the static checker happy.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 7195cff51d4c..9b6f5d024dba 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -4199,6 +4199,9 @@ megaraid_probe_one(struct pci_dev *pdev, const struct 
pci_device_id *id)
int irq, i, j;
int error = -ENODEV;
 
+   if (hba_count >= MAX_CONTROLLERS)
+   goto out;
+
if (pci_enable_device(pdev))
goto out;
pci_set_master(pdev);

Re: [PATCH 03/39] proc: introduce proc_create_seq_private

[Dan Carpenter]

On Thu, Apr 19, 2018 at 02:41:04PM +0200, Christoph Hellwig wrote:
> diff --git a/drivers/s390/cio/blacklist.c b/drivers/s390/cio/blacklist.c
> index 2a3f874a21d5..ad35cddcf6af 100644
> --- a/drivers/s390/cio/blacklist.c
> +++ b/drivers/s390/cio/blacklist.c
> @@ -391,28 +391,15 @@ static const struct seq_operations 
> cio_ignore_proc_seq_ops = {
>   .show  = cio_ignore_proc_seq_show,
>  };
>  
> -static int
> -cio_ignore_proc_open(struct inode *inode, struct file *file)
> -{
> - return seq_open_private(file, _ignore_proc_seq_ops,
> - sizeof(struct ccwdev_iter));
> -}
> -
> -static const struct file_operations cio_ignore_proc_fops = {
> - .open= cio_ignore_proc_open,
> - .read= seq_read,
> - .llseek  = seq_lseek,
> - .release = seq_release_private,
> - .write   = cio_ignore_write,
   
The cio_ignore_write() function isn't used any more so compilers will
complain.

> -};
> -
>  static int
>  cio_ignore_proc_init (void)
>  {
>   struct proc_dir_entry *entry;
>  
> - entry = proc_create("cio_ignore", S_IFREG | S_IRUGO | S_IWUSR, NULL,
> - _ignore_proc_fops);
> + entry = proc_create_seq_private("cio_ignore",
> + S_IFREG | S_IRUGO | S_IWUSR, NULL,
> + _ignore_proc_seq_ops, sizeof(struct ccwdev_iter),
> + NULL);
>   if (!entry)
>   return -ENOENT;
>   return 0;

regards,
dan carpenter

[PATCH v2] scsi: cxgb4i: silence overflow warning in t4_uld_rx_handler()

[Dan Carpenter]

Smatch marks skb->data as untrusted so it complains that there is a
potential overflow here:

drivers/scsi/cxgbi/cxgb4i/cxgb4i.c:2111 t4_uld_rx_handler()
error: buffer overflow 'cxgb4i_cplhandlers' 239 <= 255.

In this case, skb->data comes from the hardware or firmware so it's not
going to overflow unless there is a firmware bug.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
---
v2: rebase, and re-write commit message

diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c 
b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
index 406e94312d4e..beb146b7c17c 100644
--- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
+++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
@@ -2108,11 +2108,11 @@ static int t4_uld_rx_handler(void *handle, const __be64 
*rsp,
log_debug(1 << CXGBI_DBG_TOE,
"cdev %p, opcode 0x%x(0x%x,0x%x), skb %p.\n",
 cdev, opc, rpl->ot.opcode_tid, ntohl(rpl->ot.opcode_tid), skb);
-   if (cxgb4i_cplhandlers[opc])
-   cxgb4i_cplhandlers[opc](cdev, skb);
-   else {
+   if (opc >= ARRAY_SIZE(cxgb4i_cplhandlers) || !cxgb4i_cplhandlers[opc]) {
pr_err("No handler for opcode 0x%x.\n", opc);
__kfree_skb(skb);
+   } else {
+   cxgb4i_cplhandlers[opc](cdev, skb);
}
return 0;
 nomem:

Re: [PATCH] scsi: cxgb4i: potential array overflow in t4_uld_rx_handler()

[Dan Carpenter]

On Wed, Mar 28, 2018 at 09:14:25PM +0530, Varun Prakash wrote:
> On Wed, Mar 21, 2018 at 09:12:00PM -0400, Martin K. Petersen wrote:
> > 
> > Varun: Please look at this. Thanks!
> > 
> > > What happened to this one?
> > >
> > > regards,
> > > dan carpenter
> > >
> > >
> > > On Wed, Nov 29, 2017 at 02:42:20PM +0300, Dan Carpenter wrote:
> > >> The story is that Smatch marks skb->data as untrusted and so it
> > >> complains about this code:
> > >> 
> > >>  drivers/scsi/cxgbi/cxgb4i/cxgb4i.c:2111 t4_uld_rx_handler()
> > >>  error: buffer overflow 'cxgb4i_cplhandlers' 239 <= 255.
> > >> 
> > >> I don't know the code very well, but it looks like a reasonable warning
> > >> message.  Let's address it by adding a sanity check to make sure "opc"
> > >> is within bounds.
> > >> 
> > >> Fixes: bbc02c7e9d34 ("cxgb4: Add register, message, and FW definitions")
> > >> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> > >> 
> > >> diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c 
> > >> b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> > >> index 266eddf17a99..94b2d5660a07 100644
> > >> --- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> > >> +++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> > >> @@ -2108,12 +2108,12 @@ static int t4_uld_rx_handler(void *handle, const 
> > >> __be64 *rsp,
> > >>  log_debug(1 << CXGBI_DBG_TOE,
> > >>  "cdev %p, opcode 0x%x(0x%x,0x%x), skb %p.\n",
> > >>   cdev, opc, rpl->ot.opcode_tid, 
> > >> ntohl(rpl->ot.opcode_tid), skb);
> > >> -if (cxgb4i_cplhandlers[opc])
> > >> -cxgb4i_cplhandlers[opc](cdev, skb);
> > >> -else {
> > >> +if (opc >= ARRAY_SIZE(cxgb4i_cplhandlers) || 
> > >> !cxgb4i_cplhandlers[opc]) {
> > >>  pr_err("No handler for opcode 0x%x.\n", opc);
> > >>  __kfree_skb(skb);
> > >> +return 0;
> > >>  }
> > >> +cxgb4i_cplhandlers[opc](cdev, skb);
> > >>  return 0;
> > >>  nomem:
> > >>  log_debug(1 << CXGBI_DBG_TOE, "OOM bailing out.\n");
> > >
> > >
> 
> This check is not necessary but we can add it to avoid warning.

Is the reason it's not necessary, because the skb->data comes from the
firmware and we trust it?  The v5 declares the array as having 256
elements which also solves this warning.  And cxgbit_uld_lro_rx_handler()
has a bounds check.  So it seems pretty normal to prevent the array
overflow by force as well as by trust.

> The commit mentioned in "Fixes" is not correct, this code was added in commit 
> "7b36b6e [SCSI] cxgb4i v5: iscsi driver"

Yeah.  You're right.  I can resend with an updated commit message.

regards,
dan carpenter

[PATCH] scsi: dpt_i2o: Use after free in I2ORESETCMD ioctl

[Dan Carpenter]

Here is another use after free if we reset the card.  The adpt_hba_reset()
function frees "pHba" on error.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index 3c667b23a801..359e0acfbc7c 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -2046,13 +2046,16 @@ static int adpt_ioctl(struct inode *inode, struct file 
*file, uint cmd, ulong ar
}
break;
}
-   case I2ORESETCMD:
-   if(pHba->host)
-   spin_lock_irqsave(pHba->host->host_lock, flags);
+   case I2ORESETCMD: {
+   struct Scsi_Host *shost = pHba->host;
+
+   if (shost)
+   spin_lock_irqsave(shost->host_lock, flags);
adpt_hba_reset(pHba);
-   if(pHba->host)
-   spin_unlock_irqrestore(pHba->host->host_lock, flags);
+   if (shost)
+   spin_unlock_irqrestore(shost->host_lock, flags);
break;
+   }
case I2ORESCANCMD:
adpt_rescan(pHba);
break;

Re: [PATCH 1/2] scsi: dpt_i2o: use after free in adpt_release()

[Dan Carpenter]

Yeah.  You're right.  Thanks for catching that.

regards,
dan carpenter

[PATCH 1/2] scsi: dpt_i2o: use after free in adpt_release()

[Dan Carpenter]

The scsi_host_put() function frees "pHba" and then we dereference it on
the next line when we do "scsi_host_put(pHba->host);".

Fixes: 38e09e3bb056 ("scsi: dpt_i2o: stop using scsi_unregister")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index 3c667b23a801..ac2f40d9963b 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -306,8 +306,8 @@ static void adpt_release(adpt_hba *pHba)
 {
scsi_remove_host(pHba->host);
 // adpt_i2o_quiesce_hba(pHba);
-   adpt_i2o_delete_hba(pHba);
scsi_host_put(pHba->host);
+   adpt_i2o_delete_hba(pHba);
 }
 
 

Re: [PATCH] scsi: cxgb4i: potential array overflow in t4_uld_rx_handler()

[Dan Carpenter]

What happened to this one?

regards,
dan carpenter


On Wed, Nov 29, 2017 at 02:42:20PM +0300, Dan Carpenter wrote:
> The story is that Smatch marks skb->data as untrusted and so it
> complains about this code:
> 
>   drivers/scsi/cxgbi/cxgb4i/cxgb4i.c:2111 t4_uld_rx_handler()
>   error: buffer overflow 'cxgb4i_cplhandlers' 239 <= 255.
> 
> I don't know the code very well, but it looks like a reasonable warning
> message.  Let's address it by adding a sanity check to make sure "opc"
> is within bounds.
> 
> Fixes: bbc02c7e9d34 ("cxgb4: Add register, message, and FW definitions")
> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> 
> diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c 
> b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> index 266eddf17a99..94b2d5660a07 100644
> --- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> +++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
> @@ -2108,12 +2108,12 @@ static int t4_uld_rx_handler(void *handle, const 
> __be64 *rsp,
>   log_debug(1 << CXGBI_DBG_TOE,
>   "cdev %p, opcode 0x%x(0x%x,0x%x), skb %p.\n",
>cdev, opc, rpl->ot.opcode_tid, ntohl(rpl->ot.opcode_tid), skb);
> - if (cxgb4i_cplhandlers[opc])
> - cxgb4i_cplhandlers[opc](cdev, skb);
> - else {
> + if (opc >= ARRAY_SIZE(cxgb4i_cplhandlers) || !cxgb4i_cplhandlers[opc]) {
>   pr_err("No handler for opcode 0x%x.\n", opc);
>   __kfree_skb(skb);
> + return 0;
>   }
> + cxgb4i_cplhandlers[opc](cdev, skb);
>   return 0;
>  nomem:
>   log_debug(1 << CXGBI_DBG_TOE, "OOM bailing out.\n");

[bug report] scsi: lpfc: Add WQ Full Logic for NVME Target

[Dan Carpenter]

Hello James Smart,

The patch 6e8e1c14c61e: "scsi: lpfc: Add WQ Full Logic for NVME
Target" from Jan 30, 2018, leads to the following static checker
warning:

drivers/scsi/lpfc/lpfc_nvmet.c:921 lpfc_nvmet_xmt_fcp_abort()
warn: inconsistent returns 'ctxp->ctxlock'.

drivers/scsi/lpfc/lpfc_nvmet.c
   889  atomic_inc(_nvmep->xmt_fcp_abort);
   890  
   891  spin_lock_irqsave(>ctxlock, flags);
   892  ctxp->state = LPFC_NVMET_STE_ABORT;
   893  
   894  /* Since iaab/iaar are NOT set, we need to check
   895   * if the firmware is in process of aborting IO
   896   */
   897  if (ctxp->flag & LPFC_NVMET_XBUSY) {
   898  spin_unlock_irqrestore(>ctxlock, flags);
   899  return;
   900  }
   901  ctxp->flag |= LPFC_NVMET_ABORT_OP;
   902  
   903  if (ctxp->flag & LPFC_NVMET_DEFER_WQFULL) {
   904  lpfc_nvmet_unsol_fcp_issue_abort(phba, ctxp, ctxp->sid,
   905   ctxp->oxid);
   906  wq = phba->sli4_hba.nvme_wq[ctxp->wqeq->hba_wqidx];
   907  lpfc_nvmet_wqfull_flush(phba, wq, ctxp);
   908  return;
^^^
Missing unlock before this return.

   909  }
   910  
   911  /* An state of LPFC_NVMET_STE_RCV means we have just received
   912   * the NVME command and have not started processing it.
   913   * (by issuing any IO WQEs on this exchange yet)
   914   */
   915  if (ctxp->state == LPFC_NVMET_STE_RCV)
   916  lpfc_nvmet_unsol_fcp_issue_abort(phba, ctxp, ctxp->sid,
   917   ctxp->oxid);
   918  else
   919  lpfc_nvmet_sol_fcp_issue_abort(phba, ctxp, ctxp->sid,
   920 ctxp->oxid);
   921      spin_unlock_irqrestore(>ctxlock, flags);
   922  }


regards,
dan carpenter

Re: [0/8] target-iSCSI: Adjustments for several function implementations

[Dan Carpenter]

On Fri, Feb 23, 2018 at 10:06:16AM +0100, SF Markus Elfring wrote:
> > Calling crypto_free_shash(NULL) is actually fine.
> 
> Really?
> 
> 
> > It doesn't dereference the parameter, it just does pointer math on it in
> > crypto_shash_tfm() and returns if it's NULL in crypto_destroy_tfm().
> 
> Can a passed null pointer really work in this function?
> 
> https://elixir.bootlin.com/linux/v4.16-rc2/source/include/crypto/hash.h#L684
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/crypto/hash.h?id=0f9da844d87796ac31b04e81ee95e155e9043132#n751
> 
> static inline struct crypto_tfm *crypto_shash_tfm(struct crypto_shash *tfm)
> {
>   return >base;
> }

Yes.  It's not a dereference, it's just doing pointer math to get the
address.

regards,
dan carpenter

[PATCH 2/2] scsi: myrs: bitwise vs logical OR typo

[Dan Carpenter]

We accidentally used a logical || instead of a | so these bit masks are
off.

Fixes: 8a8606895947 ("scsi: myrs: Add Mylex RAID controller (SCSI interface)")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
index eaa9d143a282..db3c84fc3a29 100644
--- a/drivers/scsi/myrs.c
+++ b/drivers/scsi/myrs.c
@@ -907,7 +907,7 @@ static void myrs_log_event(myrs_hba *cs, myrs_event *ev)
}
if (sshdr.sense_key == VENDOR_SPECIFIC &&
(sshdr.asc == 0x80 || sshdr.asc == 0x81))
-   ev->ev_code = ((sshdr.asc - 0x80) << 8 || sshdr.ascq);
+   ev->ev_code = ((sshdr.asc - 0x80) << 8 | sshdr.ascq);
while (true) {
ev_code = myrs_ev_list[ev_idx].ev_code;
if (ev_code == ev->ev_code || ev_code == 0)
@@ -2201,7 +2201,7 @@ static void myrs_handle_scsi(myrs_hba *cs, myrs_cmdblk 
*cmd_blk,
status == DAC960_V2_DeviceNonresponsive2)
scmd->result = (DID_BAD_TARGET << 16);
else
-   scmd->result = (DID_OK << 16) || status;
+   scmd->result = (DID_OK << 16) | status;
scmd->scsi_done(scmd);
 }
 

[PATCH 1/2] scsi: myrs: cleanup myrs_store_suppress_enclosure_messages()

[Dan Carpenter]

This code causes a static checker because we have an upper bound on
"value" but not a lower bound.  In other words "value" can be s32min-2.
It's harmless but really it should just be bool.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
index 3b87c6942a8e..eaa9d143a282 100644
--- a/drivers/scsi/myrs.c
+++ b/drivers/scsi/myrs.c
@@ -1590,15 +1590,12 @@ static ssize_t 
myrs_store_suppress_enclosure_messages(struct device *dev,
 {
struct scsi_device *sdev = to_scsi_device(dev);
myrs_hba *cs = (myrs_hba *)sdev->host->hostdata;
-   char tmpbuf[8];
-   ssize_t len;
-   int value;
+   bool value;
+   int ret;
 
-   len = count > sizeof(tmpbuf) - 1 ? sizeof(tmpbuf) - 1 : count;
-   strncpy(tmpbuf, buf, len);
-   tmpbuf[len] = '\0';
-   if (sscanf(tmpbuf, "%d", ) != 1 || value > 2)
-   return -EINVAL;
+   ret = kstrtobool(buf, );
+   if (ret)
+   return ret;
 
cs->disable_enc_msg = value;
return count;

[patch RESEND] atp870u: 64 bit bug in atp885_init()

[Dan Carpenter]

On 64 bit CPUs there is a memory corruption bug on probe().  It should
be a u32 pointer instead of an unsigned long pointer or we write past
the end of the setupdata[] array.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
Reviewed-by: Hannes Reinecke <h...@suse.com>
---
I was reviewing buffer overflow static checker warnings and came across
this ancient patch which was never applied.  Resending.

diff --git a/drivers/scsi/atp870u.c b/drivers/scsi/atp870u.c
index 8b52a9d..b46997c 100644
--- a/drivers/scsi/atp870u.c
+++ b/drivers/scsi/atp870u.c
@@ -1413,11 +1413,11 @@ static void atp885_init(struct Scsi_Host *shpnt)
atpdev->global_map[m] = 0;
for (k = 0; k < 4; k++) {
atp_writew_base(atpdev, 0x3c, n++);
-   ((unsigned long *)[m][0])[k] = 
atp_readl_base(atpdev, 0x38);
+   ((u32 *)[m][0])[k] = 
atp_readl_base(atpdev, 0x38);
}
for (k = 0; k < 4; k++) {
atp_writew_base(atpdev, 0x3c, n++);
-   ((unsigned long *)>sp[m][0])[k] = 
atp_readl_base(atpdev, 0x38);
+   ((u32 *)>sp[m][0])[k] = 
atp_readl_base(atpdev, 0x38);
}
n += 8;
}

[bug report] scsi: myrs: Add Mylex RAID controller (SCSI interface)

[Dan Carpenter]

Hello Hannes Reinecke,

The patch 8a8606895947: "scsi: myrs: Add Mylex RAID controller (SCSI
interface)" from Jan 24, 2018, leads to the following static checker
warning:

drivers/scsi/myrs.c:479 myrs_get_event()
warn: right shifting more than type allows 16 vs 16

drivers/scsi/myrs.c
   461  static unsigned char
   462  myrs_get_event(myrs_hba *cs, unsigned short event_num,
 

   463 myrs_event *event_buf)
   464  {
   465  struct pci_dev *pdev = cs->pdev;
   466  dma_addr_t event_addr;
   467  myrs_cmdblk *cmd_blk = >mcmd_blk;
   468  myrs_cmd_mbox *mbox = _blk->mbox;
   469  myrs_sgl *sgl;
   470  unsigned char status;
   471  
   472  event_addr = dma_map_single(>dev, event_buf,
   473  sizeof(myrs_event), 
DMA_FROM_DEVICE);
   474  if (dma_mapping_error(>dev, event_addr))
   475  return DAC960_V2_AbnormalCompletion;
   476  
   477  mbox->GetEvent.opcode = DAC960_V2_IOCTL;
   478  mbox->GetEvent.dma_size = sizeof(myrs_event);
   479  mbox->GetEvent.evnum_upper = event_num >> 16;
 ^^^

This is always going to be zero.

   480  mbox->GetEvent.ctlr_num = 0;
   481  mbox->GetEvent.ioctl_opcode = DAC960_V2_GetEvent;
   482  mbox->GetEvent.evnum_lower = event_num & 0x;
   483  sgl = >GetEvent.dma_addr;
   484  sgl->sge[0].sge_addr = event_addr;
   485  sgl->sge[0].sge_count = mbox->GetEvent.dma_size;
   486  myrs_exec_cmd(cs, cmd_blk);
   487  status = cmd_blk->status;
   488  dma_unmap_single(>dev, event_addr,
   489   sizeof(myrs_event), DMA_FROM_DEVICE);
   490  
   491  return status;
   492  }

This warning is probably a false positive which you can ignore but what
made me question it was looking at the caller:

drivers/scsi/myrs.c
    static void myrs_monitor(struct work_struct *work)
  2223  {
  2224  myrs_hba *cs = container_of(work, myrs_hba, monitor_work.work);
  2225  struct Scsi_Host *shost = cs->host;
  2226  myrs_ctlr_info *info = cs->ctlr_info;
  2227  unsigned int epoch = cs->fwstat_buf->epoch;
  2228  unsigned long interval = MYRS_PRIMARY_MONITOR_INTERVAL;
  2229  unsigned char status;
  2230  
  2231  dev_dbg(>shost_gendev, "monitor tick\n");
  2232  
  2233  status = myrs_get_fwstatus(cs);
  2234  
  2235  if (cs->needs_update) {
  2236  cs->needs_update = false;
  2237  mutex_lock(>cinfo_mutex);
  2238  status = myrs_get_ctlr_info(cs);
  2239  mutex_unlock(>cinfo_mutex);
  2240  }
  2241  if (cs->fwstat_buf->next_evseq - cs->next_evseq > 0) {
  2242  status = myrs_get_event(cs, cs->next_evseq,
^^
This is an int.

  2243  cs->event_buf);
  2244  if (status == DAC960_V2_NormalCompletion) {
  2245  myrs_log_event(cs, cs->event_buf);
  2246  cs->next_evseq++;

And I guess this is where we set cs->next_evseq.

  2247  interval = 1;
  2248  }
  2249  }

regards,
dan carpenter

[PATCH] mptfusion: Add bounds check in mptctl_hp_targetinfo()

[Dan Carpenter]

My static checker complains about an out of bounds read:

drivers/message/fusion/mptctl.c:2786 mptctl_hp_targetinfo()
error: buffer overflow 'hd->sel_timeout' 255 <= u32max.

It's true that we probably should have a bounds check here.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/message/fusion/mptctl.c b/drivers/message/fusion/mptctl.c
index 8d12017b9893..4470630dd545 100644
--- a/drivers/message/fusion/mptctl.c
+++ b/drivers/message/fusion/mptctl.c
@@ -2687,6 +2687,8 @@ mptctl_hp_targetinfo(unsigned long arg)
__FILE__, __LINE__, iocnum);
return -ENODEV;
}
+   if (karg.hdr.id >= MPT_MAX_FC_DEVICES)
+   return -EINVAL;
dctlprintk(ioc, printk(MYIOC_s_DEBUG_FMT "mptctl_hp_targetinfo 
called.\n",
ioc->name));
 

[PATCH] [SCSI] sym53c8xx_2: iterator underflow in sym_getsync()

[Dan Carpenter]

We wanted to exit the loop with "div" set to zero, but instead, if we
don't hit the break then "div" is -1 when we finish the loop.  It leads
to an array underflow a few lines later.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.c 
b/drivers/scsi/sym53c8xx_2/sym_hipd.c
index ca360daa6a25..378af306fda1 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.c
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.c
@@ -536,7 +536,7 @@ sym_getsync(struct sym_hcb *np, u_char dt, u_char sfac, 
u_char *divp, u_char *fa
 *  Look for the greatest clock divisor that allows an 
 *  input speed faster than the period.
 */
-   while (div-- > 0)
+   while (--div > 0)
if (kpc >= (div_10M[div] << 2)) break;
 
/*

[PATCH] scsi: storvsc: missing error code in storvsc_probe()

[Dan Carpenter]

We should set the error code if fc_remote_port_add() fails.

Fixes: daf0cd445a21 ("scsi: storvsc: Add support for FC rport.")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 3b3d1d050cac..40fc7a590e81 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1834,8 +1834,10 @@ static int storvsc_probe(struct hv_device *device,
fc_host_node_name(host) = stor_device->node_name;
fc_host_port_name(host) = stor_device->port_name;
stor_device->rport = fc_remote_port_add(host, 0, );
-   if (!stor_device->rport)
+   if (!stor_device->rport) {
+   ret = -ENOMEM;
goto err_out4;
+   }
}
 #endif
return 0;

[bug report] scsi: qla2xxx: Fix NULL pointer access for fcport structure

[Dan Carpenter]

Hello Quinn Tran,

This is a semi-automatic email about new static checker warnings.

The patch 5c25d451163c: "scsi: qla2xxx: Fix NULL pointer access for
fcport structure" from Dec 28, 2017, leads to the following Smatch
complaint:

drivers/scsi/qla2xxx/qla_init.c:130 qla2x00_async_iocb_timeout()
error: we previously assumed 'fcport' could be null (see line 107)

drivers/scsi/qla2xxx/qla_init.c
   106  
   107  if (fcport) {
^^^
The patch adds a check for NULL.

   108  ql_dbg(ql_dbg_disc, fcport->vha, 0x2071,
   109  "Async-%s timeout - hdl=%x portid=%06x %8phC.\n",
   110  sp->name, sp->handle, fcport->d_id.b24, 
fcport->port_name);
   111  
   112  fcport->flags &= ~(FCF_ASYNC_SENT | FCF_ASYNC_ACTIVE);
   113  } else {
   114  pr_info("Async-%s timeout - hdl=%x.\n",
   115  sp->name, sp->handle);
   116  }
   117  
   118  switch (sp->type) {
   119  case SRB_LOGIN_CMD:
   120  /* Retry as needed. */
   121  lio->u.logio.data[0] = MBS_COMMAND_ERROR;
   122  lio->u.logio.data[1] = lio->u.logio.flags & 
SRB_LOGIN_RETRIED ?
   123  QLA_LOGIO_LOGIN_RETRIED : 0;
   124  memset(, 0, sizeof(ea));
   125  ea.event = FCME_PLOGI_DONE;
   126  ea.fcport = sp->fcport;
   127  ea.data[0] = lio->u.logio.data[0];
   128  ea.data[1] = lio->u.logio.data[1];
   129  ea.sp = sp;
   130  qla24xx_handle_plogi_done_event(fcport->vha, );
^^^
But there is an unchecked NULL dereference here.

   131  break;
   132  case SRB_LOGOUT_CMD:

regards,
dan carpenter

[bug report] scsi: qla2xxx: Fix memory leak in dual/target mode

[Dan Carpenter]

Hello himanshu.madh...@cavium.com,

This is a semi-automatic email about new static checker warnings.

The patch 7867b98dceb7: "scsi: qla2xxx: Fix memory leak in
dual/target mode" from Dec 4, 2017, leads to the following Smatch
complaint:

drivers/scsi/qla2xxx/qla_mid.c:586 qla25xx_delete_req_que()
error: we previously assumed 'req' could be null (see line 580)

drivers/scsi/qla2xxx/qla_mid.c:602 qla25xx_delete_rsp_que()
error: we previously assumed 'rsp' could be null (see line 596)


drivers/scsi/qla2xxx/qla_mid.c
   579  
   580  if (req && vha->flags.qpairs_req_created) {
^^^
Check for NULL

   581  req->options |= BIT_0;
   582  ret = qla25xx_init_req_que(vha, req);
   583  if (ret != QLA_SUCCESS)
   584  return QLA_FUNCTION_FAILED;
   585  }
   586  qla25xx_free_req_que(vha, req);
  ^^^
Unchecked dereference inside function.

   587  
   588  return ret;

[ snip ]

   595  
   596  if (rsp && vha->flags.qpairs_rsp_created) {
^^^ 
  597   rsp->options |= BIT_0;
   598  ret = qla25xx_init_rsp_que(vha, rsp);
   599  if (ret != QLA_SUCCESS)
   600  return QLA_FUNCTION_FAILED;
   601  }
   602  qla25xx_free_rsp_que(vha, rsp);
  ^^^
   603  
   604      return ret;

regards,
dan carpenter

regards,
dan carpenter

[bug report] scsi: lpfc: Correct driver deregistrations with host nvme transport

[Dan Carpenter]

Hello James Smart,

This is a semi-automatic email about new static checker warnings.

The patch add9d6be3d65: "scsi: lpfc: Correct driver deregistrations
with host nvme transport" from Nov 20, 2017, leads to the following
Smatch complaint:

drivers/scsi/lpfc/lpfc_nvme.c:969 lpfc_nvme_io_cmd_wqe_cmpl()
error: we previously assumed 'ndlp' could be null (see line 938)

drivers/scsi/lpfc/lpfc_nvme.c
   937  
   938  if (ndlp && NLP_CHK_NODE_ACT(ndlp))

Existing code assumes ndlp can be NULL.

   939  atomic_dec(>cmd_pending);
   940  
   941  /* Update stats and complete the IO.  There is
   942   * no need for dma unprep because the nvme_transport
   943   * owns the dma address.
   944   */
   945  #ifdef CONFIG_SCSI_LPFC_DEBUG_FS
   946  if (lpfc_ncmd->ts_cmd_start) {
   947  lpfc_ncmd->ts_isr_cmpl = pwqeIn->isr_timestamp;
   948  lpfc_ncmd->ts_data_nvme = ktime_get_ns();
   949  phba->ktime_last_cmd = lpfc_ncmd->ts_data_nvme;
   950  lpfc_nvme_ktime(phba, lpfc_ncmd);
   951  }
   952  if (phba->cpucheck_on & LPFC_CHECK_NVME_IO) {
   953  if (lpfc_ncmd->cpu != smp_processor_id())
   954  lpfc_printf_vlog(vport, KERN_ERR, 
LOG_NVME_IOERR,
   955   "6701 CPU Check cmpl: "
   956   "cpu %d expect %d\n",
   957   smp_processor_id(), 
lpfc_ncmd->cpu);
   958  if (lpfc_ncmd->cpu < LPFC_CHECK_CPU_CNT)
   959  phba->cpucheck_cmpl_io[lpfc_ncmd->cpu]++;
   960  }
   961  #endif
   962  freqpriv = nCmd->private;
   963  freqpriv->nvme_buf = NULL;
   964  
   965  /* NVME targets need completion held off until the abort 
exchange
   966   * completes unless the NVME Rport is getting unregistered.
   967   */
   968  if (!(lpfc_ncmd->flags & LPFC_SBUF_XBUSY) ||
   969  ndlp->upcall_flags & NLP_WAIT_FOR_UNREG) {
^
The patch adds an unchecked dereference.

   970  /* Clear the XBUSY flag to prevent double completions.
   971   * The nvme rport is getting unregistered and there is

regards,
dan carpenter

[bug report] scsi: arcmsr: Add a function to set date and time to firmware

[Dan Carpenter]

Hello Ching Huang,

The patch b416c099472a: "scsi: arcmsr: Add a function to set date and
time to firmware" from Dec 5, 2017, leads to the following static
checker warning:

drivers/scsi/arcmsr/arcmsr_hba.c:3682 arcmsr_set_iop_datetime()
warn: unsigned 'secs' is never less than zero.

drivers/scsi/arcmsr/arcmsr_hba.c
  3658  static void arcmsr_set_iop_datetime(struct timer_list *t)
  3659  {
  3660  struct AdapterControlBlock *pacb = from_timer(pacb, t, 
refresh_timer);
  3661  unsigned int days, j, i, a, b, c, d, e, m, year, mon, day, 
hour, min, sec, secs, next_time;

  ^^^

  3662  struct timeval tv;
  3663  union {
  3664  struct  {
  3665  uint16_tsignature;
  3666  uint8_t year;
  3667  uint8_t month;
  3668  uint8_t date;
  3669  uint8_t hour;
  3670  uint8_t minute;
  3671  uint8_t second;
  3672  } a;
  3673  struct  {
  3674  uint32_tmsg_time[2];
  3675  } b;
  3676  } datetime;
  3677  
  3678  do_gettimeofday();
  3679  secs = (u32)(tv.tv_sec - (sys_tz.tz_minuteswest * 60));
  3680  days = secs / 86400;
  3681  secs = secs - 86400 * days;
  3682  if (secs < 0) {

Not possible.

  3683  days = days - 1;
  3684  secs = secs + 86400;
  3685  }
  3686  j = days / 146097;


regards,
dan carpenter

[PATCH] scsi: cxgb4i: potential array overflow in t4_uld_rx_handler()

[Dan Carpenter]

The story is that Smatch marks skb->data as untrusted and so it
complains about this code:

drivers/scsi/cxgbi/cxgb4i/cxgb4i.c:2111 t4_uld_rx_handler()
error: buffer overflow 'cxgb4i_cplhandlers' 239 <= 255.

I don't know the code very well, but it looks like a reasonable warning
message.  Let's address it by adding a sanity check to make sure "opc"
is within bounds.

Fixes: bbc02c7e9d34 ("cxgb4: Add register, message, and FW definitions")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c 
b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
index 266eddf17a99..94b2d5660a07 100644
--- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
+++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
@@ -2108,12 +2108,12 @@ static int t4_uld_rx_handler(void *handle, const __be64 
*rsp,
log_debug(1 << CXGBI_DBG_TOE,
"cdev %p, opcode 0x%x(0x%x,0x%x), skb %p.\n",
 cdev, opc, rpl->ot.opcode_tid, ntohl(rpl->ot.opcode_tid), skb);
-   if (cxgb4i_cplhandlers[opc])
-   cxgb4i_cplhandlers[opc](cdev, skb);
-   else {
+   if (opc >= ARRAY_SIZE(cxgb4i_cplhandlers) || !cxgb4i_cplhandlers[opc]) {
pr_err("No handler for opcode 0x%x.\n", opc);
__kfree_skb(skb);
+   return 0;
}
+   cxgb4i_cplhandlers[opc](cdev, skb);
return 0;
 nomem:
log_debug(1 << CXGBI_DBG_TOE, "OOM bailing out.\n");

Re: [PATCH 0/3] scsi: arcmsr: add driver module parameter - msi_enable, msix_enable

[Dan Carpenter]

On Fri, Nov 24, 2017 at 03:12:39AM +0800, Ching Huang wrote:
> On Thu, 2017-11-23 at 04:57 -0800, Christoph Hellwig wrote:
> > On Thu, Nov 23, 2017 at 09:22:03AM +0800, Ching Huang wrote:
> > > From: Ching Huang <ching2...@areca.com.tw>
> > > 
> > > Hi all,
> > > 
> > > The following patches apply to Martin's 4.16/scsi-queue.
> > > 
> > > Patch 1: Add module parameter msi_enable to has a chance to disable msi 
> > > interrupt if it does not work properly.
> > > 
> > > Patch 2: Add module parameter msix_enable to has a chance to disable msix 
> > > interrupt if it does not work properly.
> > 
> > Why would it not work properly?
> This patch is apply to
> https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/tree/?h=4.16/scsi-queue
>

No, the question is we're adding an option called "msi_enable" which is
used to disable MSI interrupts "if it does not work properly".  Why is
the current code not working properly?

Is there a crash or a performance issue?  What does the bug in the
current code look like from a user perspective?  Can you send us a dmesg
from a failing system?

regards,
dan carpenter

Re: [PATCH 2/3] scsi: arcmsr: Add driver module parameter msix_enable

[Dan Carpenter]

On Thu, Nov 23, 2017 at 09:31:14AM +0800, Ching Huang wrote:
> @@ -829,12 +833,15 @@ arcmsr_request_irq(struct pci_dev *pdev,
>   unsigned long flags;
>   int nvec, i;
>  
> + if (msix_enable == 0)
> + goto msi_int0;

I feel like this goto is not very beautiful, but I can't actually apply
this patch?  Which tree is this written against?  I'm using linux-next.

regards,
dan carpenter

Re: [PATCH 1/3] scsi: arcmsr: Add driver module parameter msi_enable

[Dan Carpenter]

On Thu, Nov 23, 2017 at 09:27:19AM +0800, Ching Huang wrote:
> From: Ching Huang <ching2...@areca.com.tw>
> 
> Add module parameter msi_enable to has a chance to disable msi interrupt if 
> it does not work properly.
> 
> Signed-off-by: Ching Huang <ching2...@areca.com.tw>
> ---
> 
> diff -uprN a/drivers/scsi/arcmsr/arcmsr_hba.c 
> b/drivers/scsi/arcmsr/arcmsr_hba.c
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c2017-11-23 14:29:26.0 
> +0800
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c2017-11-23 16:02:28.0 
> +0800
> @@ -75,6 +75,10 @@ MODULE_DESCRIPTION("Areca ARC11xx/12xx/1
>  MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_VERSION(ARCMSR_DRIVER_VERSION);
>  
> +static int msi_enable = 1;
> +module_param(msi_enable, int, S_IRUGO);
 ^^^
checkpatch.pl will complain that this should be 0444

> +MODULE_PARM_DESC(msi_enable, " Enable MSI interrupt(0 ~ 1), 
> msi_enable=1(enable), =0(disable)");
 ^
Remove the extra space

> +
>  static int host_can_queue = ARCMSR_DEFAULT_OUTSTANDING_CMD;
>  module_param(host_can_queue, int, S_IRUGO);
>  MODULE_PARM_DESC(host_can_queue, " adapter queue depth(32 ~ 1024), default 
> is 128");
> @@ -831,11 +835,15 @@ arcmsr_request_irq(struct pci_dev *pdev,
>   pr_info("arcmsr%d: msi-x enabled\n", acb->host->host_no);
>   flags = 0;
>   } else {
> - nvec = pci_alloc_irq_vectors(pdev, 1, 1,
> - PCI_IRQ_MSI | PCI_IRQ_LEGACY);
> + if (msi_enable == 1)
> + nvec = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> + else
> + nvec = pci_alloc_irq_vectors(pdev, 1, 1, 
> PCI_IRQ_LEGACY);
>   if (nvec < 1)
>   return FAILED;

I feel like we should try PCI_IRQ_MSI then if it fails we could fall
back to PCI_IRQ_LEGACY.  Originally, it worked like this and now it just
fails unless you toggle the module param.  It's a regression.

>  
> + if (msi_enable == 1)
> + pr_info("arcmsr%d: msi enabled\n", acb->host->host_no);

This printk could be improved.  Use dev_info(>dev, for a start.
I know that the other prints don't use this, but we could use it one
time then slowly add more users until more are using dev_info() than
pr_info() and then someone will decide to clean up the old users.

regards,
dan carpenter

[PATCH] scsi: lpfc: Use after free in lpfc_rq_buf_free()

[Dan Carpenter]

The error message dereferences "rqb_entry" so we need to print it first
and then free the buffer.

Fixes: 6c621a2229b0 ("scsi: lpfc: Separate NVMET RQ buffer posting from IO 
resources SGL/iocbq/context")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/lpfc/lpfc_mem.c b/drivers/scsi/lpfc/lpfc_mem.c
index 56faeb049b4a..87c08ff37ddd 100644
--- a/drivers/scsi/lpfc/lpfc_mem.c
+++ b/drivers/scsi/lpfc/lpfc_mem.c
@@ -753,12 +753,12 @@ lpfc_rq_buf_free(struct lpfc_hba *phba, struct 
lpfc_dmabuf *mp)
drqe.address_hi = putPaddrHigh(rqb_entry->dbuf.phys);
rc = lpfc_sli4_rq_put(rqb_entry->hrq, rqb_entry->drq, , );
if (rc < 0) {
-   (rqbp->rqb_free_buffer)(phba, rqb_entry);
lpfc_printf_log(phba, KERN_ERR, LOG_INIT,
"6409 Cannot post to RQ %d: %x %x\n",
rqb_entry->hrq->queue_id,
rqb_entry->hrq->host_index,
rqb_entry->hrq->hba_index);
+   (rqbp->rqb_free_buffer)(phba, rqb_entry);
} else {
list_add_tail(_entry->hbuf.list, >rqb_buffer_list);
rqbp->buffer_count++;

Re: [PATCH 4/4] qla2xxx_nvmet: Add FC-NVMe Target handling

[Dan Carpenter]

[ Ha ha.  The kbuild-bot automatically inserts complimentary things that
  "I love your patch."  In fact, I have not looked at your patch at all,
  I'm just forwarding this email from a robot after glancing at the
  code.  - dan carpenter ]

Hi Anil,

I love your patch! Perhaps something to improve:

[auto build test WARNING on scsi/for-next]
[also build test WARNING on next-20171110]
[cannot apply to v4.14]

url:
https://github.com/0day-ci/linux/commits/Himanshu-Madhani/qla2xxx-Add-FC-NVMe-Target-support/20171107-153645
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next

drivers/scsi/qla2xxx/qla_target.c:886 qlt_queue_purex() warn: taking sizeof 
binop
drivers/scsi/qla2xxx/qla_target.c:893 qlt_queue_purex() error: memcpy() 
'p->purex_pyld' too small (4 vs 44)

# 
https://github.com/0day-ci/linux/commit/9c5e24e821aa40552221b3103bc914bc4cd42293
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 9c5e24e821aa40552221b3103bc914bc4cd42293
vim +886 drivers/scsi/qla2xxx/qla_target.c

9c5e24e8 Anil Gurumurthy 2017-11-06  863  
9c5e24e8 Anil Gurumurthy 2017-11-06  864  static void 
qlt_queue_purex(scsi_qla_host_t *vha,
9c5e24e8 Anil Gurumurthy 2017-11-06  865struct atio_from_isp *atio)
9c5e24e8 Anil Gurumurthy 2017-11-06  866  {
9c5e24e8 Anil Gurumurthy 2017-11-06  867struct qla_tgt_purex_op *p;
9c5e24e8 Anil Gurumurthy 2017-11-06  868unsigned long flags;
9c5e24e8 Anil Gurumurthy 2017-11-06  869struct purex_entry_24xx *purex =
9c5e24e8 Anil Gurumurthy 2017-11-06  870(struct 
purex_entry_24xx *)>u.raw;
9c5e24e8 Anil Gurumurthy 2017-11-06  871uint16_t len = 
purex->frame_size;
9c5e24e8 Anil Gurumurthy 2017-11-06  872uint8_t *purex_pyld_tmp;
9c5e24e8 Anil Gurumurthy 2017-11-06  873  
9c5e24e8 Anil Gurumurthy 2017-11-06  874p = kzalloc(sizeof(*p), 
GFP_ATOMIC);
9c5e24e8 Anil Gurumurthy 2017-11-06  875if (p == NULL)
9c5e24e8 Anil Gurumurthy 2017-11-06  876goto out;
9c5e24e8 Anil Gurumurthy 2017-11-06  877  
9c5e24e8 Anil Gurumurthy 2017-11-06  878p->vha = vha;
9c5e24e8 Anil Gurumurthy 2017-11-06  879memcpy(>atio, atio, 
sizeof(*atio));
9c5e24e8 Anil Gurumurthy 2017-11-06  880  
9c5e24e8 Anil Gurumurthy 2017-11-06  881ql_dbg(ql_dbg_disc + 
ql_dbg_buffer, vha, 0xff11,
9c5e24e8 Anil Gurumurthy 2017-11-06  882"Dumping the Purex IOCB 
received\n");
9c5e24e8 Anil Gurumurthy 2017-11-06  883ql_dump_buffer(ql_dbg_disc + 
ql_dbg_buffer, vha, 0xe012,
9c5e24e8 Anil Gurumurthy 2017-11-06  884(uint8_t *)purex, 64);
9c5e24e8 Anil Gurumurthy 2017-11-06  885  
9c5e24e8 Anil Gurumurthy 2017-11-06 @886p->purex_pyld = 
kzalloc(sizeof(purex->entry_count * 64), GFP_ATOMIC);
9c5e24e8 Anil Gurumurthy 2017-11-06  887purex_pyld_tmp = (uint8_t 
*)p->purex_pyld;
9c5e24e8 Anil Gurumurthy 2017-11-06  888p->purex_pyld_len = len;
9c5e24e8 Anil Gurumurthy 2017-11-06  889  
9c5e24e8 Anil Gurumurthy 2017-11-06  890if (len < PUREX_PYLD_SIZE)
9c5e24e8 Anil Gurumurthy 2017-11-06  891len = PUREX_PYLD_SIZE;
9c5e24e8 Anil Gurumurthy 2017-11-06  892  
9c5e24e8 Anil Gurumurthy 2017-11-06 @893memcpy(p->purex_pyld, 
>d_id, PUREX_PYLD_SIZE);
9c5e24e8 Anil Gurumurthy 2017-11-06  894purex_pyld_tmp += 
PUREX_PYLD_SIZE;
9c5e24e8 Anil Gurumurthy 2017-11-06  895len -= PUREX_PYLD_SIZE;
9c5e24e8 Anil Gurumurthy 2017-11-06  896  
9c5e24e8 Anil Gurumurthy 2017-11-06  897while (len > 0) {
9c5e24e8 Anil Gurumurthy 2017-11-06  898int cpylen;
9c5e24e8 Anil Gurumurthy 2017-11-06  899struct __status_cont 
*cont_atio;
9c5e24e8 Anil Gurumurthy 2017-11-06  900  
9c5e24e8 Anil Gurumurthy 2017-11-06  901cont_atio = (struct 
__status_cont *)qlt_get_next_atio_pkt(vha);
9c5e24e8 Anil Gurumurthy 2017-11-06  902cpylen = len > 
CONT_SENSE_DATA ? CONT_SENSE_DATA : len;
9c5e24e8 Anil Gurumurthy 2017-11-06  903ql_log(ql_log_info, 
vha, 0xff12,
9c5e24e8 Anil Gurumurthy 2017-11-06  904"cont_atio: %p, 
cpylen: %#x\n", cont_atio, cpylen);
9c5e24e8 Anil Gurumurthy 2017-11-06  905  
9c5e24e8 Anil Gurumurthy 2017-11-06  906memcpy(purex_pyld_tmp, 
_atio->data[0], cpylen);
9c5e24e8 Anil Gurumurthy 2017-11-06  907  
9c5e24e8 Anil Gurumurthy 2017-11-06  908purex_pyld_tmp += 
cpylen;
9c5e24e8 Anil Gurumurthy 2017-11-06  909len -= cpylen;
9c5e24e8 Anil Gurumurthy 2017-11-06  910}
9c5e24e8 Anil Gurumurthy 2017-11-06  911  
9c5e24e8 Anil Gurumurthy 2017-11-06  912ql_dbg(ql_dbg_disc + 
ql_dbg_buffer, vha, 0xff11,
9c5e24e8 Anil Gurumurthy 2017-11-06  913"Dumping the Purex IOCB(%p) 
received\n", p->purex_pyld);
9c5e24e8 Anil Gu

[PATCH 2/2] tcmu: Add a missing unlock on an error path

[Dan Carpenter]

We added a new error path here but we forgot to drop the lock first
before returning.

Fixes: 0d44374c1aae ("tcmu: fix double se_cmd completion")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 07f2d95f7ae7..cc2468a299d3 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -888,6 +888,7 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
ret = tcmu_setup_cmd_timer(tcmu_cmd);
if (ret) {
tcmu_cmd_free_data(tcmu_cmd, tcmu_cmd->dbi_cnt);
+   mutex_unlock(>cmdr_lock);
return TCM_OUT_OF_RESOURCES;
}
entry->hdr.cmd_id = tcmu_cmd->cmd_id;

[PATCH 1/2] tcmu: Fix some memory corruption

[Dan Carpenter]

"udev->nl_reply_supported" is an int but on 64 bit arches we are writing
8 bytes of data to it so it corrupts four bytes beyond the end of the
struct.

Fixes: b849b4567549 ("target: Add netlink command reply supported option for 
each device")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 3be4c9696d19..07f2d95f7ae7 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1724,11 +1724,10 @@ static ssize_t tcmu_set_configfs_dev_params(struct 
se_device *dev,
ret = -ENOMEM;
break;
}
-   ret = kstrtol(arg_p, 0,
-   (long int *) >nl_reply_supported);
+   ret = kstrtoint(arg_p, 0, >nl_reply_supported);
kfree(arg_p);
if (ret < 0)
-   pr_err("kstrtoul() failed for 
nl_reply_supported=\n");
+   pr_err("kstrtoint() failed for 
nl_reply_supported=\n");
break;
default:
break;

[patch 2/2] scsi: mpt3sas: remove a stray KERN_INFO

[Dan Carpenter]

pr_info() has a KERN_INFO already so the second KERN_INFO isn't needed.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 3a9438a1704e..b258f210120a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -8683,7 +8684,7 @@ _scsih_mark_responding_pcie_device(struct MPT3SAS_ADAPTER 
*ioc,
 
if (pcie_device->handle == pcie_device_pg0->DevHandle)
goto out;
-   pr_info(KERN_INFO "\thandle changed from(0x%04x)!!!\n",
+   pr_info("\thandle changed from(0x%04x)!!!\n",
pcie_device->handle);
pcie_device->handle = pcie_device_pg0->DevHandle;
if (sas_target_priv_data)

[PATCH 1/2] scsi: mpt3sas: cleanup _scsih_pcie_enumeration_event()

[Dan Carpenter]

The indenting wasn't right, because the last two prints weren't
indented far enough.  Also it used pr_info() where it was supposed to
use pr_cont().  I reversed the if statement and pulled the code in one
tab and did a couple other minor cleanups.

Fixes: 4318c7347847 ("scsi: mpt3sas: Handle NVMe PCIe device related events 
generated from firmware.")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c 
b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 3a9438a1704e..93b45e618edb 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -7700,17 +7700,18 @@ _scsih_pcie_enumeration_event(struct MPT3SAS_ADAPTER 
*ioc,
Mpi26EventDataPCIeEnumeration_t *event_data =
(Mpi26EventDataPCIeEnumeration_t *)fw_event->event_data;
 
-   if (ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK) {
-   pr_info(MPT3SAS_FMT "pcie enumeration event: (%s) Flag 0x%02x",
-   ioc->name,
-   ((event_data->ReasonCode ==
-   MPI26_EVENT_PCIE_ENUM_RC_STARTED) ?
-   "started" : "completed"), event_data->Flags);
+   if (!(ioc->logging_level & MPT_DEBUG_EVENT_WORK_TASK))
+   return;
+
+   pr_info(MPT3SAS_FMT "pcie enumeration event: (%s) Flag 0x%02x",
+   ioc->name,
+   (event_data->ReasonCode == MPI26_EVENT_PCIE_ENUM_RC_STARTED) ?
+   "started" : "completed",
+   event_data->Flags);
if (event_data->EnumerationStatus)
-   pr_info("enumeration_status(0x%08x)",
-   le32_to_cpu(event_data->EnumerationStatus));
-   pr_info("\n");
-   }
+   pr_cont("enumeration_status(0x%08x)",
+   le32_to_cpu(event_data->EnumerationStatus));
+   pr_cont("\n");
 }
 
 /**

Re: [bug report] scsi: mpt3sas: Added support for nvme encapsulated request message.

[Dan Carpenter]

On Tue, Nov 07, 2017 at 10:29:20AM -0700, Sathya Prakash Veerichetty wrote:
> Dan,
> The MPI structures are of variable length and can go up to a maximum of
> 128 bytes (a MPI frame size) and as MPI standard the variable length MPI
> structures are left out with the last element as a single dword array.
> Can we ignore the warning?

Yeah.  These are a one time email but eventually other people running
static checkers are going to be confused as well.

> If not we need to modify the MPI structure to
> have the NVMe_Command array to the maximum size of the frame (which is
> typically 128 but can change across hardware generations)

To be honest, to declaring arrays as their maximum size is a pretty
common idiom...

U8  NVMe_Command[4];/*0x20 */

What does the 4 *mean* in this context?  As a human being, I would
understand that to be a 4 byte array, but it's clearly not.  You're
saying it's supposed to be a single dword array but why???  Is there
something special and mandatory stored in the first 4 bytes?  Why can't
it just be a zero size array?

It would help if there were at least a comment or something.

regards,
dan carpenter

[PATCH] scsi: hpsa: remove an unecessary NULL check

[Dan Carpenter]

device->scsi3addr[] is an array, not a pointer, so it can't be NULL.
I've removed the check.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 287e5eb0723f..b0aa5dc1d54c 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -8223,8 +8223,6 @@ static void hpsa_set_ioaccel_status(struct ctlr_info *h)
 
if (!device)
continue;
-   if (!device->scsi3addr)
-   continue;
if (!hpsa_vpd_page_supported(h, device->scsi3addr,
HPSA_VPD_LV_IOACCEL_STATUS))
continue;

[bug report] scsi: mpt3sas: Added support for nvme encapsulated request message.

[Dan Carpenter]

Hello Suganath Prabu Subramani,

The patch aff39e61218f: "scsi: mpt3sas: Added support for nvme
encapsulated request message." from Oct 31, 2017, leads to the
following static checker warning:

drivers/scsi/mpt3sas/mpt3sas_base.c:1459 _base_build_nvme_prp()
error: buffer overflow 'nvme_encap_request->NVMe_Command' 4 <= 24

drivers/scsi/mpt3sas/mpt3sas_base.c
  1453  /*
  1454   * Set pointers to PRP1 and PRP2, which are in the NVMe command.
  1455   * PRP1 is located at a 24 byte offset from the start of the 
NVMe
^^^
The ->NVMe_Command is declared as a 4 byte array so this makes static
checkers puzzled how there are more than 24 bytes in it.

  1456   * command.  Then set the current PRP entry pointer to PRP1.
  1457   */
  1458  prp1_entry = (__le64 *)(nvme_encap_request->NVMe_Command +
  1459  NVME_CMD_PRP1_OFFSET);
  1460  prp2_entry = (__le64 *)(nvme_encap_request->NVMe_Command +
  1461  NVME_CMD_PRP2_OFFSET);
  1462  prp_entry = prp1_entry;
  1463  /*
  1464   * For the PRP entries, use the specially allocated buffer of
  1465   * contiguous memory.
  1466   */
  1467  prp_page = (__le64 *)mpt3sas_base_get_pcie_sgl(ioc, smid);
  1468  prp_page_phys = (__le64 *)mpt3sas_base_get_pcie_sgl_dma(ioc, 
smid);
  1469  

regards,
dan carpenter

Re: integer overflow in aic7xxx (was [bug report] Linux-2.6.12-rc2)

[Dan Carpenter]

Argh...  My script chose a stupid subject.  Sorry for not catching that.

regards,
dan carpenter

On Wed, Oct 18, 2017 at 12:52:49AM +0300, Dan Carpenter wrote:
> Hey,
> 
> This code is older than git is so it probably doesn't matter.  But just
> for laughs does anyone know what this should be?
> 
>   drivers/scsi/aic7xxx/aic7xxx_core.c:4807 ahc_init_scbdata()
>   warn: integer overflow (literal): u32max + 1
> 
> drivers/scsi/aic7xxx/aic7xxx_core.c
>   4794  
>   4795  /*
>   4796   * Create our DMA tags.  These tags define the kinds of device
>   4797   * accessible memory allocations and memory mappings we will
>   4798   * need to perform during normal operation.
>   4799   *
>   4800   * Unless we need to further restrict the allocation, we rely
>   4801   * on the restrictions of the parent dmat, hence the common
>   4802   * use of MAXADDR and MAXSIZE.
>   4803   */
>   4804  
>   4805  /* DMA tag for our hardware scb structures */
>   4806  if (ahc_dma_tag_create(ahc, ahc->parent_dmat, /*alignment*/1,
>   4807 /*boundary*/BUS_SPACE_MAXADDR_32BIT + 
> 1,
>^^^
> This is "0x + 1" which has an integer overflow so it's a
> complicated way to say zero.
> 
>   4808 /*lowaddr*/BUS_SPACE_MAXADDR_32BIT,
>   4809 /*highaddr*/BUS_SPACE_MAXADDR,
>   4810 /*filter*/NULL, /*filterarg*/NULL,
>   4811 AHC_SCB_MAX_ALLOC * sizeof(struct 
> hardware_scb),
>   4812 /*nsegments*/1,
>   4813 /*maxsegsz*/BUS_SPACE_MAXSIZE_32BIT,
>   4814 /*flags*/0, _data->hscb_dmat) != 
> 0) {
>   4815      goto error_exit;
>   4816  }
>   4817  
>   4818  scb_data->init_level++;
>   4819  
> 
> regards,
> dan carpenter

[bug report] Linux-2.6.12-rc2

[Dan Carpenter]

Hey,

This code is older than git is so it probably doesn't matter.  But just
for laughs does anyone know what this should be?

drivers/scsi/aic7xxx/aic7xxx_core.c:4807 ahc_init_scbdata()
warn: integer overflow (literal): u32max + 1

drivers/scsi/aic7xxx/aic7xxx_core.c
  4794  
  4795  /*
  4796   * Create our DMA tags.  These tags define the kinds of device
  4797   * accessible memory allocations and memory mappings we will
  4798   * need to perform during normal operation.
  4799   *
  4800   * Unless we need to further restrict the allocation, we rely
  4801   * on the restrictions of the parent dmat, hence the common
  4802   * use of MAXADDR and MAXSIZE.
  4803   */
  4804  
  4805  /* DMA tag for our hardware scb structures */
  4806  if (ahc_dma_tag_create(ahc, ahc->parent_dmat, /*alignment*/1,
  4807 /*boundary*/BUS_SPACE_MAXADDR_32BIT + 1,
   ^^^
This is "0x + 1" which has an integer overflow so it's a
complicated way to say zero.

  4808 /*lowaddr*/BUS_SPACE_MAXADDR_32BIT,
  4809 /*highaddr*/BUS_SPACE_MAXADDR,
  4810 /*filter*/NULL, /*filterarg*/NULL,
  4811 AHC_SCB_MAX_ALLOC * sizeof(struct 
hardware_scb),
  4812 /*nsegments*/1,
  4813 /*maxsegsz*/BUS_SPACE_MAXSIZE_32BIT,
  4814 /*flags*/0, _data->hscb_dmat) != 0) {
  4815  goto error_exit;
  4816  }
  4817  
  4818  scb_data->init_level++;
  4819  

regards,
dan carpenter

[PATCH] scsi: lpfc: Fix a precedence bug in lpfc_nvme_io_cmd_wqe_cmpl()

[Dan Carpenter]

The ! has higher precedence than the & operation.  I've added
parenthesis so this works as intended.

Fixes: 952c303b329c ("scsi: lpfc: Ensure io aborts interlocked with the 
target.")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/lpfc/lpfc_nvme.c b/drivers/scsi/lpfc/lpfc_nvme.c
index 60f0dbd0f192..517ae570e507 100644
--- a/drivers/scsi/lpfc/lpfc_nvme.c
+++ b/drivers/scsi/lpfc/lpfc_nvme.c
@@ -948,7 +948,7 @@ lpfc_nvme_io_cmd_wqe_cmpl(struct lpfc_hba *phba, struct 
lpfc_iocbq *pwqeIn,
/* NVME targets need completion held off until the abort exchange
 * completes.
 */
-   if (!lpfc_ncmd->flags & LPFC_SBUF_XBUSY)
+   if (!(lpfc_ncmd->flags & LPFC_SBUF_XBUSY))
nCmd->done(nCmd);
 
spin_lock_irqsave(>hbalock, flags);

[PATCH] [SCSI] bfa: integer overflow in debugfs

[Dan Carpenter]

We could allocate less memory than intended because we do:

bfad->regdata = kzalloc(len << 2, GFP_KERNEL);

The shift can overflow leading to a crash.  This is debugfs code so the
impact is very small.  I fixed the network version of this in March with
commit 13e2d5187f6b ("bna: integer overflow bug in debugfs").

Fixes: ab2a9ba189e8 ("[SCSI] bfa: add debugfs support")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/bfa/bfad_debugfs.c b/drivers/scsi/bfa/bfad_debugfs.c
index 8dcd8c70c7ee..05f523971348 100644
--- a/drivers/scsi/bfa/bfad_debugfs.c
+++ b/drivers/scsi/bfa/bfad_debugfs.c
@@ -255,7 +255,8 @@ bfad_debugfs_write_regrd(struct file *file, const char 
__user *buf,
struct bfad_s *bfad = port->bfad;
struct bfa_s *bfa = >bfa;
struct bfa_ioc_s *ioc = >ioc;
-   int addr, len, rc, i;
+   int addr, rc, i;
+   u32 len;
u32 *regbuf;
void __iomem *rb, *reg_addr;
unsigned long flags;
@@ -266,7 +267,7 @@ bfad_debugfs_write_regrd(struct file *file, const char 
__user *buf,
return PTR_ERR(kern_buf);
 
rc = sscanf(kern_buf, "%x:%x", , );
-   if (rc < 2) {
+   if (rc < 2 || len > (UINT_MAX >> 2)) {
printk(KERN_INFO
"bfad[%d]: %s failed to read user buf\n",
bfad->inst_no, __func__);

[bug report] [SCSI] lpfc: NPIV: add NPIV support on top of SLI-3

[Dan Carpenter]

ck);
  6190  return 0;
  6191  }
  6192  lpfc_debugfs_disc_trc(vport, LPFC_DISC_TRC_ELS_UNSOL,
  6193  "RCV RSCN:did:x%x/ste:x%x flg:x%x",
  6194  ndlp->nlp_DID, vport->port_state, ndlp->nlp_flag);
  6195  
  6196  spin_lock_irq(shost->host_lock);
  6197  vport->fc_flag |= FC_RSCN_MODE;
  6198  spin_unlock_irq(shost->host_lock);
  6199  vport->fc_rscn_id_list[vport->fc_rscn_id_cnt++] = pcmd;
   ^^^^^^^
But here we don't check here.  It feels like we should be checking on
this path as well.


  6200  /* Indicate we are done walking fc_rscn_id_list on this vport */
  6201  vport->fc_rscn_flush = 0;

regards,
dan carpenter

[PATCH] mpt3sas: Fix a double unlock in _transport_smp_handler()

[Dan Carpenter]

We can't "goto out;" if we're not holding "ioc->transport_cmds.mutex".
It leads to a double unlock bug, and I don't think we should set
"ioc->transport_cmds.status" if we don't have the lock.

Fixes: 651a01364994 ("scsi: scsi_transport_sas: switch to bsg-lib for SMP 
passthrough")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
---
I'm not totally sure about the .status thing.  This is a static checker
fix.

diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c 
b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index d3940c5d079d..c9cd9ed90002 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -1936,12 +1936,12 @@ _transport_smp_handler(struct bsg_job *job, struct 
Scsi_Host *shost,
pr_info(MPT3SAS_FMT "%s: host reset in progress!\n",
__func__, ioc->name);
rc = -EFAULT;
-   goto out;
+   goto job_done;
}
 
rc = mutex_lock_interruptible(>transport_cmds.mutex);
if (rc)
-   goto out;
+   goto job_done;
 
if (ioc->transport_cmds.status != MPT3_CMD_NOT_USED) {
pr_err(MPT3SAS_FMT "%s: transport_cmds in use\n", ioc->name,
@@ -2066,6 +2066,7 @@ _transport_smp_handler(struct bsg_job *job, struct 
Scsi_Host *shost,
  out:
ioc->transport_cmds.status = MPT3_CMD_NOT_USED;
mutex_unlock(>transport_cmds.mutex);
+ job_done:
bsg_job_done(job, rc, reslen);
 }
 

Re: Buffer overflow in the mptctl_replace_fw() function in linux kernel MPT ioctl driver

[Dan Carpenter]

On Fri, Sep 01, 2017 at 02:00:48PM +0800, Dison River wrote:
> newFwSize = ALIGN(karg.newImageSize, 4);

This is an integer overflow, but it's harmless...  As a static checker
developer this is where I would print a warning:
drivers/message/fusion/mptctl.c:1748 mptctl_replace_fw() warn: potential 
integer overflow from user '((karg.newImageSize)) + (((4)) - 1)'
I also caught the integer overflow from two days ago but there are too
many ones like this so I can't check them all.  In mpt_alloc_fw_memory()
there is another potential integer overflow when we do:

ioc->alloc_total += size;

But ->alloc_total is not used anywhere.

I don't see a buffer overflow here.

regards,
dan carpenter

[PATCH v2] scsi: qla2xxx: Fix an integer overflow in sysfs code

[Dan Carpenter]

The value of "size" comes from the user.  When we add "start + size"
it could lead to an integer overflow bug.

It means we vmalloc() a lot more memory than we had intended.  I believe
that on 64 bit systems vmalloc() can succeed even if we ask it to
allocate huge 4GB buffers.  So we would get memory corruption and likely
a crash when we call ha->isp_ops->write_optrom() and ->read_optrom().

Only root can trigger this bug.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=194061

Cc: sta...@vger.kernel.org
Fixes: b7cc176c9eb3 ("[SCSI] qla2xxx: Allow region-based flash-part accesses.")
Reported-by: shqking <shqk...@gmail.com>
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
---
v2: Add stable and the URL for bugzila

diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
index 75c4b312645e..9ce28c4f9812 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -318,6 +318,8 @@ qla2x00_sysfs_write_optrom_ctl(struct file *filp, struct 
kobject *kobj,
return -EINVAL;
if (start > ha->optrom_size)
return -EINVAL;
+   if (size > ha->optrom_size - start)
+   size = ha->optrom_size - start;
 
mutex_lock(>optrom_mutex);
switch (val) {
@@ -343,8 +345,7 @@ qla2x00_sysfs_write_optrom_ctl(struct file *filp, struct 
kobject *kobj,
}
 
ha->optrom_region_start = start;
-   ha->optrom_region_size = start + size > ha->optrom_size ?
-   ha->optrom_size - start : size;
+   ha->optrom_region_size = start + size;
 
ha->optrom_state = QLA_SREADING;
ha->optrom_buffer = vmalloc(ha->optrom_region_size);
@@ -417,8 +418,7 @@ qla2x00_sysfs_write_optrom_ctl(struct file *filp, struct 
kobject *kobj,
}
 
ha->optrom_region_start = start;
-   ha->optrom_region_size = start + size > ha->optrom_size ?
-   ha->optrom_size - start : size;
+   ha->optrom_region_size = start + size;
 
ha->optrom_state = QLA_SWRITING;
ha->optrom_buffer = vmalloc(ha->optrom_region_size);

Re: [PATCH] scsi: qla2xxx: Fix an integer overflow in sysfs code

[Dan Carpenter]

On Wed, Aug 30, 2017 at 08:28:52PM +0800, shqking wrote:
> Hi,
> 
> Glad to see it is fixed.
> 
> Can I apply for a CVE ID for this bug?
> 

We don't handle that on this list.  You'd need to ask on
oss-secur...@lists.openwall.com.

regards,
dan carpenter

[PATCH] scsi: qla2xxx: Fix an integer overflow in sysfs code

[Dan Carpenter]

The value of "size" comes from the user.  When we add "start + size"
it could lead to an integer overflow bug.

It means we vmalloc() a lot more memory than we had intended.  I believe
that on 64 bit systems vmalloc() can succeed even if we ask it to
allocate huge 4GB buffers.  So we would get memory corruption and likely
a crash when we call ha->isp_ops->write_optrom() and ->read_optrom().

Only root can trigger this bug.

Fixes: b7cc176c9eb3 ("[SCSI] qla2xxx: Allow region-based flash-part accesses.")
Reported-by: shqking <shqk...@gmail.com>
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
index 75c4b312645e..9ce28c4f9812 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -318,6 +318,8 @@ qla2x00_sysfs_write_optrom_ctl(struct file *filp, struct 
kobject *kobj,
return -EINVAL;
if (start > ha->optrom_size)
return -EINVAL;
+   if (size > ha->optrom_size - start)
+   size = ha->optrom_size - start;
 
mutex_lock(>optrom_mutex);
switch (val) {
@@ -343,8 +345,7 @@ qla2x00_sysfs_write_optrom_ctl(struct file *filp, struct 
kobject *kobj,
}
 
ha->optrom_region_start = start;
-   ha->optrom_region_size = start + size > ha->optrom_size ?
-   ha->optrom_size - start : size;
+   ha->optrom_region_size = start + size;
 
ha->optrom_state = QLA_SREADING;
ha->optrom_buffer = vmalloc(ha->optrom_region_size);
@@ -417,8 +418,7 @@ qla2x00_sysfs_write_optrom_ctl(struct file *filp, struct 
kobject *kobj,
}
 
ha->optrom_region_start = start;
-   ha->optrom_region_size = start + size > ha->optrom_size ?
-   ha->optrom_size - start : size;
+   ha->optrom_region_size = start + size;
 
ha->optrom_state = QLA_SWRITING;
ha->optrom_buffer = vmalloc(ha->optrom_region_size);

[PATCH] scsi: qedi: off by one in qedi_get_cmd_from_tid()

[Dan Carpenter]

The > here should be >= or we end up reading one element beyond the end
of the qedi->itt_map[] array.  The qedi->itt_map[] array is allocated in
qedi_alloc_itt().

Fixes: ace7f46ba5fd ("scsi: qedi: Add QLogic FastLinQ offload iSCSI driver 
framework.")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
index c4a470bab4dd..34adc0e0 100644
--- a/drivers/scsi/qedi/qedi_main.c
+++ b/drivers/scsi/qedi/qedi_main.c
@@ -1576,7 +1576,7 @@ struct qedi_cmd *qedi_get_cmd_from_tid(struct qedi_ctx 
*qedi, u32 tid)
 {
struct qedi_cmd *cmd = NULL;
 
-   if (tid > MAX_ISCSI_TASK_ENTRIES)
+   if (tid >= MAX_ISCSI_TASK_ENTRIES)
return NULL;
 
cmd = qedi->itt_map[tid].p_cmd;

[bug report] scsi: qla2xxx: Cleanup NPIV host in target mode during config teardown

[Dan Carpenter]

Hello Quinn Tran,

This is a semi-automatic email about new static checker warnings.

The patch 821c9f4cab5e: "scsi: qla2xxx: Cleanup NPIV host in target
mode during config teardown" from Aug 23, 2017, leads to the
following Smatch complaint:

drivers/scsi/qla2xxx/qla_target.c:1553 qlt_release()
error: we previously assumed 'vha->vha_tgt.qla_tgt' could be null (see line 
1533)

drivers/scsi/qla2xxx/qla_target.c
  1532  
  1533  if ((vha->vha_tgt.qla_tgt != NULL) && !tgt->tgt_stop &&
  1534  !tgt->tgt_stopped)
  1535  qlt_stop_phase1(tgt);
  1536  
  1537  if ((vha->vha_tgt.qla_tgt != NULL) && !tgt->tgt_stopped)
^^
Existing code assumes this can be NULL.

  1538  qlt_stop_phase2(tgt);
  1539  
  1540  for (i = 0; i < vha->hw->max_qpairs + 1; i++) {
  1541  unsigned long flags;
  1542  
  1543  h = >qphints[i];
  1544  if (h->qpair) {
  1545  spin_lock_irqsave(h->qpair->qp_lock_ptr, flags);
  1546  list_del(>hint_elem);
  1547  spin_unlock_irqrestore(h->qpair->qp_lock_ptr, 
flags);
  1548  h->qpair = NULL;
  1549  }
  1550  }
  1551  kfree(tgt->qphints);
  1552  mutex_lock(_tgt_mutex);
  1553  list_del(>vha_tgt.qla_tgt->tgt_list_entry);\
  
The patch adds a new dereference.

  1554  mutex_unlock(_tgt_mutex);
  1555  

regards,
dan carpenter

[bug report] scsi: lpfc: Fix handling of FCP and NVME FC4 types in Pt2Pt topology

[Dan Carpenter]

Hello Dick Kennedy,

This is a semi-automatic email about new static checker warnings.

The patch 8fd5f79a9cf4: "scsi: lpfc: Fix handling of FCP and NVME FC4 
types in Pt2Pt topology" from Aug 23, 2017, leads to the following 
Smatch complaint:

drivers/scsi/lpfc/lpfc_els.c:2010 lpfc_issue_els_plogi()
 error: we previously assumed 'ndlp' could be null (see line 1998)

drivers/scsi/lpfc/lpfc_els.c
  1997  ndlp = lpfc_findnode_did(vport, did);
  1998  if (ndlp && !NLP_CHK_NODE_ACT(ndlp))
  1999  ndlp = NULL;
^^^
ndlp can be NULL.

  2000  
  2001  /* If ndlp is not NULL, we will bump the reference count on it 
*/
  2002  cmdsize = (sizeof(uint32_t) + sizeof(struct serv_parm));
  2003  elsiocb = lpfc_prep_els_iocb(vport, 1, cmdsize, retry, ndlp, 
did,
  2004   ELS_CMD_PLOGI);
  2005  if (!elsiocb)
  2006  return 1;
  2007  
  2008  shost = lpfc_shost_from_vport(vport);
  2009  spin_lock_irq(shost->host_lock);
  2010  ndlp->nlp_flag &= ~NLP_FCP_PRLI_RJT;
^^
We added a new unchecked dereference.

  2011  spin_unlock_irq(shost->host_lock);
  2012  

regards,
dan carpenter

[PATCH] scsi: hpsa: fix the device_id in hpsa_update_device_info()

[Dan Carpenter]

The parentheses are in the wrong place so we specify the length as
"sizeof(this_device->device_id) < 0" which is zero.

Fixes: 988b87edd231 ("scsi: hpsa: Ignore errors for unsupported LV_DEVICE_ID 
VPD page")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 2773dd7a2087..a22295ee3b70 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -3827,7 +3827,7 @@ static int hpsa_update_device_info(struct ctlr_info *h,
memset(this_device->device_id, 0,
sizeof(this_device->device_id));
if (hpsa_get_device_id(h, scsi3addr, this_device->device_id, 8,
-   sizeof(this_device->device_id) < 0))
+   sizeof(this_device->device_id)) < 0)
dev_err(>pdev->dev,
"hpsa%d: %s: can't get device id for host 
%d:C0:T%d:L%d\t%s\t%.16s\n",
h->ctlr, __func__,

[PATCH] scsi: sg: off by one in sg_ioctl()

[Dan Carpenter]

If "val" is SG_MAX_QUEUE then we are one element beyond the end of the
"rinfo" array so the > should be >=.

Fixes: 109bade9c625 ("scsi: sg: use standard lists for sg_requests")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index d7ff71e0c85c..84e782d8e7c3 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1021,7 +1021,7 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned 
long arg)
read_lock_irqsave(>rq_list_lock, iflags);
val = 0;
list_for_each_entry(srp, >rq_list, entry) {
-   if (val > SG_MAX_QUEUE)
+   if (val >= SG_MAX_QUEUE)
break;
memset([val], 0, SZ_SG_REQ_INFO);
rinfo[val].req_state = srp->done + 1;

[PATCH] scsi: osst: silence underflow warning in osst_verify_frame()

[Dan Carpenter]

The code looks like this:

i = ntohl(aux->filemark_cnt);
if (STp->header_cache != NULL && i < OS_FM_TAB_MAX && (i > 
STp->filemark_cnt ||
STp->first_frame_position - 1 != 
ntohl(STp->header_cache->dat_fm_tab.fm_tab_ent[i]))) {

If i is negative then it's less than OS_FM_TAB_MAX so we read before
the start of the STp->header_cache->dat_fm_tab.fm_tab_ent[] array.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
---
There is a second static checker warning that I didn't know how to
address:

drivers/scsi/osst.c:723 osst_verify_frame()
warn: potential integer overflow from user 'blk_cnt * blk_sz'

diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c
index 97ab5f160bc6..2db87ec04f48 100644
--- a/drivers/scsi/osst.c
+++ b/drivers/scsi/osst.c
@@ -619,7 +619,7 @@ static int osst_verify_frame(struct osst_tape * STp, int 
frame_seq_number, int q
os_aux_t   * aux  = STp->buffer->aux;
os_partition_t * par  = &(aux->partition);
struct st_partstat * STps = &(STp->ps[STp->partition]);
-   int  blk_cnt, blk_sz, i;
+   unsigned int blk_cnt, blk_sz, i;
 
if (STp->raw) {
if (STp->buffer->syscall_result) {

[PATCH] tcmu: Oops in unmap_thread_fn()

[Dan Carpenter]

Calling list_del() on the iterator pointer in list_for_each_entry() will
cause an oops.  We need to user the _safe() version for that.

Fixes: c73d02f63c16 ("tcmu: Add fifo type waiter list support to avoid 
starvation")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/target/target_core_user.c 
b/drivers/target/target_core_user.c
index 9258b7dd2c30..fd9fcea68d23 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1985,7 +1985,7 @@ static struct target_backend_ops tcmu_ops = {
 
 static int unmap_thread_fn(void *data)
 {
-   struct tcmu_dev *udev;
+   struct tcmu_dev *udev, *tmp;
loff_t off;
uint32_t start, end, block;
static uint32_t free_blocks;
@@ -2056,7 +2056,7 @@ static int unmap_thread_fn(void *data)
 * for the global data pool blocks.
 */
mutex_lock(_udev_waiter_mutex);
-   list_for_each_entry(udev, _udev_waiter, waiter) {
+   list_for_each_entry_safe(udev, tmp, _udev_waiter, waiter) {
mutex_lock(>cmdr_lock);
if (udev->waiting_blocks < free_blocks) {
mutex_unlock(>cmdr_lock);

Re: [PATCH 2/2] scsi: aacraid: Off by one NUL terminator

[Dan Carpenter]

It would be simple enough to write it like you say, but it probably
should be done by someone who is able to test it.

regards,
dan carpenter

[PATCH 2/2] scsi: aacraid: Off by one NUL terminator

[Dan Carpenter]

We're putting a NUL terminator one character beyond the end of the
struct and that's obviously wrong.  On the other hand, I'm not positive
this is the correct fix.  This change was added deliberately and was
mentioned in the changlog of commit b836439faf04 ("aacraid: 4KB sector
support").  The relevant section is "Also fix up a name truncation
problem".  Can someone review this code and figure out the right thing
to do?

Fixes: b836439faf04 ("aacraid: 4KB sector support")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 4591113c49de..22c7461f65c9 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -549,7 +549,7 @@ static void get_container_name_callback(void *context, 
struct fib * fibptr)
if ((le32_to_cpu(get_name_reply->status) == CT_OK)
 && (get_name_reply->data[0] != '\0')) {
char *sp = get_name_reply->data;
-   sp[sizeof(((struct aac_get_name_resp *)NULL)->data)] = '\0';
+   sp[sizeof(((struct aac_get_name_resp *)NULL)->data) - 1] = '\0';
while (*sp == ' ')
++sp;
if (*sp) {

[PATCH 1/2] scsi: aacraid: reading out of bounds

[Dan Carpenter]

"qd.id" comes directly from the copy_from_user() on the line before so
we should verify that it's within bounds.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
---
This bug predates git.

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 707ee2f5954d..4591113c49de 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -3198,10 +3198,11 @@ static int query_disk(struct aac_dev *dev, void __user 
*arg)
return -EBUSY;
if (copy_from_user(, arg, sizeof (struct aac_query_disk)))
return -EFAULT;
-   if (qd.cnum == -1)
+   if (qd.cnum == -1) {
+   if (qd.id < 0 || qd.id >= dev->maximum_num_containers)
+   return -EINVAL;
qd.cnum = qd.id;
-   else if ((qd.bus == -1) && (qd.id == -1) && (qd.lun == -1))
-   {
+   } else if ((qd.bus == -1) && (qd.id == -1) && (qd.lun == -1)) {
if (qd.cnum < 0 || qd.cnum >= dev->maximum_num_containers)
return -EINVAL;
qd.instance = dev->scsi_host_ptr->host_no;

Re: [bug report] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Dan Carpenter]

On Fri, Jul 14, 2017 at 11:01:11AM +0200, Johannes Thumshirn wrote:
> On Fri, Jul 14, 2017 at 10:46:03AM +0300, Dan Carpenter wrote:
> >761  case SG_DXFER_FROM_DEV:
> >762  if (hp->dxfer_len < 0)
> > ^
> > Not possible.
> 
> Yup and thanks to:
> 647 else
> 648 hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : 
> SG_DXFER_NONE;
> 649 hp->dxfer_len = mxsize;
> 
> I should've noticed that earlier.
> Care to send a patch or want me to do?
> 

Could you send the patch?

regards,
dan carpenter

[bug report] scsi: sg: fix SG_DXFER_FROM_DEV transfers

[Dan Carpenter]

Hello Johannes Thumshirn,

The patch 68c59fcea1f2: "scsi: sg: fix SG_DXFER_FROM_DEV transfers"
from Jul 7, 2017, leads to the following static checker warning:

drivers/scsi/sg.c:762 sg_is_valid_dxfer()
warn: unsigned 'hp->dxfer_len' is never less than zero.

drivers/scsi/sg.c
   754  static bool sg_is_valid_dxfer(sg_io_hdr_t *hp)
   755  {
   756  switch (hp->dxfer_direction) {
   757  case SG_DXFER_NONE:
   758  if (hp->dxferp || hp->dxfer_len > 0)
   759  return false;
   760  return true;
   761  case SG_DXFER_FROM_DEV:
   762  if (hp->dxfer_len < 0)
^
Not possible.

   763  return false;
   764  return true;
   765  case SG_DXFER_TO_DEV:
   766  case SG_DXFER_TO_FROM_DEV:
   767  if (!hp->dxferp || hp->dxfer_len == 0)
   768  return false;
   769  return true;
   770  case SG_DXFER_UNKNOWN:
   771  if ((!hp->dxferp && hp->dxfer_len) ||
   772  (hp->dxferp && hp->dxfer_len == 0))
   773  return false;
   774  return true;
   775  default:
   776  return false;
   777  }
   778  }

Btw, I was looking up the type of hp->dxfer_len and I noticed that the
documentation for hp->resid is wrong.  It says:

int resid;  /* [o] dxfer_len - actual_transferred */

My guess is that resid stands for Response ID but I'm not positive.

regards,
dan carpenter

[PATCH] scsi: qedi: Fix return code in qedi_ep_connect()

[Dan Carpenter]

We shouldn't be writing over the "ret" variable.  It means we return
ERR_PTR(0) which is NULL and it results in a NULL dereference in the
caller.

Fixes: ace7f46ba5fd ("scsi: qedi: Add QLogic FastLinQ offload iSCSI driver 
framework.")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
index 80edd28b635f..37da9a8b43b1 100644
--- a/drivers/scsi/qedi/qedi_iscsi.c
+++ b/drivers/scsi/qedi/qedi_iscsi.c
@@ -824,7 +824,7 @@ qedi_ep_connect(struct Scsi_Host *shost, struct sockaddr 
*dst_addr,
u32 iscsi_cid = QEDI_CID_RESERVED;
u16 len = 0;
char *buf = NULL;
-   int ret;
+   int ret, tmp;
 
if (!shost) {
ret = -ENXIO;
@@ -940,10 +940,10 @@ qedi_ep_connect(struct Scsi_Host *shost, struct sockaddr 
*dst_addr,
 
 ep_rel_conn:
qedi->ep_tbl[iscsi_cid] = NULL;
-   ret = qedi_ops->release_conn(qedi->cdev, qedi_ep->handle);
-   if (ret)
+   tmp = qedi_ops->release_conn(qedi->cdev, qedi_ep->handle);
+   if (tmp)
QEDI_WARN(>dbg_ctx, "release_conn returned %d\n",
- ret);
+ tmp);
 ep_free_sq:
qedi_free_sq(qedi, qedi_ep);
 ep_conn_exit:

[PATCH] scsi: libfc: pass an error pointer to fc_disc_error()

[Dan Carpenter]

This patch is basically to silence a static checker warning.

drivers/scsi/libfc/fc_disc.c:326 fc_disc_error()
warn: passing a valid pointer to 'PTR_ERR'

It doesn't affect runtime because it treats -ENOMEM and a valid pointer
the same.  But the documentation says we should be passing an error
pointer.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/libfc/fc_disc.c b/drivers/scsi/libfc/fc_disc.c
index fd501f8dbb11..8660f923ace0 100644
--- a/drivers/scsi/libfc/fc_disc.c
+++ b/drivers/scsi/libfc/fc_disc.c
@@ -573,7 +573,7 @@ static void fc_disc_gpn_ft_resp(struct fc_seq *sp, struct 
fc_frame *fp,
event = DISC_EV_FAILED;
}
if (error)
-   fc_disc_error(disc, fp);
+   fc_disc_error(disc, ERR_PTR(error));
else if (event != DISC_EV_NONE)
fc_disc_done(disc, event);
fc_frame_free(fp);

[PATCH] scsi: qla2xxx: Off by one in qlt_ctio_to_cmd()

[Dan Carpenter]

There are "req->num_outstanding_cmds" elements in the
req->outstanding_cmds[] array so the > here should be >=.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/qla2xxx/qla_target.c 
b/drivers/scsi/qla2xxx/qla_target.c
index 6e4794367e0b..ecd1a95511f9 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -3728,7 +3728,7 @@ static struct qla_tgt_cmd *qlt_ctio_to_cmd(struct 
scsi_qla_host *vha,
h &= QLA_CMD_HANDLE_MASK;
 
if (h != QLA_TGT_NULL_HANDLE) {
-   if (unlikely(h > req->num_outstanding_cmds)) {
+   if (unlikely(h >= req->num_outstanding_cmds)) {
ql_dbg(ql_dbg_tgt, vha, 0xe052,
"qla_target(%d): Wrong handle %x received\n",
vha->vp_idx, handle);

Re: [patch] scsi: qedi: silence sprintf() overflow warning

[Dan Carpenter]

On Tue, Feb 07, 2017 at 02:27:09PM +0100, walter harms wrote:
> 
> 
> Am 07.02.2017 14:01, schrieb Dan Carpenter:
> > The problem here is this:
> > 
> > sprintf(host_buf, "qedi_ofld%d", qedi->shost->host_no);
> > 
> > host_buf is 16 character so we only have 6 characters left for
> > ->host_no.  But ->host_no is set in scsi_host_alloc():
> > 
> > index = ida_simple_get(_index_ida, 0, 0, GFP_KERNEL);
> > 
> > It could theoretically go up to 0x800 so we need space for 10
> > digits.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> > 
> > diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
> > index 5eda21d903e9..0dcf3b08230c 100644
> > --- a/drivers/scsi/qedi/qedi_main.c
> > +++ b/drivers/scsi/qedi/qedi_main.c
> > @@ -1735,7 +1735,7 @@ static int __qedi_probe(struct pci_dev *pdev, int 
> > mode)
> > u32 dp_module = 0;
> > u8 dp_level = 0;
> > bool is_vf = false;
> > -   char host_buf[16];
> > +   char host_buf[20];
> > struct qed_link_params link_params;
> > struct qed_slowpath_params sp_params;
> > struct qed_probe_params qed_params;
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" 
> > in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> any chance to use snprintf here ?
>  sprintf(host_buf, "qedi_ofld%d", qedi->shost->host_no);
> 
> or something like asprint() :)
> 
> if ever anyone change the type to very_long_type in the future it would 
> simply break
> but not hurt.

No, I don't think that's required.  There are infinite possible futures
and the future you're describing is not likely.  We'd just end up making
the code more complicated for no reason.

regards,
dan carpenter

[PATCH 2/2] scsi: lpfc: don't double count abort errors

[Dan Carpenter]

If lpfc_nvmet_unsol_fcp_issue_abort() fails then we accidentally
increment "tgtp->xmt_abort_rsp_error" and then two lines later we
increment it a second time.

Fixes: 547077a44b3b ("scsi: lpfc: Adding additional stats counters for nvme.")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/lpfc/lpfc_nvmet.c b/drivers/scsi/lpfc/lpfc_nvmet.c
index 7dc061a14f95..fbeec344c6cc 100644
--- a/drivers/scsi/lpfc/lpfc_nvmet.c
+++ b/drivers/scsi/lpfc/lpfc_nvmet.c
@@ -2583,7 +2583,6 @@ lpfc_nvmet_unsol_fcp_issue_abort(struct lpfc_hba *phba,
}
 
 aerr:
-   atomic_inc(>xmt_abort_rsp_error);
ctxp->flag &= ~LPFC_NVMET_ABORT_OP;
atomic_inc(>xmt_abort_rsp_error);
lpfc_printf_log(phba, KERN_ERR, LOG_NVME_ABTS,

[PATCH 1/2] scsi: lpfc: spin_lock_irq() is not nestable

[Dan Carpenter]

We're calling spin_lock_irq() multiple times, the problem is that on the
first spin_unlock_irq() then we will re-enable IRQs and we don't want
that.

Fixes: 966bb5b71196 ("scsi: lpfc: Break up IO ctx list into a separate get and 
put list")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/lpfc/lpfc_nvmet.c b/drivers/scsi/lpfc/lpfc_nvmet.c
index 7dc061a14f95..afc523209845 100644
--- a/drivers/scsi/lpfc/lpfc_nvmet.c
+++ b/drivers/scsi/lpfc/lpfc_nvmet.c
@@ -866,44 +866,44 @@ lpfc_nvmet_cleanup_io_context(struct lpfc_hba *phba)
unsigned long flags;
 
spin_lock_irqsave(>sli4_hba.nvmet_ctx_get_lock, flags);
-   spin_lock_irq(>sli4_hba.nvmet_ctx_put_lock);
+   spin_lock(>sli4_hba.nvmet_ctx_put_lock);
list_for_each_entry_safe(ctx_buf, next_ctx_buf,
>sli4_hba.lpfc_nvmet_ctx_get_list, list) {
-   spin_lock_irq(>sli4_hba.abts_nvme_buf_list_lock);
+   spin_lock(>sli4_hba.abts_nvme_buf_list_lock);
list_del_init(_buf->list);
-   spin_unlock_irq(>sli4_hba.abts_nvme_buf_list_lock);
+   spin_unlock(>sli4_hba.abts_nvme_buf_list_lock);
__lpfc_clear_active_sglq(phba,
 ctx_buf->sglq->sli4_lxritag);
ctx_buf->sglq->state = SGL_FREED;
ctx_buf->sglq->ndlp = NULL;
 
-   spin_lock_irq(>sli4_hba.sgl_list_lock);
+   spin_lock(>sli4_hba.sgl_list_lock);
list_add_tail(_buf->sglq->list,
  >sli4_hba.lpfc_nvmet_sgl_list);
-   spin_unlock_irq(>sli4_hba.sgl_list_lock);
+   spin_unlock(>sli4_hba.sgl_list_lock);
 
lpfc_sli_release_iocbq(phba, ctx_buf->iocbq);
kfree(ctx_buf->context);
}
list_for_each_entry_safe(ctx_buf, next_ctx_buf,
>sli4_hba.lpfc_nvmet_ctx_put_list, list) {
-   spin_lock_irq(>sli4_hba.abts_nvme_buf_list_lock);
+   spin_lock(>sli4_hba.abts_nvme_buf_list_lock);
list_del_init(_buf->list);
-   spin_unlock_irq(>sli4_hba.abts_nvme_buf_list_lock);
+   spin_unlock(>sli4_hba.abts_nvme_buf_list_lock);
__lpfc_clear_active_sglq(phba,
 ctx_buf->sglq->sli4_lxritag);
ctx_buf->sglq->state = SGL_FREED;
ctx_buf->sglq->ndlp = NULL;
 
-   spin_lock_irq(>sli4_hba.sgl_list_lock);
+   spin_lock(>sli4_hba.sgl_list_lock);
list_add_tail(_buf->sglq->list,
  >sli4_hba.lpfc_nvmet_sgl_list);
-   spin_unlock_irq(>sli4_hba.sgl_list_lock);
+   spin_unlock(>sli4_hba.sgl_list_lock);
 
lpfc_sli_release_iocbq(phba, ctx_buf->iocbq);
kfree(ctx_buf->context);
}
-   spin_unlock_irq(>sli4_hba.nvmet_ctx_put_lock);
+   spin_unlock(>sli4_hba.nvmet_ctx_put_lock);
spin_unlock_irqrestore(>sli4_hba.nvmet_ctx_get_lock, flags);
 }
 

[PATCH] cxlflash: return -EFAULT if copy_from_user() fails

[Dan Carpenter]

The copy_from/to_user() functions return the number of bytes remaining
to be copied but we had intended to return -EFAULT here.

Fixes: bc88ac47d5cb ("scsi: cxlflash: Support AFU debug")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index 7a787b6e21c4..56b6e294ab78 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -3415,9 +3415,10 @@ static int cxlflash_afu_debug(struct cxlflash_cfg *cfg,
if (is_write) {
req_flags |= SISL_REQ_FLAGS_HOST_WRITE;
 
-   rc = copy_from_user(kbuf, ubuf, ulen);
-   if (unlikely(rc))
+   if (copy_from_user(kbuf, ubuf, ulen)) {
+   rc = -EFAULT;
goto out;
+   }
}
}
 
@@ -3445,8 +3446,10 @@ static int cxlflash_afu_debug(struct cxlflash_cfg *cfg,
goto out;
}
 
-   if (ulen && !is_write)
-   rc = copy_to_user(ubuf, kbuf, ulen);
+   if (ulen && !is_write) {
+   if (copy_to_user(ubuf, kbuf, ulen))
+   rc = -EFAULT;
+   }
 out:
kfree(buf);
dev_dbg(dev, "%s: returning rc=%d\n", __func__, rc);

Re: [PATCH] scsi: hisi_sas: silence a static checker warning

[Dan Carpenter]

On Fri, Jun 23, 2017 at 04:25:27PM +0100, John Garry wrote:
> On 23/06/2017 16:15, Dan Carpenter wrote:
> > phy->phy_type is a u64.  We only ever use the first two bits so it's a
> > bit over kill perhaps.
> 
> Hi Dan,
> 
> Right, u64 is unneeded and u32 would suffice, so I think that would be a
> more appropriate fix. And if we change hisi_sas_phy.phy_type to u32, I would
> want to reorder this structure to improve packing efficiency.
> 
> I can make this change unless you really want to...
> 

Feel free to make the change.  :)

regards,
dan carpenter

[PATCH] scsi: hisi_sas: silence a static checker warning

[Dan Carpenter]

phy->phy_type is a u64.  We only ever use the first two bits so it's a
bit over kill perhaps.  Anyway, let's declare the flags as ULL as well
because that lets us do things like "phy->phy_type &= ~PORT_TYPE_SAS;".
In the current code, static checkers complain that that would
unintentionally clear the high 32 bits as well.

Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
index 4fc23087a939..b23245aeab74 100644
--- a/drivers/scsi/hisi_sas/hisi_sas.h
+++ b/drivers/scsi/hisi_sas/hisi_sas.h
@@ -56,8 +56,8 @@
 struct hisi_hba;
 
 enum {
-   PORT_TYPE_SAS = (1U << 1),
-   PORT_TYPE_SATA = (1U << 0),
+   PORT_TYPE_SAS = (1ULL << 1),
+   PORT_TYPE_SATA = (1ULL << 0),
 };
 
 enum dev_status {

[PATCH] bnx2i: missing error code in bnx2i_ep_connect()

[Dan Carpenter]

If bnx2i_map_ep_dbell_regs() then we accidentally return NULL instead
of an error pointer.  It results in a NULL dereference in
iscsi_if_ep_connect().

Fixes: cf4e6363859d ("[SCSI] bnx2i: Add bnx2i iSCSI driver.")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index f32a66f89d25..03c104b47f31 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -1909,7 +1909,8 @@ static struct iscsi_endpoint *bnx2i_ep_connect(struct 
Scsi_Host *shost,
 
bnx2i_ep_active_list_add(hba, bnx2i_ep);
 
-   if (bnx2i_map_ep_dbell_regs(bnx2i_ep))
+   rc = bnx2i_map_ep_dbell_regs(bnx2i_ep);
+   if (rc)
goto del_active_ep;
 
mutex_unlock(>net_dev_lock);

Re: [PATCH RESEND] Eliminate extra 'out_free' label from fcoe_init function

[Dan Carpenter]

On Fri, Jun 02, 2017 at 03:01:29PM +0200, walter harms wrote:
> 
> 
> Am 02.06.2017 14:39, schrieb Milan P. Gandhi:
> > Simplify the check for return code of fcoe_if_init routine
> > in fcoe_init function such that we could eliminate need for
> > extra 'out_free' label and duplicate mutex_unlock statement.
> > 
> > Signed-off-by: Milan P. Gandhi <mgan...@redhat.com>
> > ---
> >  drivers/scsi/fcoe/fcoe.c | 7 +++
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> > index ea21e7b..a2cf3d0 100644
> > --- a/drivers/scsi/fcoe/fcoe.c
> > +++ b/drivers/scsi/fcoe/fcoe.c
> > @@ -2523,14 +2523,13 @@ static int __init fcoe_init(void)
> > fcoe_dev_setup();
> >  
> > rc = fcoe_if_init();
> > +   mutex_unlock(_config_mutex);
> > +
> > if (rc)
> > -   goto out_free;
> > +   goto out_destroy;
> >  
> > -   mutex_unlock(_config_mutex);
> > return 0;
> >  
> if you do that, why not
> if (!rc) return 0;

Gar...  No.  Please don't get creative with the last if statement.

regards,
dan carpenter

Re: [PATCH RESEND] Eliminate extra 'out_free' label from fcoe_init function

[Dan Carpenter]

I'm fine with this version...

regards,
dan carpenter

Re: [PATCH] Remove an extra out label in _fcoe_create function

[Dan Carpenter]

On Thu, Jun 01, 2017 at 05:38:55PM +0530, Milan P. Gandhi wrote:
> This patch removes an extra out label in _fcoe_create function
> where we return if creation of FCOE interface is failed.
> 
> Signed-off-by: Milan P. Gandhi <mgan...@redhat.com>
> ---
>  drivers/scsi/fcoe/fcoe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index 7b960d3..ea21e7b 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -2258,7 +2258,7 @@ static int _fcoe_create(struct net_device *netdev, enum 
> fip_mode fip_mode,
>   fcoe_interface_cleanup(fcoe);
>   mutex_unlock(_config_mutex);
>   fcoe_ctlr_device_delete(ctlr_dev);
> - goto out;
> + return rc;

I don't like do nothing gotos, but I also don't like churn too much.

It doesn't really make sense to do:

rc = -EIO;
...
return rc;

We should probably preserve the error code?  There is a second
"return rc;" later which is more annoying.

drivers/scsi/fcoe/fcoe.c
  2274  /*
  2275   * If the fcoe_ctlr_device is to be set to DISABLED
  2276   * it must be done after the lport is added to the
  2277   * hostlist, but before the rtnl_lock is released.
  2278   * This is because the rtnl_lock protects the
  2279   * hostlist that fcoe_device_notification uses. If
  2280   * the FCoE Controller is intended to be created
  2281   * DISABLED then 'enabled' needs to be considered
  2282   * handling link events. 'enabled' must be set
  2283   * before the lport can be found in the hostlist
  2284   * when a link up event is received.
  2285   */
  2286  if (link_state == FCOE_CREATE_LINK_UP)
  2287  ctlr_dev->enabled = FCOE_CTLR_ENABLED;
  2288  else
  2289  ctlr_dev->enabled = FCOE_CTLR_DISABLED;
  2290  
  2291  if (link_state == FCOE_CREATE_LINK_UP &&
  2292  !fcoe_link_ok(lport)) {
  2293  rtnl_unlock();
  2294  fcoe_ctlr_link_up(ctlr);
  2295  mutex_unlock(_config_mutex);
  2296  return rc;
^
This is the same as "return 0;" and I guess it's supposed to be a
success return?  But it would look more clear if we changed it to return
a literal instead of rc.

  2297  }
  2298  
  2299  out_nodev:
  2300  rtnl_unlock();

regards,
dan carpenter

Re: [PATCH] Eliminate extra 'out_free' label from fcoe_init function

[Dan Carpenter]

On Thu, Jun 01, 2017 at 05:41:06PM +0530, Milan P. Gandhi wrote:
> Simplify the check for return code of fcoe_if_init routine
> in fcoe_init function such that we could eliminate need for
> extra 'out_free' label.
> 
> Signed-off-by: Milan P. Gandhi <mgan...@redhat.com>
> ---
>  drivers/scsi/fcoe/fcoe.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index ea21e7b..fb2a4c9 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -2523,13 +2523,11 @@ static int __init fcoe_init(void)
>   fcoe_dev_setup();
>  
>   rc = fcoe_if_init();
> - if (rc)
> - goto out_free;
> -
> - mutex_unlock(_config_mutex);
> - return 0;
> + if (rc == 0) {
> + mutex_unlock(_config_mutex);
> + return 0;
> + }
>  
> -out_free:
>   mutex_unlock(_config_mutex);

Gar...  Stop!  No1  Don't do this.

Do failure handling, not success handling.

People always think they should get creative with the last if statement
in a function.  This leads to spaghetti code and it's confusing.  Please
never do this again.

The original is correct and the new code is bad rubbish code.

regards,
dan carpenter

Re: [PATCH] scsi: lpfc: fix spelling mistake "entrys" -> "entries"

[Dan Carpenter]

On Fri, May 26, 2017 at 11:11:37AM +0100, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> Trivial fix to spelling mistake in debugfs message
> 

Are you using a tool to find all these spelling mistakes?

regards,
dan carpenter

Re: [PATCH] scsi: qla4xxx: check for null return from iscsi_lookup_endpoint

[Dan Carpenter]

This should be CC'd to qlogic-storage-upstr...@qlogic.com as well.

regards,
dan carpenter

On Sun, May 07, 2017 at 10:30:20PM +0100, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> iscsi_lookup_endpoint can potentially return null and in 9 out of
> the 10 calls to this function a null return is checked, so I think
> it is pertinent to perform a null check here too and return -EINVAL
> as in the other null cases.
> 
> Detected by CoverityScan, CID#147282 ("Dereference null return value")
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  drivers/scsi/qla4xxx/ql4_os.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> index 64c6fa563fdb..803e342e1093 100644
> --- a/drivers/scsi/qla4xxx/ql4_os.c
> +++ b/drivers/scsi/qla4xxx/ql4_os.c
> @@ -3189,6 +3189,8 @@ static int qla4xxx_conn_bind(struct iscsi_cls_session 
> *cls_session,
>   if (iscsi_conn_bind(cls_session, cls_conn, is_leading))
>   return -EINVAL;
>   ep = iscsi_lookup_endpoint(transport_fd);
> + if (!ep)
> + return -EINVAL;
>   conn = cls_conn->dd_data;
>   qla_conn = conn->dd_data;
>   qla_conn->qla_ep = ep->dd_data;
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[bug report] scsi: cxlflash: Add hardware queues attribute

[Dan Carpenter]

Hello Matthew R. Ochs,

This is a semi-automatic email about new static checker warnings.

The patch 3065267a80c8: "scsi: cxlflash: Add hardware queues 
attribute" from Apr 12, 2017, leads to the following Smatch complaint:

drivers/scsi/cxlflash/main.c:722 term_afu()
 warn: variable dereferenced before check 'cfg->afu' (see line 719)

drivers/scsi/cxlflash/main.c
   718   */
   719  for (k = cfg->afu->num_hwqs - 1; k >= 0; k--)
 
Patch introduces a new dereference.

   720  term_intr(cfg, UNMAP_THREE, k);
   721  
   722  if (cfg->afu)

But the existing code assumed it could be NULL.  Presumably it can't?

   723  stop_afu(cfg);
   724  

regards,
dan carpenter