On Mon, Jun 25, 2012 at 06:44:56PM -0700, Dan Williams wrote:
> Can we do a better job of bounding the maximum latency for acceptance
> or rejection of a patch?
I sent quite a few patches all over the kernel as part of my Smatch
work. If I don't get feedback on my patch then I assume it will be
merged. I've gone through my sent-mail box and collected the
patches which weren't merged. Hopefully, this is useful in
discussing maintainer submitter feedback.
Some of these patches are probably wrong but it would have been nice
to get some feedback on that.
The SCSI subsystem obviously stands out as pretty bad. It's the
only subsystem where you can send a patch over and over and not get
a response. It's not that I think all my patches should be merged
without any review and free hugs for everyone... For all the other
patches in this list, I feel that the patches was dropped by
mistake but in SCSI it was silently ignored as a policy.
Causes of patches being lost:
* Poor changelog on a cleanup patch.
* I didn't CC the person who introduced the bug so I never got
their acked-by.
* The patch wasn't obvious and the maintainer was planning to review
it later and forgot.
* Confusion about who was supposed to pull a patch.
* Poor communication between the maintainer and me and I didn't
realize they wanted me to redo the patch. Don't phrase your
statements as questions.
regards,
dan carpenter
If mc == BFI_MC_MAX then we're reading past the end of the
mod->mbhdlr[] array.
Signed-off-by: Dan Carpenter <[email protected]>
---
Originally sent on Wed, 6 Jul 2011.
diff --git a/drivers/scsi/bfa/bfa_ioc.c b/drivers/scsi/bfa/bfa_ioc.c
index 14e6284..8cdb79c 100644
--- a/drivers/scsi/bfa/bfa_ioc.c
+++ b/drivers/scsi/bfa/bfa_ioc.c
@@ -2357,7 +2357,7 @@ bfa_ioc_mbox_isr(struct bfa_ioc_s *ioc)
return;
}
- if ((mc > BFI_MC_MAX) || (mod->mbhdlr[mc].cbfn == NULL))
+ if ((mc >= BFI_MC_MAX) || (mod->mbhdlr[mc].cbfn == NULL))
return;
mod->mbhdlr[mc].cbfn(mod->mbhdlr[mc].cbarg, &m);
If bfad_thread_workq(bfad) was not BFA_STATUS_OK then we freed "im"
and then dereferenced it.
I did a little clean up because it seemed nicer to return directly
instead of doing a superfluous goto. I looked at other functions in
this file and it seems like returning directly is standard.
Signed-off-by: Dan Carpenter <[email protected]>
---
This is the third time I have sent this patch. It was previously sent
on Fri, 29 Jul 2011 and Wed, 29 Feb 2012.
diff --git a/drivers/scsi/bfa/bfad_im.c b/drivers/scsi/bfa/bfad_im.c
index 1ac09af..2eebf8d 100644
--- a/drivers/scsi/bfa/bfad_im.c
+++ b/drivers/scsi/bfa/bfad_im.c
@@ -687,25 +687,21 @@ bfa_status_t
bfad_im_probe(struct bfad_s *bfad)
{
struct bfad_im_s *im;
- bfa_status_t rc = BFA_STATUS_OK;
im = kzalloc(sizeof(struct bfad_im_s), GFP_KERNEL);
- if (im == NULL) {
- rc = BFA_STATUS_ENOMEM;
- goto ext;
- }
+ if (im == NULL)
+ return BFA_STATUS_ENOMEM;
bfad->im = im;
im->bfad = bfad;
if (bfad_thread_workq(bfad) != BFA_STATUS_OK) {
kfree(im);
- rc = BFA_STATUS_FAILED;
+ return BFA_STATUS_FAILED;
}
INIT_WORK(&im->aen_im_notify_work, bfad_aen_im_notify_handler);
-ext:
- return rc;
+ return BFA_STATUS_OK;
}
void
We took this lock with spin_lock() so we should unlock it with
spin_unlock() instead of spin_unlock_irq(). This was introduced in
f2c8dc402b "[SCSI] megaraid_mbox: remove scsi_assign_lock usage".
Signed-off-by: Dan Carpenter <[email protected]>
---
This was originally sent on Sat, 30 Jul 2011. I have cleaned up the
commit message a bit and added Christoph to the CC list.
diff --git a/drivers/scsi/megaraid/megaraid_mbox.c b/drivers/scsi/megaraid/megaraid_mbox.c
index 35bd138..54b1c5b 100644
--- a/drivers/scsi/megaraid/megaraid_mbox.c
+++ b/drivers/scsi/megaraid/megaraid_mbox.c
@@ -2731,7 +2731,7 @@ megaraid_reset_handler(struct scsi_cmnd *scp)
}
out:
- spin_unlock_irq(&adapter->lock);
+ spin_unlock(&adapter->lock);
return rval;
}
scsi_dma_map() returns -ENOMEM on error.
Signed-off-by: Dan Carpenter <[email protected]>
---
Originally sent on Tue, 20 Sep 2011.
diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index 374c4ed..b2c3a1a 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -8426,6 +8426,12 @@ static int asc_build_req(struct asc_board *boardp, struct scsi_cmnd *scp,
/* Build ASC_SCSI_Q */
use_sg = scsi_dma_map(scp);
+ if (use_sg < 0) {
+ scsi_dma_unmap(scp);
+ scp->result = HOST_BYTE(DID_SOFT_ERROR);
+ return ASC_ERROR;
+ }
+
if (use_sg != 0) {
int sgcnt;
struct scatterlist *slp;
This was supposed to be a subsystem specific error code. It makes
static checkers complain because "warn_code" is unsigned short so it
can't store negatives.
Signed-off-by: Dan Carpenter <[email protected]>
---
Originally sent on Tue, 20 Sep 2011.
diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index b2c3a1a..4212586 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -4724,7 +4724,7 @@ static ushort AscInitMicroCodeVar(ASC_DVC_VAR *asc_dvc)
asc_dvc->overrun_dma = dma_map_single(board->dev, asc_dvc->overrun_buf,
ASC_OVERRUN_BSIZE, DMA_FROM_DEVICE);
if (dma_mapping_error(board->dev, asc_dvc->overrun_dma)) {
- warn_code = -ENOMEM;
+ warn_code = UW_ERR;
goto err_dma_map;
}
phy_addr = cpu_to_le32(asc_dvc->overrun_dma);
I don't think we're actually likely to hit this limit but if we do
then the comparison should be done as size_t. The original code
is equivalent to:
len = strlen(sptr) % USHRT_MAX;
Signed-off-by: Dan Carpenter <[email protected]>
---
I was told this patch "has already made it upstream via the v9fs pull."
but it must have been dropped accidentally. Originally sent on Sat,
Jan 15, 2011.
diff --git a/net/9p/protocol.c b/net/9p/protocol.c
index 9ee48cb..3d33ecf 100644
--- a/net/9p/protocol.c
+++ b/net/9p/protocol.c
@@ -368,7 +368,7 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
const char *sptr = va_arg(ap, const char *);
uint16_t len = 0;
if (sptr)
- len = min_t(uint16_t, strlen(sptr),
+ len = min_t(size_t, strlen(sptr),
USHRT_MAX);
errcode = p9pdu_writef(pdu, proto_version,
"k_tmp->len" and "total" are unsigned integers. The first message
could be close to "bufsiz" (4096) and then the next message could be
4GB which would cause an integer overflow.
Signed-off-by: Dan Carpenter <[email protected]>
---
I don't have a way to test this. I originally sent this message on Tue,
18 Oct 2011. I'm not totally sure what the implications are but it
seemed like there might be security implications. I honestly don't
know. I never received any feedback on the patch.
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 830adbe..aab05e1 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -241,7 +241,7 @@ static int spidev_message(struct spidev_data *spidev,
k_tmp->len = u_tmp->len;
total += k_tmp->len;
- if (total > bufsiz) {
+ if (total > bufsiz || total < k_tmp->len) {
status = -EMSGSIZE;
goto done;
}
The ->cmd_idx field is 8 bits, not 16 so the call to cpu_to_le16()
will probably set cmd_idx to zero here on big endian systems. My guess
is that this works because it has been tested on little endian systems
where the conversion to le16 is a nop.
Signed-off-by: Dan Carpenter <[email protected]>
---
I don't have a way to test this.
I have added a sentence to the changelog. I have added David Vrabel to
the CC list. This was originally sent to the mailing list on Mon, 21
Nov 2011 and there was no response.
diff --git a/drivers/mmc/host/ushc.c b/drivers/mmc/host/ushc.c
index c0105a2..25932f5 100644
--- a/drivers/mmc/host/ushc.c
+++ b/drivers/mmc/host/ushc.c
@@ -278,7 +278,7 @@ static void ushc_request(struct mmc_host *mmc, struct mmc_request *req)
ushc->current_req = req;
/* Start cmd with CBW. */
- ushc->cbw->cmd_idx = cpu_to_le16(req->cmd->opcode);
+ ushc->cbw->cmd_idx = req->cmd->opcode;
if (req->data)
ushc->cbw->block_size = cpu_to_le16(req->data->blksz);
else
The code here has a nested spin_lock_irqsave(). It's not needed since
IRQs are already disabled and it causes a problem because it means that
IRQs won't be enabled again at the end. The second call to
spin_lock_irqsave() will overwrite the value of irq_flags and we can't
restore the proper settings.
Signed-off-by: Dan Carpenter <[email protected]>
---
I have added Jack Steiner to the CC list. This was originally sent on
Thu, 15 Dec 2011.
diff --git a/drivers/misc/sgi-xp/xpc_uv.c b/drivers/misc/sgi-xp/xpc_uv.c
index 17bbacb..87b251a 100644
--- a/drivers/misc/sgi-xp/xpc_uv.c
+++ b/drivers/misc/sgi-xp/xpc_uv.c
@@ -452,9 +452,9 @@ xpc_handle_activate_mq_msg_uv(struct xpc_partition *part,
if (msg->activate_gru_mq_desc_gpa !=
part_uv->activate_gru_mq_desc_gpa) {
- spin_lock_irqsave(&part_uv->flags_lock, irq_flags);
+ spin_lock(&part_uv->flags_lock);
part_uv->flags &= ~XPC_P_CACHED_ACTIVATE_GRU_MQ_DESC_UV;
- spin_unlock_irqrestore(&part_uv->flags_lock, irq_flags);
+ spin_unlock(&part_uv->flags_lock);
part_uv->activate_gru_mq_desc_gpa =
msg->activate_gru_mq_desc_gpa;
}
request_size is zero here so this condition is always false. Also we
already handled negative values some lines earlier so it's not negative
for that reason as well.
It looks like this was introduced because we applied Dan Rosenberg's fix
twice by mistake:
b5b515445f4 "[SCSI] pmcraid: reject negative request size"
5f6279da376 "[SCSI] pmcraid: reject negative request size"
Signed-off-by: Dan Carpenter <[email protected]>
---
I have tweaked the changelog slightly. This was originally sent on
Fri, 16 Dec 2011.
diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index ea8a0b4..d81a159 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -3870,9 +3870,6 @@ static long pmcraid_ioctl_passthrough(
pmcraid_err("couldn't build passthrough ioadls\n");
goto out_free_buffer;
}
- } else if (request_size < 0) {
- rc = -EINVAL;
- goto out_free_buffer;
}
/* If data is being written into the device, copy the data from user
The cpu_to_le32() truncates the address to 32 bits. All the other
places that set .address use cpu_to_le64().
Signed-off-by: Dan Carpenter <[email protected]>
---
What about if dma_addr_t is 32 bits on a big endian system? Can that
happen?
This patch was originally sent on Fri, 16 Dec 2011 and I sent a second
reminder on Mon, Apr 30, 2012. I was silently ignored both times.
diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index d81a159..a38dade 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -1242,7 +1242,7 @@ static struct pmcraid_cmd *pmcraid_init_hcam
ioadl[0].flags |= IOADL_FLAGS_READ_LAST;
ioadl[0].data_len = cpu_to_le32(rcb_size);
- ioadl[0].address = cpu_to_le32(dma);
+ ioadl[0].address = cpu_to_le64(dma);
cmd->cmd_done = cmd_done;
return cmd;
The find_first_zero_bit() function takes the size in bits not bytes.
Signed-off-by: Dan Carpenter <[email protected]>
---
This patch was originally sent on Wed, 2 May 2012.
diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index a38dade..719619a 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -5376,7 +5376,7 @@ static unsigned short pmcraid_get_minor(void)
{
int minor;
- minor = find_first_zero_bit(pmcraid_minor, sizeof(pmcraid_minor));
+ minor = find_first_zero_bit(pmcraid_minor, PMCRAID_MAX_ADAPTERS);
__set_bit(minor, pmcraid_minor);
return minor;
}
These are __iomem. Sparse complains if we don't have that.
drivers/scsi/isci/phy.c +149 70: warning:
incorrect type in initializer (different address spaces)
Signed-off-by: Dan Carpenter <[email protected]>
---
Sent on Wed, 18 Jan 2012.
diff --git a/drivers/scsi/isci/host.c b/drivers/scsi/isci/host.c
index bc8981e..4a095f3 100644
--- a/drivers/scsi/isci/host.c
+++ b/drivers/scsi/isci/host.c
@@ -1973,7 +1973,7 @@ static void sci_controller_afe_initialization(struct isci_host *ihost)
}
for (phy_id = 0; phy_id < SCI_MAX_PHYS; phy_id++) {
- struct scu_afe_transceiver *xcvr = &afe->scu_afe_xcvr[phy_id];
+ struct scu_afe_transceiver __iomem *xcvr = &afe->scu_afe_xcvr[phy_id];
const struct sci_phy_oem_params *oem_phy = &oem->phys[phy_id];
int cable_length_long =
is_long_cable(phy_id, cable_selection_mask);
diff --git a/drivers/scsi/isci/phy.c b/drivers/scsi/isci/phy.c
index 18f43d4..7b60353 100644
--- a/drivers/scsi/isci/phy.c
+++ b/drivers/scsi/isci/phy.c
@@ -169,7 +169,7 @@ sci_phy_link_layer_initialization(struct isci_phy *iphy,
phy_cap.gen1_no_ssc = 1;
if (ihost->oem_parameters.controller.do_enable_ssc) {
struct scu_afe_registers __iomem *afe = &ihost->scu_registers->afe;
- struct scu_afe_transceiver *xcvr = &afe->scu_afe_xcvr[phy_idx];
+ struct scu_afe_transceiver __iomem *xcvr = &afe->scu_afe_xcvr[phy_idx];
struct isci_pci_info *pci_info = to_pci_info(ihost->pdev);
bool en_sas = false;
bool en_sata = false;
Hi,
This bug is still present in linux-next.
regards,
dan carpenter
On Wed, Jan 11, 2012 at 12:32:34PM +0300, Dan Carpenter wrote:
> Hello Krishna Gudipati,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 5b7db7af522d: "[SCSI] bfa: Implement LUN Masking feature
> using the SCSI Slave Callouts." from Dec 20, 2011, leads to the
> following Smatch complaint:
>
> drivers/scsi/bfa/bfad_im.c +962 bfad_im_slave_alloc()
> warn: variable dereferenced before check 'rport' (see line 959)
>
> drivers/scsi/bfa/bfad_im.c
> 957 struct fc_rport *rport = starget_to_rport(scsi_target(sdev));
> 958 struct bfad_itnim_data_s *itnim_data =
> 959 (struct bfad_itnim_data_s *) rport->dd_data;
> ^^^^^^^
> New dereference.
>
> 960 struct bfa_s *bfa = itnim_data->itnim->bfa_itnim->bfa;
> 961
> 962 if (!rport || fc_remote_port_chkready(rport))
> ^^^^^
> Old check.
>
> 963 return -ENXIO;
> 964
>
> regards,
> dan carpenter
>
We should return here and avoid a NULL dereference.
Signed-off-by: Dan Carpenter <[email protected]>
---
Originally sent on Fri, 20 Jan 2012.
diff --git a/drivers/block/nvme.c b/drivers/block/nvme.c
index 38a2d06..b4d85b9 100644
--- a/drivers/block/nvme.c
+++ b/drivers/block/nvme.c
@@ -1037,6 +1037,8 @@ static struct nvme_iod *nvme_map_user_pages(struct nvme_dev *dev, int write,
offset = offset_in_page(addr);
count = DIV_ROUND_UP(offset + length, PAGE_SIZE);
pages = kcalloc(count, sizeof(*pages), GFP_KERNEL);
+ if (!pages)
+ return ERR_PTR(-ENOMEM);
err = get_user_pages_fast(addr, count, 1, pages);
if (err < count) {
The intent here was to test that the flag was clear but the '!' has
higher precedence than the '&'. I2C_M_RD is 0x1 so the current code is
equivalent to "&& (!sgs[i].flags) ..."
Signed-off-by: Dan Carpenter <[email protected]>
---
I sent this originally on Wed, 25 Jan 2012 and Emil Goode sent the same
fix on Thu, May 3, 2012.
diff --git a/drivers/media/dvb/dvb-usb/az6007.c b/drivers/media/dvb/dvb-usb/az6007.c
index 4008b9c..f6f0cf9 100644
--- a/drivers/media/dvb/dvb-usb/az6007.c
+++ b/drivers/media/dvb/dvb-usb/az6007.c
@@ -711,7 +711,7 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
addr = msgs[i].addr << 1;
if (((i + 1) < num)
&& (msgs[i].len == 1)
- && (!msgs[i].flags & I2C_M_RD)
+ && (!(msgs[i].flags & I2C_M_RD))
&& (msgs[i + 1].flags & I2C_M_RD)
&& (msgs[i].addr == msgs[i + 1].addr)) {
/*
"pvt->ambase" is a u64 datatype. The intent here is to fill the first
half in the first call to pci_read_config_dword() and the other half in
the second. Unfortunately the pointer math is wrong so we set the wrong
data.
Signed-off-by: Dan Carpenter <[email protected]>
---
v2: Redid it as with a union as Walter Harms suggested.
Fixed the same bug in i5400_edac.c as well.
v3: Make the struct __packed just in case.
I don't have this hardware, so please review carefully. But the
original code obviously corrupts memory so my patch could hardly be
worse...
This was originally sent on Mon, Mar 5, 2012. Btw, there are some
Sparse errors in these files. The code looks buggy but I don't know
what the intent was.
drivers/edac/i5000_edac.c:485:15: warning: right shift by bigger than source value
drivers/edac/i5000_edac.c:580:23: warning: right shift by bigger than source value
drivers/edac/i5400_edac.c:391:36: warning: right shift by bigger than source value
drivers/edac/i5400_edac.c:401:37: warning: right shift by bigger than source value
diff --git a/drivers/edac/i5000_edac.c b/drivers/edac/i5000_edac.c
index a5c33df..39c6375 100644
--- a/drivers/edac/i5000_edac.c
+++ b/drivers/edac/i5000_edac.c
@@ -328,7 +328,13 @@ struct i5000_pvt {
struct pci_dev *branch_1; /* 22.0 */
u16 tolm; /* top of low memory */
- u64 ambase; /* AMB BAR */
+ union {
+ u64 ambase; /* AMB BAR */
+ struct {
+ u32 ambase_bottom;
+ u32 ambase_top;
+ } u __packed;
+ };
u16 mir0, mir1, mir2;
@@ -1131,9 +1137,9 @@ static void i5000_get_mc_regs(struct mem_ctl_info *mci)
pvt = mci->pvt_info;
pci_read_config_dword(pvt->system_address, AMBASE,
- (u32 *) & pvt->ambase);
+ &pvt->u.ambase_bottom);
pci_read_config_dword(pvt->system_address, AMBASE + sizeof(u32),
- ((u32 *) & pvt->ambase) + sizeof(u32));
+ &pvt->u.ambase_top);
maxdimmperch = pvt->maxdimmperch;
maxch = pvt->maxch;
diff --git a/drivers/edac/i5400_edac.c b/drivers/edac/i5400_edac.c
index 50069c6..2772469 100644
--- a/drivers/edac/i5400_edac.c
+++ b/drivers/edac/i5400_edac.c
@@ -327,7 +327,13 @@ struct i5400_pvt {
struct pci_dev *branch_1; /* 22.0 */
u16 tolm; /* top of low memory */
- u64 ambase; /* AMB BAR */
+ union {
+ u64 ambase; /* AMB BAR */
+ struct {
+ u32 ambase_bottom;
+ u32 ambase_top;
+ } u __packed;
+ };
u16 mir0, mir1;
@@ -1055,9 +1061,9 @@ static void i5400_get_mc_regs(struct mem_ctl_info *mci)
pvt = mci->pvt_info;
pci_read_config_dword(pvt->system_address, AMBASE,
- (u32 *) &pvt->ambase);
+ &pvt->u.ambase_bottom);
pci_read_config_dword(pvt->system_address, AMBASE + sizeof(u32),
- ((u32 *) &pvt->ambase) + sizeof(u32));
+ &pvt->u.ambase_top);
maxdimmperch = pvt->maxdimmperch;
maxch = pvt->maxch;
On 64 bit systems the current code sets 32 bits of "seg" and leaves the
other 32 uninitialized. It doesn't matter since the variable is never
used. But it's still messy and we should fix it.
Signed-off-by: Dan Carpenter <[email protected]>
---
Originally sent on Fri, 2 Mar 2012.
diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 4d39a9f..97825f1 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -524,7 +524,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
mega_passthru *pthru;
scb_t *scb;
mbox_t *mbox;
- long seg;
+ u32 seg;
char islogical;
int max_ldrv_num;
int channel = 0;
@@ -858,7 +858,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
/* Calculate Scatter-Gather info */
mbox->m_out.numsgelements = mega_build_sglist(adapter, scb,
- (u32 *)&mbox->m_out.xferaddr, (u32 *)&seg);
+ (u32 *)&mbox->m_out.xferaddr, &seg);
return scb;
write_file_bool() modifies 32 bits of data, so "amd_iommu_unmap_flush"
needs to be 32 bits as well or we'll corrupt memory. Fortunately it
looks like the data is aligned with a gap after the declaration so this
is harmless in production.
Signed-off-by: Dan Carpenter <[email protected]>
---
Originally sent on Fri, 2 Mar 2012. This is the other patch where I
guess I was supposed to modify debugfs_create_bool() to take bool
pointers.
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 2435555..c1b1d48 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -652,7 +652,7 @@ extern unsigned long *amd_iommu_pd_alloc_bitmap;
* If true, the addresses will be flushed on unmap time, not when
* they are reused
*/
-extern bool amd_iommu_unmap_flush;
+extern u32 amd_iommu_unmap_flush;
/* Smallest number of PASIDs supported by any IOMMU in the system */
extern u32 amd_iommu_max_pasids;
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 4127b56..92b9267 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -451,7 +451,7 @@ static void amd_iommu_stats_init(void)
return;
de_fflush = debugfs_create_bool("fullflush", 0444, stats_dir,
- (u32 *)&amd_iommu_unmap_flush);
+ &amd_iommu_unmap_flush);
amd_iommu_stats_add(&compl_wait);
amd_iommu_stats_add(&cnt_map_single);
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index c04ddca..a33612f 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -129,7 +129,7 @@ u16 amd_iommu_last_bdf; /* largest PCI device id we have
to handle */
LIST_HEAD(amd_iommu_unity_map); /* a list of required unity mappings
we find in ACPI */
-bool amd_iommu_unmap_flush; /* if true, flush on every unmap */
+u32 amd_iommu_unmap_flush; /* if true, flush on every unmap */
LIST_HEAD(amd_iommu_list); /* list of all AMD IOMMUs in the
system */
Even though it has "bool" in the name, you have pass a u32 pointer to
debugfs_create_bool(). Otherwise you get memory corruption in
write_file_bool(). Fortunately in this case the corruption happens in
an alignment hole between variables so it doesn't cause any problems.
Signed-off-by: Dan Carpenter <[email protected]>
---
This was sent on Fri, 2 Mar 2012.
It's probably my fault the bug is still there. I submitted the patch
but the feed back was that debugfs_create_bool() has a stupid API and
should take a pointer to a bool. I said that yes, probably that is true
but only two places use the current API incorrectly and I fixed the
other place as well.
I assumed that both my patches would be merged and the person who cared
about API would fix the API but neither of my fixes were applied.
diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index 518aea7..66ce414 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -78,7 +78,7 @@ static LIST_HEAD(free_entries);
static DEFINE_SPINLOCK(free_entries_lock);
/* Global disable flag - will be set in case of an error */
-static bool global_disable __read_mostly;
+static u32 global_disable __read_mostly;
/* Global error count */
static u32 error_count;
@@ -657,7 +657,7 @@ static int dma_debug_fs_init(void)
global_disable_dent = debugfs_create_bool("disabled", 0444,
dma_debug_dent,
- (u32 *)&global_disable);
+ &global_disable);
if (!global_disable_dent)
goto out_err;
Sparse complains that we redeclare this with a different type, because
in the .c file we use an enum and in the .h file we declare the
parameter as a u32. Probably it's best to use an enum in both places.
Signed-off-by: Dan Carpenter <[email protected]>
---
I originally sent this on Tue, 27 Mar 2012, and Emil Goode sent the same
patch on Fri, May 4, 2012. Both were silently ignored.
diff --git a/drivers/scsi/isci/remote_node_context.h b/drivers/scsi/isci/remote_node_context.h
index a703b9c..c7ee81d 100644
--- a/drivers/scsi/isci/remote_node_context.h
+++ b/drivers/scsi/isci/remote_node_context.h
@@ -212,7 +212,7 @@ enum sci_status sci_remote_node_context_destruct(struct sci_remote_node_context
scics_sds_remote_node_context_callback callback,
void *callback_parameter);
enum sci_status sci_remote_node_context_suspend(struct sci_remote_node_context *sci_rnc,
- u32 suspend_type,
+ enum sci_remote_node_suspension_reasons reason,
u32 suspension_code);
enum sci_status sci_remote_node_context_resume(struct sci_remote_node_context *sci_rnc,
scics_sds_remote_node_context_callback cb_fn,
These are unintuitive. These are type bool and return -1 casted to true
on failure. Let's just make it return an int. The callers don't care,
but let's change this as a cleanup.
Signed-off-by: Dan Carpenter <[email protected]>
Acked-by: Daniel Vetter <[email protected]>
---
Originally sent on Wed, Mar 28, 2012. This one is prossibly my fault.
I assumed these would be in the same tree, but apparently I should have
broken them out. Daniel acked the patch and told me that if I wanted to
I could break it up and he could merge part (or all of it?). I have no
problem with redoing patches but I understood that to mean it was going
to go through a someone else's tree with his Acked-by. If you give me
an option, of course I'm going to pick the laziest one. That is my firm
promise and commitment.
diff --git a/drivers/gpu/drm/gma500/intel_bios.h b/drivers/gpu/drm/gma500/intel_bios.h
index 0a73866..2e95523 100644
--- a/drivers/gpu/drm/gma500/intel_bios.h
+++ b/drivers/gpu/drm/gma500/intel_bios.h
@@ -431,7 +431,7 @@ struct bdb_driver_features {
u8 custom_vbt_version;
} __attribute__((packed));
-extern bool psb_intel_init_bios(struct drm_device *dev);
+extern int psb_intel_init_bios(struct drm_device *dev);
extern void psb_intel_destroy_bios(struct drm_device *dev);
/*
diff --git a/drivers/gpu/drm/gma500/intel_bios.c b/drivers/gpu/drm/gma500/intel_bios.c
index 973d7f6..8d7caf0 100644
--- a/drivers/gpu/drm/gma500/intel_bios.c
+++ b/drivers/gpu/drm/gma500/intel_bios.c
@@ -427,7 +427,7 @@ parse_device_mapping(struct drm_psb_private *dev_priv,
*
* Returns 0 on success, nonzero on failure.
*/
-bool psb_intel_init_bios(struct drm_device *dev)
+int psb_intel_init_bios(struct drm_device *dev)
{
struct drm_psb_private *dev_priv = dev->dev_private;
struct pci_dev *pdev = dev->pdev;
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index dbda6e3..31c2107 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -476,7 +476,7 @@ struct bdb_edp {
} __attribute__ ((packed));
void intel_setup_bios(struct drm_device *dev);
-bool intel_parse_bios(struct drm_device *dev);
+int intel_parse_bios(struct drm_device *dev);
/*
* Driver<->VBIOS interaction occurs through scratch bits in
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 3534593..8c60741 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -692,7 +692,7 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
*
* Returns 0 on success, nonzero on failure.
*/
-bool
+int
intel_parse_bios(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
Inside the error handling in lp5523_init_led(), there is a place that
calls to led_classdev_unregister(). When we unregister the LED drivers,
it tries to set the brightness to OFF. In this driver setting the
brightness is done through a work queue and the work queue hasn't been
initialized yet.
The result is that we trigger a WARN_ON() in the __queue_work().
The fix is to move the INIT_WORK() in front of the call to
lp5523_init_led().
Matt Renzelmann found this using a bug finding tool.
Reported-by: Matt Renzelmann <[email protected]>
Signed-off-by: Dan Carpenter <[email protected]>
---
I don't have this hardware, so I can't test it. I originally sent this
on Fri, 13 Apr 2012, and that was before Bryan Wu took on the LED
subsystem. Also when I sent it, the WARN_ON() in __queue_work() was
a BUG_ON() so I've updated the commit message.
diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 857a3e1..e8a2712 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -943,6 +943,9 @@ static int __devinit lp5523_probe(struct i2c_client *client,
if (pdata->led_config[i].led_current == 0)
continue;
+ INIT_WORK(&chip->leds[led].brightness_work,
+ lp5523_led_brightness_work);
+
ret = lp5523_init_led(&chip->leds[led], &client->dev, i, pdata);
if (ret) {
dev_err(&client->dev, "error initializing leds\n");
@@ -956,9 +959,6 @@ static int __devinit lp5523_probe(struct i2c_client *client,
LP5523_REG_LED_CURRENT_BASE + chip->leds[led].chan_nr,
chip->leds[led].led_current);
- INIT_WORK(&(chip->leds[led].brightness_work),
- lp5523_led_brightness_work);
-
led++;
}
envelope->attack_level is a u16 type. We're trying to clamp it here
so it's between 0 and 0x7fff. Unfortunately, the cast to __s16 turns
all the values larger than 0x7fff into negative numbers and min_t()
thinks they are less than 0x7fff. envelope_level is an int so now
we've got negative values stored there.
Signed-off-by: Dan Carpenter <[email protected]>
---
Originally sent on Mon, 26 Sep 2011. I have added Anssi Hannula to the
CC list.
diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
index 5f55885..b107922 100644
--- a/drivers/input/ff-memless.c
+++ b/drivers/input/ff-memless.c
@@ -176,7 +176,7 @@ static int apply_envelope(struct ml_effect_state *state, int value,
value, envelope->attack_level);
time_from_level = jiffies_to_msecs(now - state->play_at);
time_of_envelope = envelope->attack_length;
- envelope_level = min_t(__s16, envelope->attack_level, 0x7fff);
+ envelope_level = min_t(u16, envelope->attack_level, 0x7fff);
} else if (envelope->fade_length && effect->replay.length &&
time_after(now,
@@ -184,7 +184,7 @@ static int apply_envelope(struct ml_effect_state *state, int value,
time_before(now, state->stop_at)) {
time_from_level = jiffies_to_msecs(state->stop_at - now);
time_of_envelope = envelope->fade_length;
- envelope_level = min_t(__s16, envelope->fade_level, 0x7fff);
+ envelope_level = min_t(u16, envelope->fade_level, 0x7fff);
} else
return value;
This code isn't reachable anymore after f073cc8f39 "x86, UV: Clean up
uv_tlb.c" because we will always hit a continue statement. This causes
a Smatch message:
arch/x86/platform/uv/tlb_uv.c:1531 parse_tunables_write()
info: ignoring unreachable code.
Signed-off-by: Dan Carpenter <[email protected]>
---
I have cleaned up the commit message. This was originally sent on
Sun, 7 Aug 2011.
diff --git a/arch/x86/platform/uv/tlb_uv.c b/arch/x86/platform/uv/tlb_uv.c
index 59880af..d625792 100644
--- a/arch/x86/platform/uv/tlb_uv.c
+++ b/arch/x86/platform/uv/tlb_uv.c
@@ -1528,8 +1528,6 @@ static int parse_tunables_write(struct bau_control *bcp, char *instr,
*tunables[cnt].tunp = val;
continue;
}
- if (q == p)
- break;
}
return 0;
}
On Wed, Jun 27, 2012 at 5:10 PM, Dan Carpenter <[email protected]> wrote:
> Inside the error handling in lp5523_init_led(), there is a place that
> calls to led_classdev_unregister(). ?When we unregister the LED drivers,
> it tries to set the brightness to OFF. ?In this driver setting the
> brightness is done through a work queue and the work queue hasn't been
> initialized yet.
>
> The result is that we trigger a WARN_ON() in the __queue_work().
>
> The fix is to move the INIT_WORK() in front of the call to
> lp5523_init_led().
>
Thanks for resending this, I applied this in my for-next branch.
> Matt Renzelmann found this using a bug finding tool.
Just be curious, what's kind of the tool here?
-Bryan
>
> Reported-by: Matt Renzelmann <[email protected]>
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> I don't have this hardware, so I can't test it. ?I originally sent this
> on Fri, 13 Apr 2012, and that was before Bryan Wu took on the LED
> subsystem. ?Also when I sent it, the WARN_ON() in __queue_work() was
> a BUG_ON() so I've updated the commit message.
>
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index 857a3e1..e8a2712 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -943,6 +943,9 @@ static int __devinit lp5523_probe(struct i2c_client *client,
> ? ? ? ? ? ? ? ?if (pdata->led_config[i].led_current == 0)
> ? ? ? ? ? ? ? ? ? ? ? ?continue;
>
> + ? ? ? ? ? ? ? INIT_WORK(&chip->leds[led].brightness_work,
> + ? ? ? ? ? ? ? ? ? ? ? lp5523_led_brightness_work);
> +
> ? ? ? ? ? ? ? ?ret = lp5523_init_led(&chip->leds[led], &client->dev, i, pdata);
> ? ? ? ? ? ? ? ?if (ret) {
> ? ? ? ? ? ? ? ? ? ? ? ?dev_err(&client->dev, "error initializing leds\n");
> @@ -956,9 +959,6 @@ static int __devinit lp5523_probe(struct i2c_client *client,
> ? ? ? ? ? ? ? ? ? ? ? ? ?LP5523_REG_LED_CURRENT_BASE + chip->leds[led].chan_nr,
> ? ? ? ? ? ? ? ? ? ? ? ? ?chip->leds[led].led_current);
>
> - ? ? ? ? ? ? ? INIT_WORK(&(chip->leds[led].brightness_work),
> - ? ? ? ? ? ? ? ? ? ? ? lp5523_led_brightness_work);
> -
> ? ? ? ? ? ? ? ?led++;
> ? ? ? ?}
>
--
Bryan Wu <[email protected]>
Kernel Developer ? ?+86.186-168-78255 Mobile
Canonical Ltd. ? ? ?http://www.canonical.com
Ubuntu - Linux for human beings | http://www.ubuntu.com
On Wed, Jun 27, 2012 at 06:49:10PM +0800, Bryan Wu wrote:
> On Wed, Jun 27, 2012 at 5:10 PM, Dan Carpenter <[email protected]> wrote:
> > Inside the error handling in lp5523_init_led(), there is a place that
> > calls to led_classdev_unregister(). ?When we unregister the LED drivers,
> > it tries to set the brightness to OFF. ?In this driver setting the
> > brightness is done through a work queue and the work queue hasn't been
> > initialized yet.
> >
> > The result is that we trigger a WARN_ON() in the __queue_work().
> >
> > The fix is to move the INIT_WORK() in front of the call to
> > lp5523_init_led().
> >
>
> Thanks for resending this, I applied this in my for-next branch.
>
> > Matt Renzelmann found this using a bug finding tool.
>
> Just be curious, what's kind of the tool here?
>
Sorry I should have CC'd Matt on this. I think it's something he
is working on at the University of Wisconsin. That's all I know.
regards,
dan carpenter
> -Bryan
>
> >
> > Reported-by: Matt Renzelmann <[email protected]>
> > Signed-off-by: Dan Carpenter <[email protected]>
> > ---
> > I don't have this hardware, so I can't test it. ?I originally sent this
> > on Fri, 13 Apr 2012, and that was before Bryan Wu took on the LED
> > subsystem. ?Also when I sent it, the WARN_ON() in __queue_work() was
> > a BUG_ON() so I've updated the commit message.
> >
> > diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> > index 857a3e1..e8a2712 100644
> > --- a/drivers/leds/leds-lp5523.c
> > +++ b/drivers/leds/leds-lp5523.c
> > @@ -943,6 +943,9 @@ static int __devinit lp5523_probe(struct i2c_client *client,
> > ? ? ? ? ? ? ? ?if (pdata->led_config[i].led_current == 0)
> > ? ? ? ? ? ? ? ? ? ? ? ?continue;
> >
> > + ? ? ? ? ? ? ? INIT_WORK(&chip->leds[led].brightness_work,
> > + ? ? ? ? ? ? ? ? ? ? ? lp5523_led_brightness_work);
> > +
> > ? ? ? ? ? ? ? ?ret = lp5523_init_led(&chip->leds[led], &client->dev, i, pdata);
> > ? ? ? ? ? ? ? ?if (ret) {
> > ? ? ? ? ? ? ? ? ? ? ? ?dev_err(&client->dev, "error initializing leds\n");
> > @@ -956,9 +959,6 @@ static int __devinit lp5523_probe(struct i2c_client *client,
> > ? ? ? ? ? ? ? ? ? ? ? ? ?LP5523_REG_LED_CURRENT_BASE + chip->leds[led].chan_nr,
> > ? ? ? ? ? ? ? ? ? ? ? ? ?chip->leds[led].led_current);
> >
> > - ? ? ? ? ? ? ? INIT_WORK(&(chip->leds[led].brightness_work),
> > - ? ? ? ? ? ? ? ? ? ? ? lp5523_led_brightness_work);
> > -
> > ? ? ? ? ? ? ? ?led++;
> > ? ? ? ?}
> >
>
>
>
> --
> Bryan Wu <[email protected]>
> Kernel Developer ? ?+86.186-168-78255 Mobile
> Canonical Ltd. ? ? ?http://www.canonical.com
> Ubuntu - Linux for human beings | http://www.ubuntu.com
On Wed, Jun 27, 2012 at 12:08:55PM +0300, Dan Carpenter wrote:
> Even though it has "bool" in the name, you have pass a u32 pointer to
> debugfs_create_bool(). Otherwise you get memory corruption in
> write_file_bool(). Fortunately in this case the corruption happens in
> an alignment hole between variables so it doesn't cause any problems.
>
> Signed-off-by: Dan Carpenter <[email protected]>
Acked-by: Neil Horman <[email protected]>
> ---
> This was sent on Fri, 2 Mar 2012.
>
> It's probably my fault the bug is still there. I submitted the patch
> but the feed back was that debugfs_create_bool() has a stupid API and
> should take a pointer to a bool. I said that yes, probably that is true
> but only two places use the current API incorrectly and I fixed the
> other place as well.
>
> I assumed that both my patches would be merged and the person who cared
> about API would fix the API but neither of my fixes were applied.
>
> diff --git a/lib/dma-debug.c b/lib/dma-debug.c
> index 518aea7..66ce414 100644
> --- a/lib/dma-debug.c
> +++ b/lib/dma-debug.c
> @@ -78,7 +78,7 @@ static LIST_HEAD(free_entries);
> static DEFINE_SPINLOCK(free_entries_lock);
>
> /* Global disable flag - will be set in case of an error */
> -static bool global_disable __read_mostly;
> +static u32 global_disable __read_mostly;
>
> /* Global error count */
> static u32 error_count;
> @@ -657,7 +657,7 @@ static int dma_debug_fs_init(void)
>
> global_disable_dent = debugfs_create_bool("disabled", 0444,
> dma_debug_dent,
> - (u32 *)&global_disable);
> + &global_disable);
> if (!global_disable_dent)
> goto out_err;
>
>
Em 27-06-2012 06:07, Dan Carpenter escreveu:
> "pvt->ambase" is a u64 datatype. The intent here is to fill the first
> half in the first call to pci_read_config_dword() and the other half in
> the second. Unfortunately the pointer math is wrong so we set the wrong
> data.
Applied, thanks!
Mauro
>
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> v2: Redid it as with a union as Walter Harms suggested.
> Fixed the same bug in i5400_edac.c as well.
> v3: Make the struct __packed just in case.
>
> I don't have this hardware, so please review carefully. But the
> original code obviously corrupts memory so my patch could hardly be
> worse...
>
> This was originally sent on Mon, Mar 5, 2012. Btw, there are some
> Sparse errors in these files. The code looks buggy but I don't know
> what the intent was.
>
> drivers/edac/i5000_edac.c:485:15: warning: right shift by bigger than source value
> drivers/edac/i5000_edac.c:580:23: warning: right shift by bigger than source value
> drivers/edac/i5400_edac.c:391:36: warning: right shift by bigger than source value
> drivers/edac/i5400_edac.c:401:37: warning: right shift by bigger than source value
>
> diff --git a/drivers/edac/i5000_edac.c b/drivers/edac/i5000_edac.c
> index a5c33df..39c6375 100644
> --- a/drivers/edac/i5000_edac.c
> +++ b/drivers/edac/i5000_edac.c
> @@ -328,7 +328,13 @@ struct i5000_pvt {
> struct pci_dev *branch_1; /* 22.0 */
>
> u16 tolm; /* top of low memory */
> - u64 ambase; /* AMB BAR */
> + union {
> + u64 ambase; /* AMB BAR */
> + struct {
> + u32 ambase_bottom;
> + u32 ambase_top;
> + } u __packed;
> + };
>
> u16 mir0, mir1, mir2;
>
> @@ -1131,9 +1137,9 @@ static void i5000_get_mc_regs(struct mem_ctl_info *mci)
> pvt = mci->pvt_info;
>
> pci_read_config_dword(pvt->system_address, AMBASE,
> - (u32 *) & pvt->ambase);
> + &pvt->u.ambase_bottom);
> pci_read_config_dword(pvt->system_address, AMBASE + sizeof(u32),
> - ((u32 *) & pvt->ambase) + sizeof(u32));
> + &pvt->u.ambase_top);
>
> maxdimmperch = pvt->maxdimmperch;
> maxch = pvt->maxch;
> diff --git a/drivers/edac/i5400_edac.c b/drivers/edac/i5400_edac.c
> index 50069c6..2772469 100644
> --- a/drivers/edac/i5400_edac.c
> +++ b/drivers/edac/i5400_edac.c
> @@ -327,7 +327,13 @@ struct i5400_pvt {
> struct pci_dev *branch_1; /* 22.0 */
>
> u16 tolm; /* top of low memory */
> - u64 ambase; /* AMB BAR */
> + union {
> + u64 ambase; /* AMB BAR */
> + struct {
> + u32 ambase_bottom;
> + u32 ambase_top;
> + } u __packed;
> + };
>
> u16 mir0, mir1;
>
> @@ -1055,9 +1061,9 @@ static void i5400_get_mc_regs(struct mem_ctl_info *mci)
> pvt = mci->pvt_info;
>
> pci_read_config_dword(pvt->system_address, AMBASE,
> - (u32 *) &pvt->ambase);
> + &pvt->u.ambase_bottom);
> pci_read_config_dword(pvt->system_address, AMBASE + sizeof(u32),
> - ((u32 *) &pvt->ambase) + sizeof(u32));
> + &pvt->u.ambase_top);
>
> maxdimmperch = pvt->maxdimmperch;
> maxch = pvt->maxch;
>
Em 27-06-2012 06:06, Dan Carpenter escreveu:
> The intent here was to test that the flag was clear but the '!' has
> higher precedence than the '&'. I2C_M_RD is 0x1 so the current code is
> equivalent to "&& (!sgs[i].flags) ..."
>
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> I sent this originally on Wed, 25 Jan 2012 and Emil Goode sent the same
> fix on Thu, May 3, 2012.
>
> diff --git a/drivers/media/dvb/dvb-usb/az6007.c b/drivers/media/dvb/dvb-usb/az6007.c
> index 4008b9c..f6f0cf9 100644
> --- a/drivers/media/dvb/dvb-usb/az6007.c
> +++ b/drivers/media/dvb/dvb-usb/az6007.c
> @@ -711,7 +711,7 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> addr = msgs[i].addr << 1;
> if (((i + 1) < num)
> && (msgs[i].len == 1)
> - && (!msgs[i].flags & I2C_M_RD)
> + && (!(msgs[i].flags & I2C_M_RD))
> && (msgs[i + 1].flags & I2C_M_RD)
> && (msgs[i].addr == msgs[i + 1].addr)) {
> /*
>
Dan,
Your logic is correct, however, I didn't apply this patch because it broke
the driver.
I'll need to re-visit the driver when I have some time, in order to be
able to apply this one, without breaking the driver. I'll likely need to
change some other things on this routine.
(this has a low priority, as the driver is working properly the way it is).
So, I'm keeping your patch at patchwork, while I don't find some time for it.
Regards,
Mauro
-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Wednesday, June 27, 2012 2:00 AM
To: Jing Huang
Cc: Krishna Gudipati; James E.J. Bottomley; [email protected]; [email protected]; [email protected]
Subject: [patch -resend] [SCSI] bfa: dereferencing freed memory in bfad_im_probe()
If bfad_thread_workq(bfad) was not BFA_STATUS_OK then we freed "im"
and then dereferenced it.
I did a little clean up because it seemed nicer to return directly
instead of doing a superfluous goto. I looked at other functions in
this file and it seems like returning directly is standard.
Signed-off-by: Dan Carpenter <[email protected]>
---
This is the third time I have sent this patch. It was previously sent
on Fri, 29 Jul 2011 and Wed, 29 Feb 2012.
diff --git a/drivers/scsi/bfa/bfad_im.c b/drivers/scsi/bfa/bfad_im.c
index 1ac09af..2eebf8d 100644
--- a/drivers/scsi/bfa/bfad_im.c
+++ b/drivers/scsi/bfa/bfad_im.c
@@ -687,25 +687,21 @@ bfa_status_t
bfad_im_probe(struct bfad_s *bfad)
{
struct bfad_im_s *im;
- bfa_status_t rc = BFA_STATUS_OK;
im = kzalloc(sizeof(struct bfad_im_s), GFP_KERNEL);
- if (im == NULL) {
- rc = BFA_STATUS_ENOMEM;
- goto ext;
- }
+ if (im == NULL)
+ return BFA_STATUS_ENOMEM;
bfad->im = im;
im->bfad = bfad;
if (bfad_thread_workq(bfad) != BFA_STATUS_OK) {
kfree(im);
- rc = BFA_STATUS_FAILED;
+ return BFA_STATUS_FAILED;
}
INIT_WORK(&im->aen_im_notify_work, bfad_aen_im_notify_handler);
-ext:
- return rc;
+ return BFA_STATUS_OK;
}
void
-----
Thanks for the patch.
Acked-by: Krishna Gudipati <[email protected]>
-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Wednesday, June 27, 2012 2:00 AM
To: Jing Huang
Cc: Krishna Gudipati; James E.J. Bottomley; [email protected]; [email protected]; [email protected]
Subject: [patch -resend] [SCSI] bfa: off by one in bfa_ioc_mbox_isr()
If mc == BFI_MC_MAX then we're reading past the end of the
mod->mbhdlr[] array.
Signed-off-by: Dan Carpenter <[email protected]>
---
Originally sent on Wed, 6 Jul 2011.
diff --git a/drivers/scsi/bfa/bfa_ioc.c b/drivers/scsi/bfa/bfa_ioc.c
index 14e6284..8cdb79c 100644
--- a/drivers/scsi/bfa/bfa_ioc.c
+++ b/drivers/scsi/bfa/bfa_ioc.c
@@ -2357,7 +2357,7 @@ bfa_ioc_mbox_isr(struct bfa_ioc_s *ioc)
return;
}
- if ((mc > BFI_MC_MAX) || (mod->mbhdlr[mc].cbfn == NULL))
+ if ((mc >= BFI_MC_MAX) || (mod->mbhdlr[mc].cbfn == NULL))
return;
mod->mbhdlr[mc].cbfn(mod->mbhdlr[mc].cbarg, &m);
-----
Thanks for the patch.
Acked-by: Krishna Gudipati <[email protected]>
On Wed, Jun 27, 2012 at 2:05 AM, Dan Carpenter <[email protected]> wrote:
> These are __iomem. ?Sparse complains if we don't have that.
>
> drivers/scsi/isci/phy.c +149 70: warning:
> ? ? ? ?incorrect type in initializer (different address spaces)
>
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> Sent on Wed, 18 Jan 2012.
>
Thanks for nudge, this fell off the radar. I'll get this into -next
with the rest of the pending bits.
From: Dan Carpenter <[email protected]>
Date: Wed, 27 Jun 2012 12:01:41 +0300
> I don't think we're actually likely to hit this limit but if we do
> then the comparison should be done as size_t. The original code
> is equivalent to:
> len = strlen(sptr) % USHRT_MAX;
>
> Signed-off-by: Dan Carpenter <[email protected]>
Applied, thanks Dan.
On Wed, Jun 27, 2012 at 2:08 AM, Dan Carpenter <[email protected]> wrote:
> On 64 bit systems the current code sets 32 bits of "seg" and leaves the
> other 32 uninitialized. ?It doesn't matter since the variable is never
> used. ?But it's still messy and we should fix it.
>
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> Originally sent on Fri, 2 Mar 2012.
>
> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
> index 4d39a9f..97825f1 100644
> --- a/drivers/scsi/megaraid.c
> +++ b/drivers/scsi/megaraid.c
> @@ -524,7 +524,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
> ? ? ? ?mega_passthru ? *pthru;
> ? ? ? ?scb_t ? *scb;
> ? ? ? ?mbox_t ?*mbox;
> - ? ? ? long ? ?seg;
> + ? ? ? u32 ? ? seg;
> ? ? ? ?char ? ?islogical;
> ? ? ? ?int ? ? max_ldrv_num;
> ? ? ? ?int ? ? channel = 0;
> @@ -858,7 +858,7 @@ mega_build_cmd(adapter_t *adapter, Scsi_Cmnd *cmd, int *busy)
>
> ? ? ? ? ? ? ? ? ? ? ? ?/* Calculate Scatter-Gather info */
> ? ? ? ? ? ? ? ? ? ? ? ?mbox->m_out.numsgelements = mega_build_sglist(adapter, scb,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (u32 *)&mbox->m_out.xferaddr, (u32 *)&seg);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (u32 *)&mbox->m_out.xferaddr, &seg);
>
> ? ? ? ? ? ? ? ? ? ? ? ?return scb;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
Acked-by: Adam Radford <[email protected]>
On Wed, Jun 27, 2012 at 2:00 AM, Dan Carpenter <[email protected]> wrote:
> We took this lock with spin_lock() so we should unlock it with
> spin_unlock() instead of spin_unlock_irq(). ?This was introduced in
> f2c8dc402b "[SCSI] megaraid_mbox: remove scsi_assign_lock usage".
>
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> This was originally sent on Sat, 30 Jul 2011. ?I have cleaned up the
> commit message a bit and added Christoph to the CC list.
>
> diff --git a/drivers/scsi/megaraid/megaraid_mbox.c b/drivers/scsi/megaraid/megaraid_mbox.c
> index 35bd138..54b1c5b 100644
> --- a/drivers/scsi/megaraid/megaraid_mbox.c
> +++ b/drivers/scsi/megaraid/megaraid_mbox.c
> @@ -2731,7 +2731,7 @@ megaraid_reset_handler(struct scsi_cmnd *scp)
> ? ? ? ?}
>
> ?out:
> - ? ? ? spin_unlock_irq(&adapter->lock);
> + ? ? ? spin_unlock(&adapter->lock);
> ? ? ? ?return rval;
> ?}
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
Acked-by: Adam Radford <[email protected]>
On Wed, Jun 27, 2012 at 10:11:00AM -0300, Mauro Carvalho Chehab wrote:
> Em 27-06-2012 06:06, Dan Carpenter escreveu:
> > The intent here was to test that the flag was clear but the '!' has
> > higher precedence than the '&'. I2C_M_RD is 0x1 so the current code is
> > equivalent to "&& (!sgs[i].flags) ..."
> >
> > Signed-off-by: Dan Carpenter <[email protected]>
> > ---
> > I sent this originally on Wed, 25 Jan 2012 and Emil Goode sent the same
> > fix on Thu, May 3, 2012.
> >
> > diff --git a/drivers/media/dvb/dvb-usb/az6007.c b/drivers/media/dvb/dvb-usb/az6007.c
> > index 4008b9c..f6f0cf9 100644
> > --- a/drivers/media/dvb/dvb-usb/az6007.c
> > +++ b/drivers/media/dvb/dvb-usb/az6007.c
> > @@ -711,7 +711,7 @@ static int az6007_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
> > addr = msgs[i].addr << 1;
> > if (((i + 1) < num)
> > && (msgs[i].len == 1)
> > - && (!msgs[i].flags & I2C_M_RD)
> > + && (!(msgs[i].flags & I2C_M_RD))
> > && (msgs[i + 1].flags & I2C_M_RD)
> > && (msgs[i].addr == msgs[i + 1].addr)) {
> > /*
> >
>
> Dan,
>
> Your logic is correct, however, I didn't apply this patch because it broke
> the driver.
>
> I'll need to re-visit the driver when I have some time, in order to be
> able to apply this one, without breaking the driver. I'll likely need to
> change some other things on this routine.
>
> (this has a low priority, as the driver is working properly the way it is).
>
> So, I'm keeping your patch at patchwork, while I don't find some time for it.
We could just put a comment next to the code and forget about it.
&& (!(msgs[i].flags & I2C_M_RD)) /* the fix needs testing. */
Sparse complains about this so it people are going to keep sending
patches for it. It's not like you should be stuck doing all the
work.
regards,
dan carpenter
> >
> > Just be curious, what's kind of the tool here?
> >
>
> Sorry I should have CC'd Matt on this. I think it's something he
> is working on at the University of Wisconsin. That's all I know.
>
> regards,
> dan carpenter
>
> > -Bryan
Dan is correct. We've submitted the project for publication but whether it will
be accepted is another question. If it's published I'd be happy to provide a
reference.
In a nutshell, the tool we've developed uses symbolic execution to reduce the
need for device hardware for driver testing and execute real Linux drivers in
the kernel. It provides symbolic data in place of hardware input. The tool
allows us to run real drivers even if the hardware is unavailable. It can't
find all bugs, but it can find many that could manifest in real conditions, e.g.
null pointer dereferences, incorrect kernel API usage, some "panics" and "BUG"
calls, etc.
I'll try to remember to write back when it's published, at which point I'd be
happy to provide more detail.
Thanks for the interest,
Matt