[PATCH] fnic: check pci_map_single() return value
the kernel prints some warnings when compiled with CONFIG_DMA_API_DEBUG.
This is because the fnic driver doesn't check the return value of
pci_map_single().
[ 11.942770] scsi host12: fnic
[ 11.950811] [ cut here ]
[ 11.950818] WARNING: at lib/dma-debug.c:937 check_unmap+0x47b/0x920()
[ 11.950821] fnic :0c:00.0: DMA-API: device driver failed to check map
error[device address=0x002020a30040] [size=44 bytes] [mapped as single]
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/fnic/fnic_fcs.c | 32 +---
drivers/scsi/fnic/fnic_scsi.c | 16
2 files changed, 45 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/fnic/fnic_fcs.c b/drivers/scsi/fnic/fnic_fcs.c
index bf0bbd4..4adf9c3 100644
--- a/drivers/scsi/fnic/fnic_fcs.c
+++ b/drivers/scsi/fnic/fnic_fcs.c
@@ -939,6 +939,7 @@ int fnic_alloc_rq_frame(struct vnic_rq *rq)
struct sk_buff *skb;
u16 len;
dma_addr_t pa;
+ int r;
len = FC_FRAME_HEADROOM + FC_MAX_FRAME + FC_FRAME_TAILROOM;
skb = dev_alloc_skb(len);
@@ -952,8 +953,17 @@ int fnic_alloc_rq_frame(struct vnic_rq *rq)
skb_reset_network_header(skb);
skb_put(skb, len);
pa = pci_map_single(fnic->pdev, skb->data, len, PCI_DMA_FROMDEVICE);
+
+ r = pci_dma_mapping_error(fnic->pdev, pa);
+ if (r) {
+ printk(KERN_ERR "PCI mapping failed with error %d\n", r);
+ kfree_skb(skb);
+ goto err;
+ }
+
fnic_queue_rq_desc(rq, skb, pa, len);
- return 0;
+err:
+ return r;
}
void fnic_free_rq_buf(struct vnic_rq *rq, struct vnic_rq_buf *buf)
@@ -981,6 +991,7 @@ void fnic_eth_send(struct fcoe_ctlr *fip, struct sk_buff
*skb)
struct ethhdr *eth_hdr;
struct vlan_ethhdr *vlan_hdr;
unsigned long flags;
+ int r;
if (!fnic->vlan_hw_insert) {
eth_hdr = (struct ethhdr *)skb_mac_header(skb);
@@ -1003,6 +1014,13 @@ void fnic_eth_send(struct fcoe_ctlr *fip, struct sk_buff
*skb)
pa = pci_map_single(fnic->pdev, skb->data, skb->len, PCI_DMA_TODEVICE);
+ r = pci_dma_mapping_error(fnic->pdev, pa);
+ if (r) {
+ printk(KERN_ERR "PCI mapping failed with error %d\n", r);
+ kfree_skb(skb);
+ return;
+ }
+
spin_lock_irqsave(&fnic->wq_lock[0], flags);
if (!vnic_wq_desc_avail(wq)) {
pci_unmap_single(fnic->pdev, pa, skb->len, PCI_DMA_TODEVICE);
@@ -1071,6 +1089,12 @@ static int fnic_send_frame(struct fnic *fnic, struct
fc_frame *fp)
pa = pci_map_single(fnic->pdev, eth_hdr, tot_len, PCI_DMA_TODEVICE);
+ ret = pci_dma_mapping_error(fnic->pdev, pa);
+ if (ret) {
+ printk(KERN_ERR "DMA map failed with error %d\n", ret);
+ goto err_mapping_error;
+ }
+
if ((fnic_fc_trace_set_data(fnic->lport->host->host_no, FNIC_FC_SEND,
(char *)eth_hdr, tot_len)) != 0) {
printk(KERN_ERR "fnic ctlr frame trace error!!!");
@@ -1082,15 +1106,17 @@ static int fnic_send_frame(struct fnic *fnic, struct
fc_frame *fp)
pci_unmap_single(fnic->pdev, pa,
tot_len, PCI_DMA_TODEVICE);
ret = -1;
- goto fnic_send_frame_end;
+ goto err_desc_avail;
}
fnic_queue_wq_desc(wq, skb, pa, tot_len, fr_eof(fp),
0 /* hw inserts cos value */,
fnic->vlan_id, 1, 1, 1);
-fnic_send_frame_end:
+
+err_desc_avail:
spin_unlock_irqrestore(&fnic->wq_lock[0], flags);
+err_mapping_error:
if (ret)
dev_kfree_skb_any(fp_skb(fp));
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index 155b286..2949d17 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -330,6 +330,7 @@ static inline int fnic_queue_wq_copy_desc(struct fnic *fnic,
int flags;
u8 exch_flags;
struct scsi_lun fc_lun;
+ int r;
if (sg_count) {
/* For each SGE, create a device desc entry */
@@ -346,6 +347,12 @@ static inline int fnic_queue_wq_copy_desc(struct fnic
*fnic,
io_req->sgl_list,
sizeof(io_req->sgl_list[0]) * sg_count,
PCI_DMA_TODEVICE);
+
+ r = pci_dma_mapping_error(fnic->pdev, io_req->sgl_list_pa);
+ if (r) {
+ printk(KERN_ERR "PCI mapping failed with error %d\n",
r);
+ return SCSI_MLQUEUE_HOST_BUSY;
+ }
}
io_req->sense_buf_pa = pci_map_single(fnic->pdev,
@@ -353,6 +3
Re: [PATCH] fnic: check pci_map_single() return value
Hi Johannes, On 08/12/2015 01:42 PM, Johannes Thumshirn wrote: > hmmm, how about > > goto free_skb; >> +} >> + >> fnic_queue_rq_desc(rq, skb, pa, len); >> -return 0; >> +err: > > free_skb: > kfree_skb(skb); > >> +return r; >> } > > Although the return is near to the error handling, it's preferred to > have the labels named after the action that is taken there. Ok, I'll post a V2 of this patch soon, thanks for the review. Regards, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V2] fnic: check pci_map_single() return value
the kernel prints some warnings when compiled with CONFIG_DMA_API_DEBUG.
This is because the fnic driver doesn't check the return value of
pci_map_single().
[ 11.942770] scsi host12: fnic
[ 11.950811] [ cut here ]
[ 11.950818] WARNING: at lib/dma-debug.c:937 check_unmap+0x47b/0x920()
[ 11.950821] fnic :0c:00.0: DMA-API: device driver failed to check map
error[device address=0x002020a30040] [size=44 bytes] [mapped as single]
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/fnic/fnic_fcs.c | 32 ++--
drivers/scsi/fnic/fnic_scsi.c | 16
2 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/fnic/fnic_fcs.c b/drivers/scsi/fnic/fnic_fcs.c
index bf0bbd4..be7b3da 100644
--- a/drivers/scsi/fnic/fnic_fcs.c
+++ b/drivers/scsi/fnic/fnic_fcs.c
@@ -939,6 +939,7 @@ int fnic_alloc_rq_frame(struct vnic_rq *rq)
struct sk_buff *skb;
u16 len;
dma_addr_t pa;
+ int r;
len = FC_FRAME_HEADROOM + FC_MAX_FRAME + FC_FRAME_TAILROOM;
skb = dev_alloc_skb(len);
@@ -952,8 +953,19 @@ int fnic_alloc_rq_frame(struct vnic_rq *rq)
skb_reset_network_header(skb);
skb_put(skb, len);
pa = pci_map_single(fnic->pdev, skb->data, len, PCI_DMA_FROMDEVICE);
+
+ r = pci_dma_mapping_error(fnic->pdev, pa);
+ if (r) {
+ printk(KERN_ERR "PCI mapping failed with error %d\n", r);
+ goto free_skb;
+ }
+
fnic_queue_rq_desc(rq, skb, pa, len);
return 0;
+
+free_skb:
+ kfree_skb(skb);
+ return r;
}
void fnic_free_rq_buf(struct vnic_rq *rq, struct vnic_rq_buf *buf)
@@ -981,6 +993,7 @@ void fnic_eth_send(struct fcoe_ctlr *fip, struct sk_buff
*skb)
struct ethhdr *eth_hdr;
struct vlan_ethhdr *vlan_hdr;
unsigned long flags;
+ int r;
if (!fnic->vlan_hw_insert) {
eth_hdr = (struct ethhdr *)skb_mac_header(skb);
@@ -1003,6 +1016,13 @@ void fnic_eth_send(struct fcoe_ctlr *fip, struct sk_buff
*skb)
pa = pci_map_single(fnic->pdev, skb->data, skb->len, PCI_DMA_TODEVICE);
+ r = pci_dma_mapping_error(fnic->pdev, pa);
+ if (r) {
+ printk(KERN_ERR "PCI mapping failed with error %d\n", r);
+ kfree_skb(skb);
+ return;
+ }
+
spin_lock_irqsave(&fnic->wq_lock[0], flags);
if (!vnic_wq_desc_avail(wq)) {
pci_unmap_single(fnic->pdev, pa, skb->len, PCI_DMA_TODEVICE);
@@ -1071,6 +1091,12 @@ static int fnic_send_frame(struct fnic *fnic, struct
fc_frame *fp)
pa = pci_map_single(fnic->pdev, eth_hdr, tot_len, PCI_DMA_TODEVICE);
+ ret = pci_dma_mapping_error(fnic->pdev, pa);
+ if (ret) {
+ printk(KERN_ERR "DMA map failed with error %d\n", ret);
+ goto free_skb_on_err;
+ }
+
if ((fnic_fc_trace_set_data(fnic->lport->host->host_no, FNIC_FC_SEND,
(char *)eth_hdr, tot_len)) != 0) {
printk(KERN_ERR "fnic ctlr frame trace error!!!");
@@ -1082,15 +1108,17 @@ static int fnic_send_frame(struct fnic *fnic, struct
fc_frame *fp)
pci_unmap_single(fnic->pdev, pa,
tot_len, PCI_DMA_TODEVICE);
ret = -1;
- goto fnic_send_frame_end;
+ goto irq_restore;
}
fnic_queue_wq_desc(wq, skb, pa, tot_len, fr_eof(fp),
0 /* hw inserts cos value */,
fnic->vlan_id, 1, 1, 1);
-fnic_send_frame_end:
+
+irq_restore:
spin_unlock_irqrestore(&fnic->wq_lock[0], flags);
+free_skb_on_err:
if (ret)
dev_kfree_skb_any(fp_skb(fp));
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index 155b286..2949d17 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -330,6 +330,7 @@ static inline int fnic_queue_wq_copy_desc(struct fnic *fnic,
int flags;
u8 exch_flags;
struct scsi_lun fc_lun;
+ int r;
if (sg_count) {
/* For each SGE, create a device desc entry */
@@ -346,6 +347,12 @@ static inline int fnic_queue_wq_copy_desc(struct fnic
*fnic,
io_req->sgl_list,
sizeof(io_req->sgl_list[0]) * sg_count,
PCI_DMA_TODEVICE);
+
+ r = pci_dma_mapping_error(fnic->pdev, io_req->sgl_list_pa);
+ if (r) {
+ printk(KERN_ERR "PCI mapping failed with error %d\n",
r);
+ return SCSI_MLQUEUE_HOST_BUSY;
+ }
}
io_req->sense_buf_pa = pci_map_single(fnic->pdev,
@@ -353,6 +3
Re: [PATCH V2] fnic: check pci_map_single() return value
Hi,
On 08/12/2015 03:33 PM, Johannes Thumshirn wrote:
> Hi Maurizio,
>
> Sorry but it looks like you've forgotten one change in fnic_eth_send().
Ok, I converted fnic_eth_send() to use gotos for error cleanup and sent V3.
Hopefully it'll be the final version.
Thanks for the review.
Regards,
Maurizio Lombardi
>
> Maurizio Lombardi writes:
>
>> the kernel prints some warnings when compiled with CONFIG_DMA_API_DEBUG.
>> This is because the fnic driver doesn't check the return value of
>> pci_map_single().
>>
>> [ 11.942770] scsi host12: fnic
>> }
>
> [..]
>>
>> void fnic_free_rq_buf(struct vnic_rq *rq, struct vnic_rq_buf *buf)
>> @@ -981,6 +993,7 @@ void fnic_eth_send(struct fcoe_ctlr *fip, struct sk_buff
>> *skb)
>> struct ethhdr *eth_hdr;
>> struct vlan_ethhdr *vlan_hdr;
>> unsigned long flags;
>> +int r;
>>
>> if (!fnic->vlan_hw_insert) {
>> eth_hdr = (struct ethhdr *)skb_mac_header(skb);
>> @@ -1003,6 +1016,13 @@ void fnic_eth_send(struct fcoe_ctlr *fip, struct
>> sk_buff *skb)
>>
>> pa = pci_map_single(fnic->pdev, skb->data, skb->len, PCI_DMA_TODEVICE);
>>
>> +r = pci_dma_mapping_error(fnic->pdev, pa);
>> +if (r) {
>> +printk(KERN_ERR "PCI mapping failed with error %d\n", r);
>> +kfree_skb(skb);
>> +return;
>> +}
>> +
>> spin_lock_irqsave(&fnic->wq_lock[0], flags);
>> if (!vnic_wq_desc_avail(wq)) {
>> pci_unmap_single(fnic->pdev, pa, skb->len, PCI_DMA_TODEVICE);
>> @@ -1071,6 +1091,12 @@ static int fnic_send_frame(struct fnic *fnic, struct
>> fc_frame *fp)
>>
>> pa = pci_map_single(fnic->pdev, eth_hdr, tot_len, PCI_DMA_TODEVICE);
> [..]
>
> Otherwise:
>
> Reviewed-by: Johannes Thumshirn
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3] fnic: check pci_map_single() return value
the kernel prints some warnings when compiled with CONFIG_DMA_API_DEBUG.
This is because the fnic driver doesn't check the return value of
pci_map_single().
[ 11.942770] scsi host12: fnic
[ 11.950811] [ cut here ]
[ 11.950818] WARNING: at lib/dma-debug.c:937 check_unmap+0x47b/0x920()
[ 11.950821] fnic :0c:00.0: DMA-API: device driver failed to check map
error[device address=0x002020a30040] [size=44 bytes] [mapped as single]
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/fnic/fnic_fcs.c | 46 +++
drivers/scsi/fnic/fnic_scsi.c | 16 +++
2 files changed, 54 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/fnic/fnic_fcs.c b/drivers/scsi/fnic/fnic_fcs.c
index bf0bbd4..67669a9 100644
--- a/drivers/scsi/fnic/fnic_fcs.c
+++ b/drivers/scsi/fnic/fnic_fcs.c
@@ -939,6 +939,7 @@ int fnic_alloc_rq_frame(struct vnic_rq *rq)
struct sk_buff *skb;
u16 len;
dma_addr_t pa;
+ int r;
len = FC_FRAME_HEADROOM + FC_MAX_FRAME + FC_FRAME_TAILROOM;
skb = dev_alloc_skb(len);
@@ -952,8 +953,19 @@ int fnic_alloc_rq_frame(struct vnic_rq *rq)
skb_reset_network_header(skb);
skb_put(skb, len);
pa = pci_map_single(fnic->pdev, skb->data, len, PCI_DMA_FROMDEVICE);
+
+ r = pci_dma_mapping_error(fnic->pdev, pa);
+ if (r) {
+ printk(KERN_ERR "PCI mapping failed with error %d\n", r);
+ goto free_skb;
+ }
+
fnic_queue_rq_desc(rq, skb, pa, len);
return 0;
+
+free_skb:
+ kfree_skb(skb);
+ return r;
}
void fnic_free_rq_buf(struct vnic_rq *rq, struct vnic_rq_buf *buf)
@@ -981,6 +993,7 @@ void fnic_eth_send(struct fcoe_ctlr *fip, struct sk_buff
*skb)
struct ethhdr *eth_hdr;
struct vlan_ethhdr *vlan_hdr;
unsigned long flags;
+ int r;
if (!fnic->vlan_hw_insert) {
eth_hdr = (struct ethhdr *)skb_mac_header(skb);
@@ -1003,18 +1016,27 @@ void fnic_eth_send(struct fcoe_ctlr *fip, struct
sk_buff *skb)
pa = pci_map_single(fnic->pdev, skb->data, skb->len, PCI_DMA_TODEVICE);
- spin_lock_irqsave(&fnic->wq_lock[0], flags);
- if (!vnic_wq_desc_avail(wq)) {
- pci_unmap_single(fnic->pdev, pa, skb->len, PCI_DMA_TODEVICE);
- spin_unlock_irqrestore(&fnic->wq_lock[0], flags);
- kfree_skb(skb);
- return;
+ r = pci_dma_mapping_error(fnic->pdev, pa);
+ if (r) {
+ printk(KERN_ERR "PCI mapping failed with error %d\n", r);
+ goto free_skb;
}
+ spin_lock_irqsave(&fnic->wq_lock[0], flags);
+ if (!vnic_wq_desc_avail(wq))
+ goto irq_restore;
+
fnic_queue_wq_eth_desc(wq, skb, pa, skb->len,
0 /* hw inserts cos value */,
fnic->vlan_id, 1);
spin_unlock_irqrestore(&fnic->wq_lock[0], flags);
+ return;
+
+irq_restore:
+ spin_unlock_irqrestore(&fnic->wq_lock[0], flags);
+ pci_unmap_single(fnic->pdev, pa, skb->len, PCI_DMA_TODEVICE);
+free_skb:
+ kfree_skb(skb);
}
/*
@@ -1071,6 +1093,12 @@ static int fnic_send_frame(struct fnic *fnic, struct
fc_frame *fp)
pa = pci_map_single(fnic->pdev, eth_hdr, tot_len, PCI_DMA_TODEVICE);
+ ret = pci_dma_mapping_error(fnic->pdev, pa);
+ if (ret) {
+ printk(KERN_ERR "DMA map failed with error %d\n", ret);
+ goto free_skb_on_err;
+ }
+
if ((fnic_fc_trace_set_data(fnic->lport->host->host_no, FNIC_FC_SEND,
(char *)eth_hdr, tot_len)) != 0) {
printk(KERN_ERR "fnic ctlr frame trace error!!!");
@@ -1082,15 +1110,17 @@ static int fnic_send_frame(struct fnic *fnic, struct
fc_frame *fp)
pci_unmap_single(fnic->pdev, pa,
tot_len, PCI_DMA_TODEVICE);
ret = -1;
- goto fnic_send_frame_end;
+ goto irq_restore;
}
fnic_queue_wq_desc(wq, skb, pa, tot_len, fr_eof(fp),
0 /* hw inserts cos value */,
fnic->vlan_id, 1, 1, 1);
-fnic_send_frame_end:
+
+irq_restore:
spin_unlock_irqrestore(&fnic->wq_lock[0], flags);
+free_skb_on_err:
if (ret)
dev_kfree_skb_any(fp_skb(fp));
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index 155b286..2949d17 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -330,6 +330,7 @@ static inline int fnic_queue_wq_copy_desc(struct fnic *fnic,
int flags;
u8 exch_flags;
struct scsi_lun fc_lun;
+ int r;
if (sg_cou
[PATCH RFC] enclosure: fix symlinks creation
James, one of our customers complains that in some cases, when using multipath, the enclosure driver fails to create the symlinks in sysfs. This is an example of what happens: [ 19.251902] scsi 0:0:27:0: Direct-Access SEAGATE ST8000NM0075 E002 PQ: 0 ANSI: 6 [ 19.261874] scsi 0:0:27:0: SSP: handle(0x0027), sas_addr(0x5000c50085826dd6), phy(34), device_name(0x5000c50085826dd4) [ 19.274656] scsi 0:0:27:0: SSP: enclosure_logical_id(0x500093d001be4000), slot(57) [ 19.283944] scsi 0:0:27:0: SSP: enclosure level(0x), connector name( ) [ 19.292933] scsi 0:0:27:0: qdepth(254), tagged(1), simple(0), ordered(0), scsi_l [...] [ 60.066524] enclosure_add_device(0:0:27:0) called, enclosure_add_links() failed with error -2 [...] [ 75.119146] sd 0:0:27:0: Attached scsi generic sg27 type 0 [ 75.126722] sd 0:0:27:0: [sdaa] 15628053168 512-byte logical blocks: (8.00 TB/7.27 TiB) [ 75.126723] sd 0:0:27:0: [sdaa] 4096-byte physical blocks [ 75.129059] sd 0:0:27:0: [sdaa] Write Protect is off [ 75.129061] sd 0:0:27:0: [sdaa] Mode Sense: db 00 10 08 [ 75.130417] sd 0:0:27:0: [sdaa] Write cache: enabled, read cache: enabled, supports DPO and FUA [ 75.158088] sd 0:0:27:0: [sdaa] Attached SCSI disk [...] [ 75.192722] enclosure_add_device(0:0:27:0) called, device already exists The first time enclosure_add_device() is called, the symlinks creation failed. The second time it would have succeeded, but the enclosure_add_device() function returned immediately without retrying to create the symlinks. Maurizio Lombardi (1): enclosure: fix sysfs symlinks creation when using multipath drivers/misc/enclosure.c | 16 ++-- include/linux/enclosure.h | 1 + 2 files changed, 15 insertions(+), 2 deletions(-) -- Maurizio Lombardi
[PATCH RFC] enclosure: fix sysfs symlinks creation when using multipath
With multipath, it may happen that the same device is passed
to enclosure_add_device() multiple times and that the enclosure_add_links()
function fails to create the symlinks because the device's sysfs
directory entry is still NULL.
In this case, the links will never be created because all the subsequent
calls to enclosure_add_device() will immediately fail with EEXIST.
This patch modifies the code so the driver will detect this condition
and will retry to create the symlinks when enclosure_add_device() is called.
Signed-off-by: Maurizio Lombardi
---
drivers/misc/enclosure.c | 16 ++--
include/linux/enclosure.h | 1 +
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 65fed71..a856c98 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,21 +375,33 @@ int enclosure_add_device(struct enclosure_device *edev,
int component,
struct device *dev)
{
struct enclosure_component *cdev;
+ int error;
if (!edev || component >= edev->components)
return -EINVAL;
cdev = &edev->component[component];
- if (cdev->dev == dev)
+ if (cdev->dev == dev) {
+ if (!cdev->links_created) {
+ error = enclosure_add_links(cdev);
+ if (!error)
+ cdev->links_created = 1;
+ }
return -EEXIST;
+ }
if (cdev->dev)
enclosure_remove_links(cdev);
put_device(cdev->dev);
cdev->dev = get_device(dev);
- return enclosure_add_links(cdev);
+ error = enclosure_add_links(cdev);
+ if (!error)
+ cdev->links_created = 1;
+ else
+ cdev->links_created = 0;
+ return error;
}
EXPORT_SYMBOL_GPL(enclosure_add_device);
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index a4cf57c..c3bdc4c 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -97,6 +97,7 @@ struct enclosure_component {
struct device cdev;
struct device *dev;
enum enclosure_component_type type;
+ int links_created;
int number;
int fault;
int active;
--
Maurizio Lombardi
Re: [PATCH RFC] enclosure: fix sysfs symlinks creation when using multipath
Hi James,
have you noticed this patch?
Dne 7.2.2017 v 15:08 Maurizio Lombardi napsal(a):
> With multipath, it may happen that the same device is passed
> to enclosure_add_device() multiple times and that the enclosure_add_links()
> function fails to create the symlinks because the device's sysfs
> directory entry is still NULL.
> In this case, the links will never be created because all the subsequent
> calls to enclosure_add_device() will immediately fail with EEXIST.
>
> This patch modifies the code so the driver will detect this condition
> and will retry to create the symlinks when enclosure_add_device() is called.
>
> Signed-off-by: Maurizio Lombardi
> ---
> drivers/misc/enclosure.c | 16 ++--
> include/linux/enclosure.h | 1 +
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> index 65fed71..a856c98 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -375,21 +375,33 @@ int enclosure_add_device(struct enclosure_device *edev,
> int component,
>struct device *dev)
> {
> struct enclosure_component *cdev;
> + int error;
>
> if (!edev || component >= edev->components)
> return -EINVAL;
>
> cdev = &edev->component[component];
>
> - if (cdev->dev == dev)
> + if (cdev->dev == dev) {
> + if (!cdev->links_created) {
> + error = enclosure_add_links(cdev);
> + if (!error)
> + cdev->links_created = 1;
> + }
> return -EEXIST;
> + }
>
> if (cdev->dev)
> enclosure_remove_links(cdev);
>
> put_device(cdev->dev);
> cdev->dev = get_device(dev);
> - return enclosure_add_links(cdev);
> + error = enclosure_add_links(cdev);
> + if (!error)
> + cdev->links_created = 1;
> + else
> + cdev->links_created = 0;
> + return error;
> }
> EXPORT_SYMBOL_GPL(enclosure_add_device);
>
> diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
> index a4cf57c..c3bdc4c 100644
> --- a/include/linux/enclosure.h
> +++ b/include/linux/enclosure.h
> @@ -97,6 +97,7 @@ struct enclosure_component {
> struct device cdev;
> struct device *dev;
> enum enclosure_component_type type;
> + int links_created;
> int number;
> int fault;
> int active;
>
[PATCH] enclosure: fix sysfs symlinks creation when using multipath
With multipath, it may happen that the same device is passed
to enclosure_add_device() multiple times and that the enclosure_add_links()
function fails to create the symlinks because the device's sysfs
directory entry is still NULL.
In this case, the links will never be created because all the subsequent
calls to enclosure_add_device() will immediately fail with EEXIST.
This is an example of what happens:
[ 19.251902] scsi 0:0:27:0: Direct-Access SEAGATE ST8000NM0075 E002
PQ: 0 ANSI: 6
[ 19.261874] scsi 0:0:27:0: SSP: handle(0x0027),
sas_addr(0x5000c50085826dd6), phy(34), device_name(0x5000c50085826dd4)
[ 19.274656] scsi 0:0:27:0: SSP: enclosure_logical_id(0x500093d001be4000),
slot(57)
[ 19.283944] scsi 0:0:27:0: SSP: enclosure level(0x), connector name(
)
[ 19.292933] scsi 0:0:27:0: qdepth(254), tagged(1), simple(0), ordered(0),
scsi_l
[...]
[ 60.066524] enclosure_add_device(0:0:27:0) called, enclosure_add_links()
failed with error -2
[...]
[ 75.119146] sd 0:0:27:0: Attached scsi generic sg27 type 0
[ 75.126722] sd 0:0:27:0: [sdaa] 15628053168 512-byte logical blocks: (8.00
TB/7.27 TiB)
[ 75.126723] sd 0:0:27:0: [sdaa] 4096-byte physical blocks
[ 75.129059] sd 0:0:27:0: [sdaa] Write Protect is off
[ 75.129061] sd 0:0:27:0: [sdaa] Mode Sense: db 00 10 08
[ 75.130417] sd 0:0:27:0: [sdaa] Write cache: enabled, read cache: enabled,
supports DPO and FUA
[ 75.158088] sd 0:0:27:0: [sdaa] Attached SCSI disk
[...]
[ 75.192722] enclosure_add_device(0:0:27:0) called, device already exists
This patch modifies the code so the driver will detect this condition
and will retry to create the symlinks when enclosure_add_device() is called.
Signed-off-by: Maurizio Lombardi
---
drivers/misc/enclosure.c | 16 ++--
include/linux/enclosure.h | 1 +
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index 65fed71..a856c98 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -375,21 +375,33 @@ int enclosure_add_device(struct enclosure_device *edev,
int component,
struct device *dev)
{
struct enclosure_component *cdev;
+ int error;
if (!edev || component >= edev->components)
return -EINVAL;
cdev = &edev->component[component];
- if (cdev->dev == dev)
+ if (cdev->dev == dev) {
+ if (!cdev->links_created) {
+ error = enclosure_add_links(cdev);
+ if (!error)
+ cdev->links_created = 1;
+ }
return -EEXIST;
+ }
if (cdev->dev)
enclosure_remove_links(cdev);
put_device(cdev->dev);
cdev->dev = get_device(dev);
- return enclosure_add_links(cdev);
+ error = enclosure_add_links(cdev);
+ if (!error)
+ cdev->links_created = 1;
+ else
+ cdev->links_created = 0;
+ return error;
}
EXPORT_SYMBOL_GPL(enclosure_add_device);
diff --git a/include/linux/enclosure.h b/include/linux/enclosure.h
index a4cf57c..c3bdc4c 100644
--- a/include/linux/enclosure.h
+++ b/include/linux/enclosure.h
@@ -97,6 +97,7 @@ struct enclosure_component {
struct device cdev;
struct device *dev;
enum enclosure_component_type type;
+ int links_created;
int number;
int fault;
int active;
--
Maurizio Lombardi
[PATCH] bnx2fc: do not add shared skbs to the fcoe_rx_list
In some cases, the fcoe_rx_list may contains multiple instances
of the same skb (the so called "shared skbs").
the bnx2fc_l2_rcv thread is a loop that extracts a skb from the list,
modifies (and destroys) its content and the proceed to the next one.
The problem is that if the skb is shared, the remaining instances will
be corrupted.
The solution is to use skb_share_check() before adding the skb to the
fcoe_rx_list.
[ 6286.808725] [ cut here ]
[ 6286.808729] WARNING: at include/scsi/fc_frame.h:173
bnx2fc_l2_rcv_thread+0x425/0x450 [bnx2fc]()
[ 6286.808748] Modules linked in: bnx2x(-) mdio dm_service_time bnx2fc cnic uio
fcoe libfcoe 8021q garp stp mrp libfc llc scsi_transport_fc scsi_tgt sg
iTCO_wdt iTCO_vendor_support coretemp kvm_intel kvm crct10dif_pclmul
crc32_pclmul crc32c_intel e1000e ghash_clmulni_intel aesni_intel lrw gf128mul
glue_helper ablk_helper ptp cryptd hpilo serio_raw hpwdt lpc_ich pps_core
ipmi_si pcspkr mfd_core ipmi_msghandler shpchp pcc_cpufreq mperf nfsd
auth_rpcgss nfs_acl lockd sunrpc dm_multipath xfs libcrc32c ata_generic
pata_acpi sd_mod crc_t10dif crct10dif_common mgag200 syscopyarea sysfillrect
sysimgblt i2c_algo_bit ata_piix drm_kms_helper ttm drm libata i2c_core hpsa
dm_mirror dm_region_hash dm_log dm_mod [last unloaded: mdio]
[ 6286.808750] CPU: 3 PID: 1304 Comm: bnx2fc_l2_threa Not tainted
3.10.0-121.el7.x86_64 #1
[ 6286.808750] Hardware name: HP ProLiant DL120 G7, BIOS J01 07/01/2013
[ 6286.808752] 0b36e715 8800deba1e00
815ec0ba
[ 6286.808753] 8800deba1e38 8105dee1 a05618c0
8801e4c81888
[ 6286.808754] e8663868 8801f402b180 8801f56bc000
8800deba1e48
[ 6286.808754] Call Trace:
[ 6286.808759] [] dump_stack+0x19/0x1b
[ 6286.808762] [] warn_slowpath_common+0x61/0x80
[ 6286.808763] [] warn_slowpath_null+0x1a/0x20
[ 6286.808765] [] bnx2fc_l2_rcv_thread+0x425/0x450 [bnx2fc]
[ 6286.808767] [] ? bnx2fc_disable+0x90/0x90 [bnx2fc]
[ 6286.808769] [] kthread+0xcf/0xe0
[ 6286.808770] [] ? kthread_create_on_node+0x140/0x140
[ 6286.808772] [] ret_from_fork+0x7c/0xb0
[ 6286.808773] [] ? kthread_create_on_node+0x140/0x140
[ 6286.808774] ---[ end trace c6cdb939184ccb4e ]---
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 785d0d7..a190ab6 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -411,6 +411,7 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct
net_device *dev,
struct fc_frame_header *fh;
struct fcoe_rcv_info *fr;
struct fcoe_percpu_s *bg;
+ struct sk_buff *tmp_skb;
unsigned short oxid;
interface = container_of(ptype, struct bnx2fc_interface,
@@ -423,6 +424,12 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct
net_device *dev,
goto err;
}
+ tmp_skb = skb_share_check(skb, GFP_ATOMIC);
+ if (!tmp_skb)
+ goto err;
+
+ skb = tmp_skb;
+
if (unlikely(eth_hdr(skb)->h_proto != htons(ETH_P_FCOE))) {
printk(KERN_ERR PFX "bnx2fc_rcv: Wrong FC type frame\n");
goto err;
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bnx2fc: do not add shared skbs to the fcoe_rx_list
Hi Eddie,
just want to add you to the CC list.
Some days ago the bnx2fc's maintainer email address has been updated,
this should be the new one: qlogic-storage-upstr...@qlogic.com
I tried to send this patch to the new address but I received the following
delivery failure notification:
Delivery has failed to these recipients or groups:
dept_linux...@qlogic.com<mailto:dept_linux...@qlogic.com>
Your message can't be delivered because delivery to this address is restricted.
On 07/25/2014 10:02 AM, Maurizio Lombardi wrote:
> In some cases, the fcoe_rx_list may contains multiple instances
> of the same skb (the so called "shared skbs").
>
> the bnx2fc_l2_rcv thread is a loop that extracts a skb from the list,
> modifies (and destroys) its content and the proceed to the next one.
> The problem is that if the skb is shared, the remaining instances will
> be corrupted.
>
> The solution is to use skb_share_check() before adding the skb to the
> fcoe_rx_list.
>
> [ 6286.808725] [ cut here ]
> [ 6286.808729] WARNING: at include/scsi/fc_frame.h:173
> bnx2fc_l2_rcv_thread+0x425/0x450 [bnx2fc]()
> [ 6286.808748] Modules linked in: bnx2x(-) mdio dm_service_time bnx2fc cnic
> uio fcoe libfcoe 8021q garp stp mrp libfc llc scsi_transport_fc scsi_tgt sg
> iTCO_wdt iTCO_vendor_support coretemp kvm_intel kvm crct10dif_pclmul
> crc32_pclmul crc32c_intel e1000e ghash_clmulni_intel aesni_intel lrw gf128mul
> glue_helper ablk_helper ptp cryptd hpilo serio_raw hpwdt lpc_ich pps_core
> ipmi_si pcspkr mfd_core ipmi_msghandler shpchp pcc_cpufreq mperf nfsd
> auth_rpcgss nfs_acl lockd sunrpc dm_multipath xfs libcrc32c ata_generic
> pata_acpi sd_mod crc_t10dif crct10dif_common mgag200 syscopyarea sysfillrect
> sysimgblt i2c_algo_bit ata_piix drm_kms_helper ttm drm libata i2c_core hpsa
> dm_mirror dm_region_hash dm_log dm_mod [last unloaded: mdio]
> [ 6286.808750] CPU: 3 PID: 1304 Comm: bnx2fc_l2_threa Not tainted
> 3.10.0-121.el7.x86_64 #1
> [ 6286.808750] Hardware name: HP ProLiant DL120 G7, BIOS J01 07/01/2013
> [ 6286.808752] 0b36e715 8800deba1e00
> 815ec0ba
> [ 6286.808753] 8800deba1e38 8105dee1 a05618c0
> 8801e4c81888
> [ 6286.808754] e8663868 8801f402b180 8801f56bc000
> 8800deba1e48
> [ 6286.808754] Call Trace:
> [ 6286.808759] [] dump_stack+0x19/0x1b
> [ 6286.808762] [] warn_slowpath_common+0x61/0x80
> [ 6286.808763] [] warn_slowpath_null+0x1a/0x20
> [ 6286.808765] [] bnx2fc_l2_rcv_thread+0x425/0x450 [bnx2fc]
> [ 6286.808767] [] ? bnx2fc_disable+0x90/0x90 [bnx2fc]
> [ 6286.808769] [] kthread+0xcf/0xe0
> [ 6286.808770] [] ? kthread_create_on_node+0x140/0x140
> [ 6286.808772] [] ret_from_fork+0x7c/0xb0
> [ 6286.808773] [] ? kthread_create_on_node+0x140/0x140
> [ 6286.808774] ---[ end trace c6cdb939184ccb4e ]---
>
> Signed-off-by: Maurizio Lombardi
> ---
> drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> index 785d0d7..a190ab6 100644
> --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> @@ -411,6 +411,7 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct
> net_device *dev,
> struct fc_frame_header *fh;
> struct fcoe_rcv_info *fr;
> struct fcoe_percpu_s *bg;
> + struct sk_buff *tmp_skb;
> unsigned short oxid;
>
> interface = container_of(ptype, struct bnx2fc_interface,
> @@ -423,6 +424,12 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct
> net_device *dev,
> goto err;
> }
>
> + tmp_skb = skb_share_check(skb, GFP_ATOMIC);
> + if (!tmp_skb)
> + goto err;
> +
> + skb = tmp_skb;
> +
> if (unlikely(eth_hdr(skb)->h_proto != htons(ETH_P_FCOE))) {
> printk(KERN_ERR PFX "bnx2fc_rcv: Wrong FC type frame\n");
> goto err;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] qla4xxx: check the return value of dma_alloc_coherent()
the qla4xxx_alloc_fw_dump() calls dma_alloc_coherent() but does not
check its return value.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/qla4xxx/ql4_init.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/scsi/qla4xxx/ql4_init.c b/drivers/scsi/qla4xxx/ql4_init.c
index 6f12f85..4180d6d 100644
--- a/drivers/scsi/qla4xxx/ql4_init.c
+++ b/drivers/scsi/qla4xxx/ql4_init.c
@@ -334,6 +334,12 @@ void qla4xxx_alloc_fw_dump(struct scsi_qla_host *ha)
/* Allocate memory for saving the template */
md_tmp = dma_alloc_coherent(&ha->pdev->dev, ha->fw_dump_tmplt_size,
&md_tmp_dma, GFP_KERNEL);
+ if (!md_tmp) {
+ ql4_printk(KERN_INFO, ha,
+ "scsi%ld: Failed to allocate DMA memory\n",
+ ha->host_no);
+ return;
+ }
/* Request template */
status = qla4xxx_get_minidump_template(ha, md_tmp_dma);
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] bnx2fc: fix incorrect DMA memory mapping in bnx2fc_map_sg()
In the bnx2fc_map_sg() function, the original behaviour is to
allocate the DMA memory by directly calling dma_map_sg()
instead of using scsi_dma_map().
In contrast, bnx2fc_unmap_sg_list() calls scsi_dma_unmap().
The problem is that bnx2fc_map_sg() passes to the dma_map_sg() function
the pointer to the "device" structure taken from pci_dev
and not from Scsi_Host (as scsi_dma_map/unmap() do).
In some circumstances, the two devices may be different and the
DMA library will be unable to find the correct entry
when freeing the memory.
This patch fixes the bug by replacing dma_map_sg() with scsi_dma_map().
[56068.747547] WARNING: CPU: 1 PID: 12048 at lib/dma-debug.c:1080
check_unmap+0x90a/0xa00()
[56068.785706] fcoe ctlr_0: DMA-API: device driver tries to free DMA memory it
has not allocated [device address=0x000202a1aa00] [size=36 bytes]
[56068.846700] Modules linked in: 8021q garp stp mrp llc fcoe bnx2fc cnic uio
libfcoe libfc scsi_transport_fc scsi_tgt cfg80211 rfkill x86_pkg_temp_thermal
coretemp kvm_intel kvm crct10dif_pclmul iTCO_wdt crc32_pclmul gpio_ich
crc32c_intel iTCO_vendor_support ghash_clmulni_intel joydev lpc_ich hpwdt nfsd
mfd_core microcode hpilo serio_raw pcspkr ipmi_si auth_rpcgss nfs_acl lockd
shpchp ipmi_msghandler sunrpc xfs e1000e mgag200 i2c_algo_bit drm_kms_helper
ttm ptp drm ata_generic pata_acpi bnx2x i2c_core pps_core hpsa mdio libcrc32c
[56069.077673] CPU: 1 PID: 12048 Comm: bnx2fc_thread/1 Not tainted 3.16.0-rc7+
#1
[56069.115286] Hardware name: HP ProLiant DL120 G7, BIOS J01 07/01/2013
[56069.145091] 0009 8801f36dbbd8 816c323a
8801f36dbc20
[56069.185083] 8801f36dbc10 8108350d 8800e9880960
8801f36dbd00
[56069.222738] 0024 000202a1aa00 0001
8801f36dbc70
[56069.263389] Call Trace:
[56069.275945] [] dump_stack+0x45/0x56
[56069.302133] [] warn_slowpath_common+0x7d/0xa0
[56069.334050] [] warn_slowpath_fmt+0x4c/0x50
[56069.364537] [] ? debug_dma_mapping_error+0x7c/0x90
[56069.398268] [] check_unmap+0x90a/0xa00
[56069.423761] [] debug_dma_unmap_sg+0x65/0x110
[56069.474134] [] scsi_dma_unmap+0x73/0xb0
[56069.499313] [] bnx2fc_process_scsi_cmd_compl+0xcc/0x2a0
[bnx2fc]
[56069.535588] [] bnx2fc_process_cq_compl+0x21b/0x280
[bnx2fc]
[56069.570311] [] bnx2fc_percpu_io_thread+0x106/0x180
[bnx2fc]
[56069.604560] [] ? bnx2fc_get_src_mac+0x20/0x20 [bnx2fc]
[56069.635801] [] kthread+0xd2/0xf0
[56069.659421] [] ? kthread_create_on_node+0x170/0x170
[56069.689416] [] ret_from_fork+0x7c/0xb0
[56069.715510] [] ? kthread_create_on_node+0x170/0x170
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_io.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
index 7bc47fc..fe2cd9b 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -1640,8 +1640,6 @@ static int bnx2fc_split_bd(struct bnx2fc_cmd *io_req, u64
addr, int sg_len,
static int bnx2fc_map_sg(struct bnx2fc_cmd *io_req)
{
- struct bnx2fc_interface *interface = io_req->port->priv;
- struct bnx2fc_hba *hba = interface->hba;
struct scsi_cmnd *sc = io_req->sc_cmd;
struct fcoe_bd_ctx *bd = io_req->bd_tbl->bd_tbl;
struct scatterlist *sg;
@@ -1653,8 +1651,8 @@ static int bnx2fc_map_sg(struct bnx2fc_cmd *io_req)
u64 addr;
int i;
- sg_count = dma_map_sg(&hba->pcidev->dev, scsi_sglist(sc),
- scsi_sg_count(sc), sc->sc_data_direction);
+ sg_count = scsi_dma_map(sc);
+
scsi_for_each_sg(sc, sg, sg_count, i) {
sg_len = sg_dma_len(sg);
addr = sg_dma_address(sg);
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bnx2fc: fix incorrect DMA memory mapping in bnx2fc_map_sg()
Hi Eddie,
On 08/20/2014 07:35 PM, Eddie Wai wrote:
> On Mon, 2014-08-04 at 10:20 +0200, Maurizio Lombardi wrote:
>> In the bnx2fc_map_sg() function, the original behaviour is to
>> allocate the DMA memory by directly calling dma_map_sg()
>> instead of using scsi_dma_map().
>>
>> In contrast, bnx2fc_unmap_sg_list() calls scsi_dma_unmap().
>>
>> The problem is that bnx2fc_map_sg() passes to the dma_map_sg() function
>> the pointer to the "device" structure taken from pci_dev
>> and not from Scsi_Host (as scsi_dma_map/unmap() do).
>>
> Great find, Maurizio, Thanks again.
Thanks :)
>
>> In some circumstances, the two devices may be different and the
>> DMA library will be unable to find the correct entry
>> when freeing the memory.
>>
> I'm curious at how the hba->pcidev->dev in the dma_map_sg call could end
> up being different from the scsi_cmnd's device->host->dma_dev... Can
> you share the test scenario?
It happens every time you set up an fcoe interface, so you just have to compile
the
kernel with the DMA API debug option.
It is 100% reproducible with RHEL6, RHEL7 and the latest mainline kernel also.
>
> I see that scsi_dma_map could possibly return a -ENOMEM. It would be
> best if we also add the check for sg_count being < 0 and return the
> bd_count or 0.
>
>> scsi_for_each_sg(sc, sg, sg_count, i) {
>> sg_len = sg_dma_len(sg);
>> addr = sg_dma_address(sg);
True, but sg_count is only used for the "scsi_for_each()",
if it is 0 or -ENOMEM it will simply skip the loop and will execute "return
bd_count".
Regards,
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bnx2fc: fix incorrect DMA memory mapping in bnx2fc_map_sg()
Hi Chad,
On 08/22/2014 02:08 PM, Chad Dupuis wrote:
> Eddie, Maurizio,
>
> Since it looks like there can be a difference in the pdev would it make sense
> instead to convert the unmap function to use the bare DMA API so we're
> consistent in our use of hba->pcidev->dev? Maybe something like this:
I looked at what the bnx2i driver code does, it has a hba->pcidev pointer too
but makes use of scsi_map_sg()/scsi_unmap_sg().
So, from my point of view, it is the bnx2fc's map operation (that bypasses
scsi_map_sg() and uses dma_map_sg()) to look like a suspicious exception and I
decided to change it.
So far, I didn't get any error or strange behaviour after this change.
Eddie, what do you think about it?
Regards,
Maurizio Lombardi
>
> diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
> index 32a5e0a..8b4adcf 100644
> --- a/drivers/scsi/bnx2fc/bnx2fc_io.c
> +++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
> @@ -1700,9 +1700,12 @@ static int bnx2fc_build_bd_list_from_sg(struct
> bnx2fc_cmd *io_req)
> static void bnx2fc_unmap_sg_list(struct bnx2fc_cmd *io_req)
> {
> struct scsi_cmnd *sc = io_req->sc_cmd;
> + struct bnx2fc_interface *interface = io_req->port->priv;
> + struct bnx2fc_hba *hba = interface->hba;
>
> - if (io_req->bd_tbl->bd_valid && sc) {
> - scsi_dma_unmap(sc);
> + if (io_req->bd_tbl->bd_valid && sc && scsi_sg_count(sc)) {
> + dma_unmap_sg(&hba->pcidev->dev, scsi_sglist(sc),
> + scsi_sg_count(sc), sc->sc_data_direction);
> io_req->bd_tbl->bd_valid = 0;
> }
> }
>
> On Fri, 22 Aug 2014, Maurizio Lombardi wrote:
>
>> Hi Eddie,
>>
>> On 08/20/2014 07:35 PM, Eddie Wai wrote:
>>> On Mon, 2014-08-04 at 10:20 +0200, Maurizio Lombardi wrote:
>>>> In the bnx2fc_map_sg() function, the original behaviour is to
>>>> allocate the DMA memory by directly calling dma_map_sg()
>>>> instead of using scsi_dma_map().
>>>>
>>>> In contrast, bnx2fc_unmap_sg_list() calls scsi_dma_unmap().
>>>>
>>>> The problem is that bnx2fc_map_sg() passes to the dma_map_sg() function
>>>> the pointer to the "device" structure taken from pci_dev
>>>> and not from Scsi_Host (as scsi_dma_map/unmap() do).
>>>>
>>> Great find, Maurizio, Thanks again.
>>
>> Thanks :)
>>
>>>
>>>> In some circumstances, the two devices may be different and the
>>>> DMA library will be unable to find the correct entry
>>>> when freeing the memory.
>>>>
>>> I'm curious at how the hba->pcidev->dev in the dma_map_sg call could end
>>> up being different from the scsi_cmnd's device->host->dma_dev... Can
>>> you share the test scenario?
>>
>> It happens every time you set up an fcoe interface, so you just have to
>> compile the
>> kernel with the DMA API debug option.
>> It is 100% reproducible with RHEL6, RHEL7 and the latest mainline kernel
>> also.
>>
>>>
>>> I see that scsi_dma_map could possibly return a -ENOMEM. It would be
>>> best if we also add the check for sg_count being < 0 and return the
>>> bd_count or 0.
>>>
>>>> scsi_for_each_sg(sc, sg, sg_count, i) {
>>>> sg_len = sg_dma_len(sg);
>>>> addr = sg_dma_address(sg);
>>
>> True, but sg_count is only used for the "scsi_for_each()",
>> if it is 0 or -ENOMEM it will simply skip the loop and will execute "return
>> bd_count".
>>
>> Regards,
>> Maurizio Lombardi
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bnx2fc: do not add shared skbs to the fcoe_rx_list
Hi Eddie,
On 08/23/2014 01:02 AM, Eddie Wai wrote:
> is this still a problem? The mail reflector seems to work for me...
>
Works for me now.
Regards,
Maurizio Lombardi
>>
>> On 07/25/2014 10:02 AM, Maurizio Lombardi wrote:
>>> In some cases, the fcoe_rx_list may contains multiple instances
>>> of the same skb (the so called "shared skbs").
>>>
>>> the bnx2fc_l2_rcv thread is a loop that extracts a skb from the list,
>>> modifies (and destroys) its content and the proceed to the next one.
>>> The problem is that if the skb is shared, the remaining instances will
>>> be corrupted.
>>>
>>> The solution is to use skb_share_check() before adding the skb to the
>>> fcoe_rx_list.
>>>
>>> [ 6286.808725] [ cut here ]
>>> [ 6286.808729] WARNING: at include/scsi/fc_frame.h:173
>>> bnx2fc_l2_rcv_thread+0x425/0x450 [bnx2fc]()
>>> [ 6286.808748] Modules linked in: bnx2x(-) mdio dm_service_time bnx2fc cnic
>>> uio fcoe libfcoe 8021q garp stp mrp libfc llc scsi_transport_fc scsi_tgt sg
>>> iTCO_wdt iTCO_vendor_support coretemp kvm_intel kvm crct10dif_pclmul
>>> crc32_pclmul crc32c_intel e1000e ghash_clmulni_intel aesni_intel lrw
>>> gf128mul glue_helper ablk_helper ptp cryptd hpilo serio_raw hpwdt lpc_ich
>>> pps_core ipmi_si pcspkr mfd_core ipmi_msghandler shpchp pcc_cpufreq mperf
>>> nfsd auth_rpcgss nfs_acl lockd sunrpc dm_multipath xfs libcrc32c
>>> ata_generic pata_acpi sd_mod crc_t10dif crct10dif_common mgag200
>>> syscopyarea sysfillrect sysimgblt i2c_algo_bit ata_piix drm_kms_helper ttm
>>> drm libata i2c_core hpsa dm_mirror dm_region_hash dm_log dm_mod [last
>>> unloaded: mdio]
>>> [ 6286.808750] CPU: 3 PID: 1304 Comm: bnx2fc_l2_threa Not tainted
>>> 3.10.0-121.el7.x86_64 #1
>>> [ 6286.808750] Hardware name: HP ProLiant DL120 G7, BIOS J01 07/01/2013
>>> [ 6286.808752] 0b36e715 8800deba1e00
>>> 815ec0ba
>>> [ 6286.808753] 8800deba1e38 8105dee1 a05618c0
>>> 8801e4c81888
>>> [ 6286.808754] e8663868 8801f402b180 8801f56bc000
>>> 8800deba1e48
>>> [ 6286.808754] Call Trace:
>>> [ 6286.808759] [] dump_stack+0x19/0x1b
>>> [ 6286.808762] [] warn_slowpath_common+0x61/0x80
>>> [ 6286.808763] [] warn_slowpath_null+0x1a/0x20
>>> [ 6286.808765] [] bnx2fc_l2_rcv_thread+0x425/0x450
>>> [bnx2fc]
>>> [ 6286.808767] [] ? bnx2fc_disable+0x90/0x90 [bnx2fc]
>>> [ 6286.808769] [] kthread+0xcf/0xe0
>>> [ 6286.808770] [] ? kthread_create_on_node+0x140/0x140
>>> [ 6286.808772] [] ret_from_fork+0x7c/0xb0
>>> [ 6286.808773] [] ? kthread_create_on_node+0x140/0x140
>>> [ 6286.808774] ---[ end trace c6cdb939184ccb4e ]---
>>>
>>> Signed-off-by: Maurizio Lombardi
>>> ---
>>> drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 7 +++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
>>> b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
>>> index 785d0d7..a190ab6 100644
>>> --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
>>> +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
>>> @@ -411,6 +411,7 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct
>>> net_device *dev,
>>> struct fc_frame_header *fh;
>>> struct fcoe_rcv_info *fr;
>>> struct fcoe_percpu_s *bg;
>>> + struct sk_buff *tmp_skb;
>>> unsigned short oxid;
>>>
>>> interface = container_of(ptype, struct bnx2fc_interface,
>>> @@ -423,6 +424,12 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct
>>> net_device *dev,
>>> goto err;
>>> }
>>>
>>> + tmp_skb = skb_share_check(skb, GFP_ATOMIC);
>>> + if (!tmp_skb)
>>> + goto err;
>>> +
>>> + skb = tmp_skb;
>>> +
>>> if (unlikely(eth_hdr(skb)->h_proto != htons(ETH_P_FCOE))) {
>>> printk(KERN_ERR PFX "bnx2fc_rcv: Wrong FC type frame\n");
>>> goto err;
>>>
> Seems logical, but this patch requires some testing which ought to be
> verified by the Qlogic team. Thanks.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND] bnx2fc: do not add shared skbs to the fcoe_rx_list
In some cases, the fcoe_rx_list may contains multiple instances
of the same skb (the so called "shared skbs").
the bnx2fc_l2_rcv thread is a loop that extracts a skb from the list,
modifies (and destroys) its content and the proceed to the next one.
The problem is that if the skb is shared, the remaining instances will
be corrupted.
The solution is to use skb_share_check() before adding the skb to the
fcoe_rx_list.
[ 6286.808725] [ cut here ]
[ 6286.808729] WARNING: at include/scsi/fc_frame.h:173
bnx2fc_l2_rcv_thread+0x425/0x450 [bnx2fc]()
[ 6286.808748] Modules linked in: bnx2x(-) mdio dm_service_time bnx2fc cnic uio
fcoe libfcoe 8021q garp stp mrp libfc llc scsi_transport_fc scsi_tgt sg
iTCO_wdt iTCO_vendor_support coretemp kvm_intel kvm crct10dif_pclmul
crc32_pclmul crc32c_intel e1000e ghash_clmulni_intel aesni_intel lrw gf128mul
glue_helper ablk_helper ptp cryptd hpilo serio_raw hpwdt lpc_ich pps_core
ipmi_si pcspkr mfd_core ipmi_msghandler shpchp pcc_cpufreq mperf nfsd
auth_rpcgss nfs_acl lockd sunrpc dm_multipath xfs libcrc32c ata_generic
pata_acpi sd_mod crc_t10dif crct10dif_common mgag200 syscopyarea sysfillrect
sysimgblt i2c_algo_bit ata_piix drm_kms_helper ttm drm libata i2c_core hpsa
dm_mirror dm_region_hash dm_log dm_mod [last unloaded: mdio]
[ 6286.808750] CPU: 3 PID: 1304 Comm: bnx2fc_l2_threa Not tainted
3.10.0-121.el7.x86_64 #1
[ 6286.808750] Hardware name: HP ProLiant DL120 G7, BIOS J01 07/01/2013
[ 6286.808752] 0b36e715 8800deba1e00
815ec0ba
[ 6286.808753] 8800deba1e38 8105dee1 a05618c0
8801e4c81888
[ 6286.808754] e8663868 8801f402b180 8801f56bc000
8800deba1e48
[ 6286.808754] Call Trace:
[ 6286.808759] [] dump_stack+0x19/0x1b
[ 6286.808762] [] warn_slowpath_common+0x61/0x80
[ 6286.808763] [] warn_slowpath_null+0x1a/0x20
[ 6286.808765] [] bnx2fc_l2_rcv_thread+0x425/0x450 [bnx2fc]
[ 6286.808767] [] ? bnx2fc_disable+0x90/0x90 [bnx2fc]
[ 6286.808769] [] kthread+0xcf/0xe0
[ 6286.808770] [] ? kthread_create_on_node+0x140/0x140
[ 6286.808772] [] ret_from_fork+0x7c/0xb0
[ 6286.808773] [] ? kthread_create_on_node+0x140/0x140
[ 6286.808774] ---[ end trace c6cdb939184ccb4e ]---
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 79e5c94..72533c5 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -412,6 +412,7 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct
net_device *dev,
struct fc_frame_header *fh;
struct fcoe_rcv_info *fr;
struct fcoe_percpu_s *bg;
+ struct sk_buff *tmp_skb;
unsigned short oxid;
interface = container_of(ptype, struct bnx2fc_interface,
@@ -424,6 +425,12 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct
net_device *dev,
goto err;
}
+ tmp_skb = skb_share_check(skb, GFP_ATOMIC);
+ if (!tmp_skb)
+ goto err;
+
+ skb = tmp_skb;
+
if (unlikely(eth_hdr(skb)->h_proto != htons(ETH_P_FCOE))) {
printk(KERN_ERR PFX "bnx2fc_rcv: Wrong FC type frame\n");
goto err;
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bnx2i: Make boot_nic entry visible in the sysfs session objects
Hi Cristoph, Jens, On 05/19/2014 01:32 PM, vikas.chaudh...@qlogic.com wrote: > From: Tej Parkash > > Signed-off-by: Tej Parkash > Signed-off-by: Vikas Chaudhary > --- > drivers/scsi/bnx2i/bnx2i_iscsi.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c > b/drivers/scsi/bnx2i/bnx2i_iscsi.c > index 166543f..cabc8c1 100644 > --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c > +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c > @@ -2234,6 +2234,9 @@ static umode_t bnx2i_attr_is_visible(int param_type, > int param) > case ISCSI_PARAM_TGT_RESET_TMO: > case ISCSI_PARAM_IFACE_NAME: > case ISCSI_PARAM_INITIATOR_NAME: > + case ISCSI_PARAM_BOOT_ROOT: > + case ISCSI_PARAM_BOOT_NIC: > + case ISCSI_PARAM_BOOT_TARGET: > return S_IRUGO; > default: > return 0; > Can you merge this patch? It has been ACKed already. Thanks, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] be2iscsi: set the boot_kset pointer to NULL in case of failure
In beiscsi_setup_boot_info(), the boot_kset pointer should be set to NULL in case of failure otherwise an invalid pointer dereference may occur later. Signed-off-by: Maurizio Lombardi --- drivers/scsi/be2iscsi/be_main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c index cb9072a..069e5c5 100644 --- a/drivers/scsi/be2iscsi/be_main.c +++ b/drivers/scsi/be2iscsi/be_main.c @@ -4468,6 +4468,7 @@ put_shost: scsi_host_put(phba->shost); free_kset: iscsi_boot_destroy_kset(phba->boot_kset); + phba->boot_kset = NULL; return -ENOMEM; } -- Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] be2iscsi: set the boot_kset pointer to NULL in case of failure
On 03/08/2016 03:03 AM, Martin K. Petersen wrote: >>>>>> "Maurizio" == Maurizio Lombardi writes: > > Maurizio, > > Maurizio> In beiscsi_setup_boot_info(), the boot_kset pointer should be > Maurizio> set to NULL in case of failure otherwise an invalid pointer > Maurizio> dereference may occur later. > > iscsi_boot_destroy_kset() checks before deref and the other use location > just checks to see whether it's NULL. Are there places in the core iSCSI > code that use this without checking? 1) At the beginning of the beiscsi_setup_boot_info() function there is the following check: -- /* it has been created previously */ if (phba->boot_kset) return 0; -- If the function fails and the boot_kset pointer is not set to NULL, subsequent calls to beiscsi_setup_boot_info() will incorrectly return success because it assumes that the boot_kset pointer is valid. 2) it is true that iscsi_boot_destroy_kset() checks whether the pointer is NULL or not, but it the kset has been already destroyed and the pointer is not set to NULL, then it will dereference an invalid pointer. Regards, Maurizio -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] fnic: move printk()s outside of the critical code section.
This patch moves a printk() outside of the code section where
interrupt are disabled. In some cases a flood of error messages may
cause a kernel panic.
It also removes one of the printk()s because the same error
message was printed twice.
[709686.317197] Kernel panic - not syncing: Watchdog detected hard LOCKUP on
cpu 12
[709686.317200] CPU: 12 PID: 1963 Comm: systemd-journal Tainted: GF
O-- 3.10.0-229.el7.x86_64 #1
[709686.317201] Hardware name: Cisco Systems Inc UCSB-B200-M3/UCSB-B200-M3,
BIOS B200M3.2.2.3.6.030620151309 03/06/2015
[709686.317206] 8182b2e8 392722ba 88046fcc5c48
81603f36
[709686.317209] 88046fcc5cc8 815fd7da 0010
88046fcc5cd8
[709686.317211] 88046fcc5c78 392722ba 88046fcc5c88
000c
[709686.317212] Call Trace:
[709686.317221][] dump_stack+0x19/0x1b
[709686.317223] [] panic+0xd8/0x1e7
[709686.317227] [] ?
watchdog_enable_all_cpus.part.2+0x40/0x40
[709686.317229] [] watchdog_overflow_callback+0xc2/0xd0
[709686.317233] [] __perf_event_overflow+0xa1/0x250
[709686.317235] [] perf_event_overflow+0x14/0x20
[709686.317239] [] intel_pmu_handle_irq+0x1fd/0x410
[709686.317242] [] ? unmap_kernel_range_noflush+0x11/0x20
[709686.317246] [] ? ghes_copy_tofrom_phys+0x124/0x210
[709686.317249] [] perf_event_nmi_handler+0x2b/0x50
[709686.317251] [] nmi_handle.isra.0+0x69/0xb0
[709686.317252] [] do_nmi+0xd0/0x340
[709686.317256] [] end_repeat_nmi+0x1e/0x2e
[709686.317260] [] ? memcpy+0xd/0x110
[709686.317263] [] ? memcpy+0xd/0x110
[709686.317265] [] ? memcpy+0xd/0x110
[709686.317269] <> [] ? vgacon_scroll+0x2d7/0x330
[709686.317273] [] scrup+0xfc/0x110
[709686.317275] [] lf+0xa0/0xb0
[709686.317278] [] vt_console_print+0x2d2/0x420
[709686.317283] []
call_console_drivers.constprop.15+0x91/0xf0
[709686.317287] [] console_unlock+0x3bf/0x400
[709686.317291] [] vprintk_emit+0x2b6/0x530
[709686.317294] [] printk_emit+0x44/0x5b
[709686.317297] [] devkmsg_writev+0x158/0x1d0
[709686.317303] [] do_sync_readv_writev+0x79/0xd0
[709686.317307] [] do_readv_writev+0xce/0x260
[709686.317310] [] ? __sb_start_write+0x58/0x110
[709686.317314] [] vfs_writev+0x35/0x60
[709686.317318] [] SyS_writev+0x5c/0xd0
[709686.317322] [] system_call_fastpath+0x16/0x1b
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/fnic/fnic_scsi.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index 266b909..f3032ca 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -958,23 +958,22 @@ static void fnic_fcpio_icmnd_cmpl_handler(struct fnic
*fnic,
case FCPIO_INVALID_PARAM:/* some parameter in request invalid */
case FCPIO_REQ_NOT_SUPPORTED:/* request type is not supported */
default:
- shost_printk(KERN_ERR, fnic->lport->host, "hdr status = %s\n",
-fnic_fcpio_status_to_str(hdr_status));
sc->result = (DID_ERROR << 16) | icmnd_cmpl->scsi_status;
break;
}
- if (hdr_status != FCPIO_SUCCESS) {
- atomic64_inc(&fnic_stats->io_stats.io_failures);
- shost_printk(KERN_ERR, fnic->lport->host, "hdr status = %s\n",
-fnic_fcpio_status_to_str(hdr_status));
- }
/* Break link with the SCSI command */
CMD_SP(sc) = NULL;
CMD_FLAGS(sc) |= FNIC_IO_DONE;
spin_unlock_irqrestore(io_lock, flags);
+ if (hdr_status != FCPIO_SUCCESS) {
+ atomic64_inc(&fnic_stats->io_stats.io_failures);
+ shost_printk(KERN_ERR, fnic->lport->host, "hdr status = %s\n",
+fnic_fcpio_status_to_str(hdr_status));
+ }
+
fnic_release_ioreq_buf(fnic, io_req, sc);
mempool_free(io_req, fnic->io_req_pool);
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] bnx2fc: replace printk() with BNX2FC_IO_DBG()
The "fcp_rsp_code = %d" message isn't an error, it's meant to
be informative only.
This patch prevents a flood of such messages in some situations.
Tested-by: Laurence Oberman
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_io.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
index 026f394..8f24d60 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -1758,7 +1758,7 @@ static void bnx2fc_parse_fcp_rsp(struct bnx2fc_cmd
*io_req,
if ((fcp_rsp_len == 4) || (fcp_rsp_len == 8)) {
/* Only for task management function */
io_req->fcp_rsp_code = rq_data[3];
- printk(KERN_ERR PFX "fcp_rsp_code = %d\n",
+ BNX2FC_IO_DBG(io_req, "fcp_rsp_code = %d\n",
io_req->fcp_rsp_code);
}
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] be2iscsi: Use pci_zalloc_consistent() where possible and get rid of memset(,0,)
---
drivers/scsi/be2iscsi/be_iscsi.c | 3 +--
drivers/scsi/be2iscsi/be_main.c | 3 +--
drivers/scsi/be2iscsi/be_mgmt.c | 3 +--
3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c
index b7391a3..85d42b1 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -1133,7 +1133,7 @@ static int beiscsi_open_conn(struct iscsi_endpoint *ep,
else
req_memsize = sizeof(struct tcp_connect_and_offload_in_v1);
- nonemb_cmd.va = pci_alloc_consistent(phba->ctrl.pdev,
+ nonemb_cmd.va = pci_zalloc_consistent(phba->ctrl.pdev,
req_memsize,
&nonemb_cmd.dma);
if (nonemb_cmd.va == NULL) {
@@ -1146,7 +1146,6 @@ static int beiscsi_open_conn(struct iscsi_endpoint *ep,
return -ENOMEM;
}
nonemb_cmd.size = req_memsize;
- memset(nonemb_cmd.va, 0, nonemb_cmd.size);
tag = mgmt_open_connection(phba, dst_addr, beiscsi_ep, &nonemb_cmd);
if (tag <= 0) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_CONFIG,
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index a7cc618..b63344c 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -344,7 +344,7 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc)
spin_unlock_bh(&session->frwd_lock);
inv_tbl = phba->inv_tbl;
- nonemb_cmd.va = pci_alloc_consistent(phba->ctrl.pdev,
+ nonemb_cmd.va = pci_zalloc_consistent(phba->ctrl.pdev,
sizeof(struct invalidate_commands_params_in),
&nonemb_cmd.dma);
if (nonemb_cmd.va == NULL) {
@@ -354,7 +354,6 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc)
return FAILED;
}
nonemb_cmd.size = sizeof(struct invalidate_commands_params_in);
- memset(nonemb_cmd.va, 0, nonemb_cmd.size);
tag = mgmt_invalidate_icds(phba, inv_tbl, num_invalidate,
cid, &nonemb_cmd);
if (!tag) {
diff --git a/drivers/scsi/be2iscsi/be_mgmt.c b/drivers/scsi/be2iscsi/be_mgmt.c
index 681d4e8..b27a625 100644
--- a/drivers/scsi/be2iscsi/be_mgmt.c
+++ b/drivers/scsi/be2iscsi/be_mgmt.c
@@ -428,7 +428,7 @@ int mgmt_check_supported_fw(struct be_ctrl_info *ctrl,
struct be_sge *sge = nonembedded_sgl(wrb);
int status = 0;
- nonemb_cmd.va = pci_alloc_consistent(ctrl->pdev,
+ nonemb_cmd.va = pci_zalloc_consistent(ctrl->pdev,
sizeof(struct be_mgmt_controller_attributes),
&nonemb_cmd.dma);
if (nonemb_cmd.va == NULL) {
@@ -439,7 +439,6 @@ int mgmt_check_supported_fw(struct be_ctrl_info *ctrl,
}
nonemb_cmd.size = sizeof(struct be_mgmt_controller_attributes);
req = nonemb_cmd.va;
- memset(req, 0, sizeof(*req));
spin_lock(&ctrl->mbox_lock);
memset(wrb, 0, sizeof(*wrb));
be_wrb_hdr_prepare(wrb, sizeof(*req), false, 1);
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] be2iscsi: Use pci_zalloc_consistent() where possible and get rid of memset(,0,)
I forgot the "signed-off-by", I'll send a new version.
On Fri, 2015-03-06 at 14:52 +0100, Maurizio Lombardi wrote:
> ---
> drivers/scsi/be2iscsi/be_iscsi.c | 3 +--
> drivers/scsi/be2iscsi/be_main.c | 3 +--
> drivers/scsi/be2iscsi/be_mgmt.c | 3 +--
> 3 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/be2iscsi/be_iscsi.c
> b/drivers/scsi/be2iscsi/be_iscsi.c
> index b7391a3..85d42b1 100644
> --- a/drivers/scsi/be2iscsi/be_iscsi.c
> +++ b/drivers/scsi/be2iscsi/be_iscsi.c
> @@ -1133,7 +1133,7 @@ static int beiscsi_open_conn(struct iscsi_endpoint *ep,
> else
> req_memsize = sizeof(struct tcp_connect_and_offload_in_v1);
>
> - nonemb_cmd.va = pci_alloc_consistent(phba->ctrl.pdev,
> + nonemb_cmd.va = pci_zalloc_consistent(phba->ctrl.pdev,
> req_memsize,
> &nonemb_cmd.dma);
> if (nonemb_cmd.va == NULL) {
> @@ -1146,7 +1146,6 @@ static int beiscsi_open_conn(struct iscsi_endpoint *ep,
> return -ENOMEM;
> }
> nonemb_cmd.size = req_memsize;
> - memset(nonemb_cmd.va, 0, nonemb_cmd.size);
> tag = mgmt_open_connection(phba, dst_addr, beiscsi_ep, &nonemb_cmd);
> if (tag <= 0) {
> beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_CONFIG,
> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index a7cc618..b63344c 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -344,7 +344,7 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc)
> spin_unlock_bh(&session->frwd_lock);
> inv_tbl = phba->inv_tbl;
>
> - nonemb_cmd.va = pci_alloc_consistent(phba->ctrl.pdev,
> + nonemb_cmd.va = pci_zalloc_consistent(phba->ctrl.pdev,
> sizeof(struct invalidate_commands_params_in),
> &nonemb_cmd.dma);
> if (nonemb_cmd.va == NULL) {
> @@ -354,7 +354,6 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc)
> return FAILED;
> }
> nonemb_cmd.size = sizeof(struct invalidate_commands_params_in);
> - memset(nonemb_cmd.va, 0, nonemb_cmd.size);
> tag = mgmt_invalidate_icds(phba, inv_tbl, num_invalidate,
> cid, &nonemb_cmd);
> if (!tag) {
> diff --git a/drivers/scsi/be2iscsi/be_mgmt.c b/drivers/scsi/be2iscsi/be_mgmt.c
> index 681d4e8..b27a625 100644
> --- a/drivers/scsi/be2iscsi/be_mgmt.c
> +++ b/drivers/scsi/be2iscsi/be_mgmt.c
> @@ -428,7 +428,7 @@ int mgmt_check_supported_fw(struct be_ctrl_info *ctrl,
> struct be_sge *sge = nonembedded_sgl(wrb);
> int status = 0;
>
> - nonemb_cmd.va = pci_alloc_consistent(ctrl->pdev,
> + nonemb_cmd.va = pci_zalloc_consistent(ctrl->pdev,
> sizeof(struct be_mgmt_controller_attributes),
> &nonemb_cmd.dma);
> if (nonemb_cmd.va == NULL) {
> @@ -439,7 +439,6 @@ int mgmt_check_supported_fw(struct be_ctrl_info *ctrl,
> }
> nonemb_cmd.size = sizeof(struct be_mgmt_controller_attributes);
> req = nonemb_cmd.va;
> - memset(req, 0, sizeof(*req));
> spin_lock(&ctrl->mbox_lock);
> memset(wrb, 0, sizeof(*wrb));
> be_wrb_hdr_prepare(wrb, sizeof(*req), false, 1);
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V2] be2iscsi: Use pci_zalloc_consistent() where possible and get rid of memset(,0,)
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/be2iscsi/be_iscsi.c | 3 +--
drivers/scsi/be2iscsi/be_main.c | 3 +--
drivers/scsi/be2iscsi/be_mgmt.c | 3 +--
3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c
index b7391a3..85d42b1 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -1133,7 +1133,7 @@ static int beiscsi_open_conn(struct iscsi_endpoint *ep,
else
req_memsize = sizeof(struct tcp_connect_and_offload_in_v1);
- nonemb_cmd.va = pci_alloc_consistent(phba->ctrl.pdev,
+ nonemb_cmd.va = pci_zalloc_consistent(phba->ctrl.pdev,
req_memsize,
&nonemb_cmd.dma);
if (nonemb_cmd.va == NULL) {
@@ -1146,7 +1146,6 @@ static int beiscsi_open_conn(struct iscsi_endpoint *ep,
return -ENOMEM;
}
nonemb_cmd.size = req_memsize;
- memset(nonemb_cmd.va, 0, nonemb_cmd.size);
tag = mgmt_open_connection(phba, dst_addr, beiscsi_ep, &nonemb_cmd);
if (tag <= 0) {
beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_CONFIG,
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index a7cc618..b63344c 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -344,7 +344,7 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc)
spin_unlock_bh(&session->frwd_lock);
inv_tbl = phba->inv_tbl;
- nonemb_cmd.va = pci_alloc_consistent(phba->ctrl.pdev,
+ nonemb_cmd.va = pci_zalloc_consistent(phba->ctrl.pdev,
sizeof(struct invalidate_commands_params_in),
&nonemb_cmd.dma);
if (nonemb_cmd.va == NULL) {
@@ -354,7 +354,6 @@ static int beiscsi_eh_device_reset(struct scsi_cmnd *sc)
return FAILED;
}
nonemb_cmd.size = sizeof(struct invalidate_commands_params_in);
- memset(nonemb_cmd.va, 0, nonemb_cmd.size);
tag = mgmt_invalidate_icds(phba, inv_tbl, num_invalidate,
cid, &nonemb_cmd);
if (!tag) {
diff --git a/drivers/scsi/be2iscsi/be_mgmt.c b/drivers/scsi/be2iscsi/be_mgmt.c
index 681d4e8..b27a625 100644
--- a/drivers/scsi/be2iscsi/be_mgmt.c
+++ b/drivers/scsi/be2iscsi/be_mgmt.c
@@ -428,7 +428,7 @@ int mgmt_check_supported_fw(struct be_ctrl_info *ctrl,
struct be_sge *sge = nonembedded_sgl(wrb);
int status = 0;
- nonemb_cmd.va = pci_alloc_consistent(ctrl->pdev,
+ nonemb_cmd.va = pci_zalloc_consistent(ctrl->pdev,
sizeof(struct be_mgmt_controller_attributes),
&nonemb_cmd.dma);
if (nonemb_cmd.va == NULL) {
@@ -439,7 +439,6 @@ int mgmt_check_supported_fw(struct be_ctrl_info *ctrl,
}
nonemb_cmd.size = sizeof(struct be_mgmt_controller_attributes);
req = nonemb_cmd.va;
- memset(req, 0, sizeof(*req));
spin_lock(&ctrl->mbox_lock);
memset(wrb, 0, sizeof(*wrb));
be_wrb_hdr_prepare(wrb, sizeof(*req), false, 1);
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] megaraid: fix null pointer check in megasas_detach_one().
The pd_seq_sync pointer can't be NULL, we have to check
its entries instead.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/megaraid/megaraid_sas_base.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
b/drivers/scsi/megaraid/megaraid_sas_base.c
index 97a1c1c..f083e74 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -5941,11 +5941,11 @@ static void megasas_detach_one(struct pci_dev *pdev)
if (fusion->ld_drv_map[i])
free_pages((ulong)fusion->ld_drv_map[i],
fusion->drv_map_pages);
- if (fusion->pd_seq_sync)
- dma_free_coherent(&instance->pdev->dev,
- pd_seq_map_sz,
- fusion->pd_seq_sync[i],
- fusion->pd_seq_phys[i]);
+ if (fusion->pd_seq_sync[i])
+ dma_free_coherent(&instance->pdev->dev,
+ pd_seq_map_sz,
+ fusion->pd_seq_sync[i],
+ fusion->pd_seq_phys[i]);
}
free_pages((ulong)instance->ctrl_context,
instance->ctrl_context_pages);
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] bnx2fc: bnx2fc_eh_abort(): fix wrong return code.
If the link is not ready, the bnx2fc_eh_abort() function
should return FAILED.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_io.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
index 0002caf..2230dab 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -1104,8 +1104,7 @@ int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd)
struct bnx2fc_cmd *io_req;
struct fc_lport *lport;
struct bnx2fc_rport *tgt;
- int rc = FAILED;
-
+ int rc;
rc = fc_block_scsi_eh(sc_cmd);
if (rc)
@@ -1114,7 +1113,7 @@ int bnx2fc_eh_abort(struct scsi_cmnd *sc_cmd)
lport = shost_priv(sc_cmd->device->host);
if ((lport->state != LPORT_ST_READY) || !(lport->link_up)) {
printk(KERN_ERR PFX "eh_abort: link not ready\n");
- return rc;
+ return FAILED;
}
tgt = (struct bnx2fc_rport *)&rp[1];
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] be2iscsi: Fix memory leak in beiscsi_alloc_mem()
ping.
On 10/01/2015 10:56 AM, Maurizio Lombardi wrote:
> In case of error, the memory allocated for phwi_ctrlr was not freed.
>
> Signed-off-by: Maurizio Lombardi
> ---
> drivers/scsi/be2iscsi/be_main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index 7a6dbfb..360aa88 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -2706,8 +2706,10 @@ static int beiscsi_alloc_mem(struct beiscsi_hba *phba)
> phwi_ctrlr->wrb_context = kzalloc(sizeof(struct hwi_wrb_context) *
> phba->params.cxns_per_ctrl,
> GFP_KERNEL);
> - if (!phwi_ctrlr->wrb_context)
> + if (!phwi_ctrlr->wrb_context) {
> + kfree(phba->phwi_ctrlr);
> return -ENOMEM;
> + }
>
> phba->init_mem = kcalloc(SE_MEM_MAX, sizeof(*mem_descr),
>GFP_KERNEL);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3] fnic: check pci_map_single() return value
ping?
On 08/12/2015 05:00 PM, Maurizio Lombardi wrote:
> the kernel prints some warnings when compiled with CONFIG_DMA_API_DEBUG.
> This is because the fnic driver doesn't check the return value of
> pci_map_single().
>
> [ 11.942770] scsi host12: fnic
> [ 11.950811] [ cut here ]
> [ 11.950818] WARNING: at lib/dma-debug.c:937 check_unmap+0x47b/0x920()
> [ 11.950821] fnic :0c:00.0: DMA-API: device driver failed to check map
> error[device address=0x002020a30040] [size=44 bytes] [mapped as single]
>
> Signed-off-by: Maurizio Lombardi
> ---
> drivers/scsi/fnic/fnic_fcs.c | 46
> +++
> drivers/scsi/fnic/fnic_scsi.c | 16 +++
> 2 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/fnic/fnic_fcs.c b/drivers/scsi/fnic/fnic_fcs.c
> index bf0bbd4..67669a9 100644
> --- a/drivers/scsi/fnic/fnic_fcs.c
> +++ b/drivers/scsi/fnic/fnic_fcs.c
> @@ -939,6 +939,7 @@ int fnic_alloc_rq_frame(struct vnic_rq *rq)
> struct sk_buff *skb;
> u16 len;
> dma_addr_t pa;
> + int r;
>
> len = FC_FRAME_HEADROOM + FC_MAX_FRAME + FC_FRAME_TAILROOM;
> skb = dev_alloc_skb(len);
> @@ -952,8 +953,19 @@ int fnic_alloc_rq_frame(struct vnic_rq *rq)
> skb_reset_network_header(skb);
> skb_put(skb, len);
> pa = pci_map_single(fnic->pdev, skb->data, len, PCI_DMA_FROMDEVICE);
> +
> + r = pci_dma_mapping_error(fnic->pdev, pa);
> + if (r) {
> + printk(KERN_ERR "PCI mapping failed with error %d\n", r);
> + goto free_skb;
> + }
> +
> fnic_queue_rq_desc(rq, skb, pa, len);
> return 0;
> +
> +free_skb:
> + kfree_skb(skb);
> + return r;
> }
>
> void fnic_free_rq_buf(struct vnic_rq *rq, struct vnic_rq_buf *buf)
> @@ -981,6 +993,7 @@ void fnic_eth_send(struct fcoe_ctlr *fip, struct sk_buff
> *skb)
> struct ethhdr *eth_hdr;
> struct vlan_ethhdr *vlan_hdr;
> unsigned long flags;
> + int r;
>
> if (!fnic->vlan_hw_insert) {
> eth_hdr = (struct ethhdr *)skb_mac_header(skb);
> @@ -1003,18 +1016,27 @@ void fnic_eth_send(struct fcoe_ctlr *fip, struct
> sk_buff *skb)
>
> pa = pci_map_single(fnic->pdev, skb->data, skb->len, PCI_DMA_TODEVICE);
>
> - spin_lock_irqsave(&fnic->wq_lock[0], flags);
> - if (!vnic_wq_desc_avail(wq)) {
> - pci_unmap_single(fnic->pdev, pa, skb->len, PCI_DMA_TODEVICE);
> - spin_unlock_irqrestore(&fnic->wq_lock[0], flags);
> - kfree_skb(skb);
> - return;
> + r = pci_dma_mapping_error(fnic->pdev, pa);
> + if (r) {
> + printk(KERN_ERR "PCI mapping failed with error %d\n", r);
> + goto free_skb;
> }
>
> + spin_lock_irqsave(&fnic->wq_lock[0], flags);
> + if (!vnic_wq_desc_avail(wq))
> + goto irq_restore;
> +
> fnic_queue_wq_eth_desc(wq, skb, pa, skb->len,
> 0 /* hw inserts cos value */,
> fnic->vlan_id, 1);
> spin_unlock_irqrestore(&fnic->wq_lock[0], flags);
> + return;
> +
> +irq_restore:
> + spin_unlock_irqrestore(&fnic->wq_lock[0], flags);
> + pci_unmap_single(fnic->pdev, pa, skb->len, PCI_DMA_TODEVICE);
> +free_skb:
> + kfree_skb(skb);
> }
>
> /*
> @@ -1071,6 +1093,12 @@ static int fnic_send_frame(struct fnic *fnic, struct
> fc_frame *fp)
>
> pa = pci_map_single(fnic->pdev, eth_hdr, tot_len, PCI_DMA_TODEVICE);
>
> + ret = pci_dma_mapping_error(fnic->pdev, pa);
> + if (ret) {
> + printk(KERN_ERR "DMA map failed with error %d\n", ret);
> + goto free_skb_on_err;
> + }
> +
> if ((fnic_fc_trace_set_data(fnic->lport->host->host_no, FNIC_FC_SEND,
> (char *)eth_hdr, tot_len)) != 0) {
> printk(KERN_ERR "fnic ctlr frame trace error!!!");
> @@ -1082,15 +1110,17 @@ static int fnic_send_frame(struct fnic *fnic, struct
> fc_frame *fp)
> pci_unmap_single(fnic->pdev, pa,
>tot_len, PCI_DMA_TODEVICE);
> ret = -1;
> - goto fnic_send_frame_end;
> + goto irq_restore;
> }
>
> fnic_queue_wq_desc(wq, skb, pa, tot_len, fr_eof(fp),
> 0 /* hw inserts cos value */,
> fnic->vlan_id, 1, 1, 1);
> -fnic_send_frame_end:
> +
> +irq_restore:
Re: [RFC PATCH 0/3] fix *pbl format support
Hi Rasmus,
On 09/16/2015 10:35 PM, Rasmus Villemoes wrote:
>
> I just remembered: I noticed a while ago that the qualifier member is
> only used inside format_decode (in the end, the information is folded
> into the type member), so one might as well use a local variable for
> that. This gives option (3): Make field_width a 24 bit bitfield. I think
> that should be sufficient for any realistic bitmap that one would
> print. The patch is also rather small. Surprisingly, bloat-o-meter even
> says that it's also a net win in code size:
I tested your patch with scsi-debug and it works for me:
# modprobe scsi-debug dev_size_mb=256 lbpu=1 lbpws10=1
# cat /sys/bus/pseudo/drivers/scsi_debug/map
# vgcreate tsvg /dev/sdb
Physical volume "/dev/sdb" successfully created
Volume group "tsvg" successfully created
# cat /sys/bus/pseudo/drivers/scsi_debug/map
0-15
# lvcreate -V200m -l99%FREE -T tsvg/pool -n lv1 --discards ignore
Logical volume "lv1" created.
# cat /sys/bus/pseudo/drivers/scsi_debug/map
0-31,2048-2055,501760-501871
Thanks,
Maurizio Lombardi
>
> add/remove: 18/16 grow/shrink: 3/2 up/down: 5551/-5775 (-224)
>
> (the huge absolute numbers are due to stuff like
>
> pointer-1148 +1148
> pointer.isra1116 - -1116
>
> so the functions haven't actually changed that much). I'll have to check
> how this can be (smaller might still be worse), but at least it doesn't
> seem to be a catastrophe in terms of .text bloat.
>
> [Grr, why don't we have a way to do a compile-time assert outside
> function context?]
>
> Only compile-tested.
>
> From: Rasmus Villemoes
> Date: Wed, 16 Sep 2015 22:06:14 +0200
> Subject: [RFC] lib/vsprintf.c: expand field_width to 24 bits
>
> Maurizio Lombardi reported a problem with the %pb extension: It
> doesn't work for sufficiently large bitmaps, since the size is stashed
> in the field_width field of the struct printf_spec, which is currently
> an s16. Concretely, this manifested itself in
> /sys/bus/pseudo/drivers/scsi_debug/map being empty, since the bitmap
> printer got a size of 0, which is the 16 bit truncation of the actual
> bitmap size.
>
> We do want to keep struct printf_spec at 8 bytes so that it can
> cheaply be passed by value. The qualifier field is only used for
> internal bookkeeping in format_decode, so we might as well use a local
> variable for that. This gives us an additional 8 bits, which we can
> then use for the field width. To stay in 8 bytes, we need to do a
> little rearranging and make the type member a bitfield as well.
>
> Signed-off-by: Rasmus Villemoes
> ---
> lib/vsprintf.c | 33 +
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 95cd63b43b99..cce2a780a82e 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -380,13 +380,13 @@ enum format_type {
> };
>
> struct printf_spec {
> - u8 type; /* format_type enum */
> + u8 type:8; /* format_type enum */
> + s32 field_width:24; /* width of output field */
> u8 flags; /* flags to number() */
> u8 base; /* number base, 8, 10 or 16 only */
> - u8 qualifier; /* number qualifier, one of 'hHlLtzZ' */
> - s16 field_width;/* width of output field */
> s16 precision; /* # of digits/chars */
> };
> +extern char __check_printf_spec[1-2*(sizeof(struct printf_spec) != 8)];
>
> static noinline_for_stack
> char *number(char *buf, char *end, unsigned long long num,
> @@ -1633,6 +1633,7 @@ static noinline_for_stack
> int format_decode(const char *fmt, struct printf_spec *spec)
> {
> const char *start = fmt;
> + char qualifier;
>
> /* we finished early by reading the field width */
> if (spec->type == FORMAT_TYPE_WIDTH) {
> @@ -1715,16 +1716,16 @@ precision:
>
> qualifier:
> /* get the conversion qualifier */
> - spec->qualifier = -1;
> + qualifier = 0;
> if (*fmt == 'h' || _tolower(*fmt) == 'l' ||
> _tolower(*fmt) == 'z' || *fmt == 't') {
> - spec->qualifier = *fmt++;
> - if (unlikely(spec->qualifier == *fmt)) {
> - if (spec->qualifier == 'l') {
> - spec->qualifier = 'L';
> + qualifier = *fmt++;
> + if (unlikely(qualifier == *fmt)) {
> + if (qualifier == 'l') {
> +
bnx2fc patch merge request
Hi James, the following patch for bnx2fc has been acked by QLogic but has never been included into the scsi branch. Can you please merge it? http://marc.info/?l=linux-scsi&m=140207797017410&w=2 Thanks, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] be2iscsi: Fix memory leak in beiscsi_alloc_mem()
In case of error, the memory allocated for phwi_ctrlr was not freed.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/be2iscsi/be_main.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 7a6dbfb..360aa88 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -2706,8 +2706,10 @@ static int beiscsi_alloc_mem(struct beiscsi_hba *phba)
phwi_ctrlr->wrb_context = kzalloc(sizeof(struct hwi_wrb_context) *
phba->params.cxns_per_ctrl,
GFP_KERNEL);
- if (!phwi_ctrlr->wrb_context)
+ if (!phwi_ctrlr->wrb_context) {
+ kfree(phba->phwi_ctrlr);
return -ENOMEM;
+ }
phba->init_mem = kcalloc(SE_MEM_MAX, sizeof(*mem_descr),
GFP_KERNEL);
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bnx2fc: reduce stack usage in __bnx2fc_enable
Hi, On 10/07/2015 03:11 PM, Arnd Bergmann wrote: > - memset(&npiv_tbl, 0, sizeof(npiv_tbl)); > - if (hba->cnic->get_fc_npiv_tbl(hba->cnic, &npiv_tbl)) > + npiv_tbl = kzalloc(sizeof(struct cnic_fc_npiv_tbl), GFP_KERNEL); > + if (!npiv_tbl) > goto done; > If kzalloc() fails perhaps the function should return -ENOMEM, not zero. Regards, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] st: trivial: remove form feed characters
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/st.c | 24
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index b37b9b0..7c4e518 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -226,7 +226,6 @@ static DEFINE_SPINLOCK(st_use_lock);
static DEFINE_IDR(st_index_idr);
-
#include "osst_detect.h"
#ifndef SIGS_FROM_OSST
#define SIGS_FROM_OSST \
@@ -305,7 +304,6 @@ static char * st_incompatible(struct scsi_device* SDp)
}
return NULL;
}
-
static inline char *tape_name(struct scsi_tape *tape)
{
@@ -877,7 +875,7 @@ static int flush_buffer(struct scsi_tape *STp, int
seek_next)
return result;
}
-
+
/* Set the mode parameters */
static int set_mode_densblk(struct scsi_tape * STp, struct st_modedef * STm)
{
@@ -952,7 +950,7 @@ static void reset_state(struct scsi_tape *STp)
STp->new_partition = STp->partition;
}
}
-
+
/* Test if the drive is ready. Returns either one of the codes below or a
negative system
error code. */
#define CHKRES_READY 0
@@ -1241,7 +1239,7 @@ static int check_tape(struct scsi_tape *STp, struct file
*filp)
}
- /* Open the device. Needs to take the BKL only because of incrementing the
SCSI host
+/* Open the device. Needs to take the BKL only because of incrementing the
SCSI host
module count. */
static int st_open(struct inode *inode, struct file *filp)
{
@@ -1334,7 +1332,6 @@ static int st_open(struct inode *inode, struct file *filp)
return retval;
}
-
/* Flush the tape buffer before close */
static int st_flush(struct file *filp, fl_owner_t id)
@@ -1470,7 +1467,7 @@ static int st_release(struct inode *inode, struct file
*filp)
return result;
}
-
+
/* The checks common to both reading and writing */
static ssize_t rw_checks(struct scsi_tape *STp, struct file *filp, size_t
count)
{
@@ -1889,7 +1886,7 @@ st_write(struct file *filp, const char __user *buf,
size_t count, loff_t * ppos)
return retval;
}
-
+
/* Read data from the tape. Returns zero in the normal case, one if the
eof status has changed, and the negative error code in case of a
fatal error. Otherwise updates the buffer and the eof state.
@@ -2085,7 +2082,6 @@ static long read_tape(struct scsi_tape *STp, long count,
}
return retval;
}
-
/* Read command */
static ssize_t
@@ -2233,7 +2229,6 @@ st_read(struct file *filp, char __user *buf, size_t
count, loff_t * ppos)
return retval;
}
-
DEB(
@@ -2447,7 +2442,7 @@ static int st_set_options(struct scsi_tape *STp, long
options)
return 0;
}
-
+
#define MODE_HEADER_LENGTH 4
/* Mode header and page byte offsets */
@@ -2665,7 +2660,7 @@ static int do_load_unload(struct scsi_tape *STp, struct
file *filp, int load_cod
return retval;
}
-
+
#if DEBUG
#define ST_DEB_FORWARD 0
#define ST_DEB_BACKWARD 1
@@ -3091,7 +3086,6 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned
int cmd_in, unsigned lon
return ioctl_result;
}
-
/* Get the tape position. If bt == 2, arg points into a kernel space mt_loc
structure. */
@@ -3283,7 +3277,7 @@ static int switch_partition(struct scsi_tape *STp)
STps->last_block_visited = 0;
return set_location(STp, STps->last_block_visited, STp->new_partition,
1);
}
-
+
/* Functions for reading and writing the medium partition mode page. */
#define PART_PAGE 0x11
@@ -3396,7 +3390,6 @@ static int partition_tape(struct scsi_tape *STp, int size)
return result;
}
-
/* The ioctl command */
@@ -3766,7 +3759,6 @@ static long st_compat_ioctl(struct file *file, unsigned
int cmd, unsigned long a
}
#endif
-
/* Try to allocate a new tape buffer. Calling function must not hold
dev_arr_lock. */
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] st: trivial: remove form feed characters
On 11/04/2015 10:04 PM, "Kai Mäkisara (Kolumbus)" wrote: > What’s the point? Is there an “official” rule that form feeds are not allowed > (to > put different things to different pages in printout)? I wrote it just because on some editors - and with thunderbird in particular - those characters make the code look weird. It's not a real issue so if you want to keep them it's ok for me. Regards, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] st: fix potential null pointer dereference.
If cdev_add() returns an error, the code calls
cdev_del() passing the STm->cdevs[rew] pointer as parameter;
the problem is that the pointer has not been initialized yet.
This patch fixes the problem by moving the STm->cdevs[rew] pointer
initialization before the call to cdev_add().
It also sets STm->devs[rew] = NULL if device_create() fails, just to be
sure we won't end up calling device_unregister() with an invalid pointer.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/st.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index e0a1e52..dff3bdb 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -4083,6 +4083,7 @@ static int create_one_cdev(struct scsi_tape *tape, int
mode, int rew)
}
cdev->owner = THIS_MODULE;
cdev->ops = &st_fops;
+ STm->cdevs[rew] = cdev;
error = cdev_add(cdev, cdev_devno, 1);
if (error) {
@@ -4091,7 +4092,6 @@ static int create_one_cdev(struct scsi_tape *tape, int
mode, int rew)
pr_err("st%d: Device not attached.\n", dev_num);
goto out_free;
}
- STm->cdevs[rew] = cdev;
i = mode << (4 - ST_NBR_MODE_BITS);
snprintf(name, 10, "%s%s%s", rew ? "n" : "",
@@ -4102,6 +4102,7 @@ static int create_one_cdev(struct scsi_tape *tape, int
mode, int rew)
if (IS_ERR(dev)) {
pr_err("st%d: device_create failed\n", dev_num);
error = PTR_ERR(dev);
+ STm->devs[rew] = NULL;
goto out_free;
}
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V2] st: fix potential null pointer dereference.
If cdev_add() returns an error, the code calls
cdev_del() passing the STm->cdevs[rew] pointer as parameter;
the problem is that the pointer has not been initialized yet.
This patch fixes the problem by moving the STm->cdevs[rew] pointer
initialization before the call to cdev_add().
It also sets STm->devs[rew] and STm->cdevs[rew] to NULL in
case of failure.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/st.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index e0a1e52..2e52295 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -4083,6 +4083,7 @@ static int create_one_cdev(struct scsi_tape *tape, int
mode, int rew)
}
cdev->owner = THIS_MODULE;
cdev->ops = &st_fops;
+ STm->cdevs[rew] = cdev;
error = cdev_add(cdev, cdev_devno, 1);
if (error) {
@@ -4091,7 +4092,6 @@ static int create_one_cdev(struct scsi_tape *tape, int
mode, int rew)
pr_err("st%d: Device not attached.\n", dev_num);
goto out_free;
}
- STm->cdevs[rew] = cdev;
i = mode << (4 - ST_NBR_MODE_BITS);
snprintf(name, 10, "%s%s%s", rew ? "n" : "",
@@ -4110,8 +4110,9 @@ static int create_one_cdev(struct scsi_tape *tape, int
mode, int rew)
return 0;
out_free:
cdev_del(STm->cdevs[rew]);
- STm->cdevs[rew] = NULL;
out:
+ STm->cdevs[rew] = NULL;
+ STm->devs[rew] = NULL;
return error;
}
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] st: fix potential null pointer dereference.
Please ignore this one, I sent a V2
On 11/18/2015 02:18 PM, Maurizio Lombardi wrote:
> If cdev_add() returns an error, the code calls
> cdev_del() passing the STm->cdevs[rew] pointer as parameter;
> the problem is that the pointer has not been initialized yet.
>
> This patch fixes the problem by moving the STm->cdevs[rew] pointer
> initialization before the call to cdev_add().
> It also sets STm->devs[rew] = NULL if device_create() fails, just to be
> sure we won't end up calling device_unregister() with an invalid pointer.
>
> Signed-off-by: Maurizio Lombardi
> ---
> drivers/scsi/st.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index e0a1e52..dff3bdb 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -4083,6 +4083,7 @@ static int create_one_cdev(struct scsi_tape *tape, int
> mode, int rew)
> }
> cdev->owner = THIS_MODULE;
> cdev->ops = &st_fops;
> + STm->cdevs[rew] = cdev;
>
> error = cdev_add(cdev, cdev_devno, 1);
> if (error) {
> @@ -4091,7 +4092,6 @@ static int create_one_cdev(struct scsi_tape *tape, int
> mode, int rew)
> pr_err("st%d: Device not attached.\n", dev_num);
> goto out_free;
> }
> - STm->cdevs[rew] = cdev;
>
> i = mode << (4 - ST_NBR_MODE_BITS);
> snprintf(name, 10, "%s%s%s", rew ? "n" : "",
> @@ -4102,6 +4102,7 @@ static int create_one_cdev(struct scsi_tape *tape, int
> mode, int rew)
> if (IS_ERR(dev)) {
> pr_err("st%d: device_create failed\n", dev_num);
> error = PTR_ERR(dev);
> + STm->devs[rew] = NULL;
> goto out_free;
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] st: fix corruption of the st_modedef structures in st_set_options()
When copying the st_modedef structures the devs pointers must be preserved
in the same way as with the cdevs pointers.
This fixes bug 70271: https://bugzilla.kernel.org/show_bug.cgi?id=70271
[ 135.037052] BUG: unable to handle kernel NULL pointer dereference at
0098
[ 135.045048] IP: [] kernfs_find_ns+0x21/0x150
[ 135.050999] PGD 220623067 PUD 222171067 PMD 0
[ 135.055593] Oops: [#1] SMP
[ 135.058938] Modules linked in: bnx2fc cnic uio fcoe libfcoe libfc 8021q mrp
scsi_transport_fc garp scsi_tgt stp llc binfmt_misc dm_round_robin dm_multipath
uinput iTCO_wdt iTCO_vendor_support microcode sg pcspkr serio_raw osst st(-)
i2c_i801 lpc_ich mfd_core e1000e ptp pps_core ipmi_si ipmi_msghandler video
tpm_infineon ext4(F) jbd2(F) mbcache(F) sd_mod(F) crc_t10dif(F)
crct10dif_common(F) sr_mod(F) cdrom(F) pata_acpi(F) ata_generic(F) ata_piix(F)
libata(F) mpt2sas(F) scsi_transport_sas(F) raid_class(F) ast(F) ttm(F)
drm_kms_helper(F) drm(F) i2c_algo_bit(F) sysimgblt(F) sysfillrect(F)
i2c_core(F) syscopyarea(F) dm_mirror(F) dm_region_hash(F) dm_log(F) dm_mod(F)
[ 135.119686] CPU: 2 PID: 2028 Comm: rmmod Tainted: GF
3.14.0-rc1-linux-mainline+ #14
[ 135.128453] Hardware name: wortmann To be filled by O.E.M./P8B-M Series,
BIOS 6103 12/06/2012
[ 135.137127] task: 880001de29d0 ti: 8802206e4000 task.ti:
8802206e4000
[ 135.144742] RIP: 0010:[] []
kernfs_find_ns+0x21/0x150
[ 135.153148] RSP: 0018:8802206e5c98 EFLAGS: 00010282
[ 135.158562] RAX: 880001de29d0 RBX: RCX: 0006
[ 135.165814] RDX: RSI: 817627e0 RDI:
[ 135.173040] RBP: 8802206e5cc8 R08: R09: 0001
[ 135.180303] R10: R11: 0001 R12: 817627e0
[ 135.187554] R13: R14: R15: 0001
[ 135.194774] FS: 7f817c720700() GS:88022720()
knlGS:
[ 135.202995] CS: 0010 DS: ES: CR0: 80050033
[ 135.208878] CR2: 0098 CR3: 0002219b CR4: 000407e0
[ 135.216139] Stack:
[ 135.218185] 81af63a0 817627e0
[ 135.225783] 0001 8802206e5cf8
812af8de
[ 135.233347] 880226801900 81b43320
880221a7c1c0
[ 135.240972] Call Trace:
[ 135.243463] [] kernfs_find_and_get_ns+0x3e/0x70
[ 135.249743] [] sysfs_unmerge_group+0x1d/0x60
[ 135.255716] [] pm_qos_sysfs_remove_latency+0x19/0x20
[ 135.262430] [] dev_pm_qos_constraints_destroy+0x31/0x1e0
[ 135.269500] [] dpm_sysfs_remove+0x16/0x50
[ 135.275263] [] device_del+0x47/0x1e0
[ 135.280554] [] device_unregister+0x22/0x60
[ 135.286406] [] remove_cdevs+0x4d/0x90 [st]
[ 135.292247] [] st_remove+0x3f/0xb0 [st]
[ 135.297851] [] __device_release_driver+0x7f/0xf0
[ 135.304237] [] driver_detach+0xd8/0xe0
[ 135.309722] [] bus_remove_driver+0x5c/0xd0
[ 135.315553] [] driver_unregister+0x30/0x70
[ 135.321366] [] exit_st+0x5c/0x868 [st]
[ 135.326861] [] SyS_delete_module+0x19a/0x1f0
[ 135.332891] [] ? trace_hardirqs_on+0xd/0x10
[ 135.338811] [] ? __audit_syscall_entry+0x94/0x100
[ 135.345282] [] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 135.351806] [] system_call_fastpath+0x16/0x1b
[ 135.357859] Code: ff eb e3 0f 1f 80 00 00 00 00 55 48 89 e5 48 83 ec 30 48
89 5d d8 4c 89 65 e0 4c 89 6d e8 4c 89 75 f0 4c 89 7d f8 66 66 66 66 90 <44> 0f
b7 bf 98 00 00 00 8b 05 71 6d 87 00 48 89 fb 49 89 f4 49
[ 135.378282] RIP [] kernfs_find_ns+0x21/0x150
[ 135.384355] RSP
[ 135.387881] CR2: 0098
[ 135.391298] ---[ end trace 1968409221ddb3c8 ]---
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/st.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index a1d6986..afc834e 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -2198,12 +2198,19 @@ static int st_set_options(struct scsi_tape *STp, long
options)
struct st_modedef *STm;
char *name = tape_name(STp);
struct cdev *cd0, *cd1;
+ struct device *d0, *d1;
STm = &(STp->modes[STp->current_mode]);
if (!STm->defined) {
- cd0 = STm->cdevs[0]; cd1 = STm->cdevs[1];
+ cd0 = STm->cdevs[0];
+ cd1 = STm->cdevs[1];
+ d0 = STm->devs[0];
+ d1 = STm->devs[1];
memcpy(STm, &(STp->modes[0]), sizeof(struct st_modedef));
- STm->cdevs[0] = cd0; STm->cdevs[1] = cd1;
+ STm->cdevs[0] = cd0;
+ STm->cdevs[1] = cd1;
+ STm->devs[0] = d0;
+ STm->devs[1] = d1;
modes_defined = 1;
DEBC(printk(ST_DEB_MSG
"%s: Initialized mode %d definitio
Re: [PATCH 06/16] scsi_dh_alua: use local buffer for VPD inquiry
Hi,
On 01/31/2014 10:29 AM, Hannes Reinecke wrote:
> static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
> {
> + unsigned char *buff;
> + unsigned char bufflen = 36;
> int len, timeout = ALUA_FAILOVER_TIMEOUT;
[...]
>+ len = (buff[2] << 8) + buff[3] + 4;
>+ if (len > bufflen) {
[...]
>+ bufflen = len;
just a nit: is it safe to use char as the type of bufflen? Isn't better
to declare it as int just in case len is > than 255 ?
Regards,
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] st: use dev_printk() to avoid linebreaks in kernel messages.
This patch converts the st driver to use dev_printk() instead of printk(),
every st device has a reference to the underlying scsi device.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/st.c | 613 +++---
1 file changed, 349 insertions(+), 264 deletions(-)
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index a1d6986..b2138cb 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -171,6 +171,9 @@ static int debugging = DEBUG;
24 bits) */
#define SET_DENS_AND_BLK 0x10001
+#define stdev_printk(prefix, stdev, fmt, a...) \
+ sdev_printk(prefix, (stdev)->device, fmt, ##a)
+
static int st_fixed_buffer_size = ST_FIXED_BUFFER_SIZE;
static int st_max_sg_segs = ST_MAX_SG;
@@ -179,8 +182,8 @@ static int modes_defined;
static int enlarge_buffer(struct st_buffer *, int, int);
static void clear_buffer(struct st_buffer *);
static void normalize_buffer(struct st_buffer *);
-static int append_to_buffer(const char __user *, struct st_buffer *, int);
-static int from_buffer(struct st_buffer *, char __user *, int);
+static int append_to_buffer(const char __user *, struct st_buffer *, int, char
**errmsg);
+static int from_buffer(struct st_buffer *, char __user *, int, char **errmsg);
static void move_buffer_data(struct st_buffer *, int);
static int sgl_map_user_pages(struct st_buffer *, const unsigned int,
@@ -360,19 +363,20 @@ static int st_chk_result(struct scsi_tape *STp, struct
st_request * SRpnt)
DEB(
if (debugging) {
-printk(ST_DEB_MSG "%s: Error: %x, cmd: %x %x %x %x %x %x\n",
- name, result,
- SRpnt->cmd[0], SRpnt->cmd[1], SRpnt->cmd[2],
- SRpnt->cmd[3], SRpnt->cmd[4], SRpnt->cmd[5]);
+stdev_printk(ST_DEB_MSG, STp,
+"%s: Error: %x, cmd: %x %x %x %x %x %x\n",
+name, result,
+SRpnt->cmd[0], SRpnt->cmd[1], SRpnt->cmd[2],
+SRpnt->cmd[3], SRpnt->cmd[4], SRpnt->cmd[5]);
if (cmdstatp->have_sense)
__scsi_print_sense(name, SRpnt->sense,
SCSI_SENSE_BUFFERSIZE);
} ) /* end DEB */
if (!debugging) { /* Abnormal conditions for tape */
if (!cmdstatp->have_sense)
- printk(KERN_WARNING
- "%s: Error %x (driver bt 0x%x, host bt 0x%x).\n",
- name, result, driver_byte(result),
- host_byte(result));
+ stdev_printk(KERN_WARNING, STp,
+"%s: Error %x (driver bt 0x%x, host bt
0x%x).\n",
+name, result, driver_byte(result),
+host_byte(result));
else if (cmdstatp->have_sense &&
scode != NO_SENSE &&
scode != RECOVERED_ERROR &&
@@ -419,8 +423,9 @@ static int st_chk_result(struct scsi_tape *STp, struct
st_request * SRpnt)
stp = "write";
else
stp = "ioctl";
- printk(ST_DEB_MSG "%s: Recovered %s error (%d).\n",
name, stp,
- STp->recover_count);
+ stdev_printk(ST_DEB_MSG, STp,
+"%s: Recovered %s error (%d).\n", name,
stp,
+STp->recover_count);
} ) /* end DEB */
if (cmdstatp->flags == 0)
@@ -437,8 +442,9 @@ static struct st_request *st_allocate_request(struct
scsi_tape *stp)
if (streq)
streq->stp = stp;
else {
- DEBC(printk(KERN_ERR "%s: Can't get SCSI request.\n",
- tape_name(stp)););
+ DEBC(stdev_printk(KERN_ERR, stp,
+ "%s: Can't get SCSI request.\n",
+ tape_name(stp)););
if (signal_pending(current))
stp->buffer->syscall_result = -EINTR;
else
@@ -525,8 +531,9 @@ st_do_scsi(struct st_request * SRpnt, struct scsi_tape *
STp, unsigned char *cmd
/* if async, make sure there's no command outstanding */
if (!do_wait && ((STp->buffer)->last_SRpnt)) {
- printk(KERN_ERR "%s: Async command already active.\n",
- tape_name(STp));
+ stdev_printk(KERN_ERR, STp,
+"%s: Async command already active.\n",
+tape_name(STp));
if (signal_pendi
Re: [PATCH] st: use dev_printk() to avoid linebreaks in kernel messages.
Hi Hannes, On 02/19/2014 10:09 AM, Hannes Reinecke wrote: > Thanks for doing this. > > I've actually a full patchset queued, converting all the stray > 'printk' invocations in the SCSI stack to dev_printk() and friends. > And moving to a per-device scsi_log_level :-) > > So far I haven't posted them as I'm still waiting for feedback > from my EVPD patchset. > > But I can rebase them on top of that if you want to have a look at them. > Are you talking about this patchset? "scsi: avoid linebreaks in syslog output" http://marc.info/?l=linux-scsi&m=135003083808290&w=2 If you have a more complete and up-to-date patchset in your queue it would be interesting to have a look at it. Regards, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] bnx2fc: remove unused variable hash_table_size
hash_table_size is not used by the bnx2fc_free_hash_table() function.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_hwi.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index 46a3765..261af2a 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -1966,12 +1966,9 @@ static void bnx2fc_free_hash_table(struct bnx2fc_hba
*hba)
{
int i;
int segment_count;
- int hash_table_size;
u32 *pbl;
segment_count = hba->hash_tbl_segment_count;
- hash_table_size = BNX2FC_NUM_MAX_SESS * BNX2FC_MAX_ROWS_IN_HASH_TBL *
- sizeof(struct fcoe_hash_table_entry);
pbl = hba->hash_tbl_pbl;
for (i = 0; i < segment_count; ++i) {
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] bnx2fc: fix memory leak and potential NULL pointer dereference.
If bnx2fc_allocate_hash_table() for some reasons fails, it is possible that the
hash_tbl_segments or the hash_tbl_pbl pointers are NULL.
In this case bnx2fc_free_hash_table() will panic the system.
this patch also fixes a memory leak, the hash_tbl_segments pointer was never
freed.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_hwi.c | 34 --
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index 261af2a..f83bae4 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -1968,21 +1968,27 @@ static void bnx2fc_free_hash_table(struct bnx2fc_hba
*hba)
int segment_count;
u32 *pbl;
- segment_count = hba->hash_tbl_segment_count;
-
- pbl = hba->hash_tbl_pbl;
- for (i = 0; i < segment_count; ++i) {
- dma_addr_t dma_address;
-
- dma_address = le32_to_cpu(*pbl);
- ++pbl;
- dma_address += ((u64)le32_to_cpu(*pbl)) << 32;
- ++pbl;
- dma_free_coherent(&hba->pcidev->dev,
- BNX2FC_HASH_TBL_CHUNK_SIZE,
- hba->hash_tbl_segments[i],
- dma_address);
+ if (hba->hash_tbl_segments) {
+
+ pbl = hba->hash_tbl_pbl;
+ if (pbl) {
+ segment_count = hba->hash_tbl_segment_count;
+ for (i = 0; i < segment_count; ++i) {
+ dma_addr_t dma_address;
+
+ dma_address = le32_to_cpu(*pbl);
+ ++pbl;
+ dma_address += ((u64)le32_to_cpu(*pbl)) << 32;
+ ++pbl;
+ dma_free_coherent(&hba->pcidev->dev,
+ BNX2FC_HASH_TBL_CHUNK_SIZE,
+ hba->hash_tbl_segments[i],
+ dma_address);
+ }
+ }
+ kfree(hba->hash_tbl_segments);
+ hba->hash_tbl_segments = NULL;
}
if (hba->hash_tbl_pbl) {
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] bnx2fc: fix memory leak and potential NULL pointer dereferences
The first patch removes a unused variable from the bnx2fc_free_hash_table() function, it was already posted on the linux-scsi mailing list: http://www.spinics.net/lists/linux-scsi/msg72740.html but has not been picked up yet. The second patch fixes a memory leak and some NULL pointer dereferences that may happen if bnx2fc_allocate_hash_table() fails. Maurizio Lombardi (2): bnx2fc: remove unused variable hash_table_size bnx2fc: fix memory leak and potential NULL pointer dereference. drivers/scsi/bnx2fc/bnx2fc_hwi.c | 36 ++-- 1 file changed, 18 insertions(+), 18 deletions(-) -- Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] bnx2fc: remove unused variable hash_table_size
hash_table_size is not used by the bnx2fc_free_hash_table() function.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_hwi.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index 46a3765..261af2a 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -1966,12 +1966,9 @@ static void bnx2fc_free_hash_table(struct bnx2fc_hba
*hba)
{
int i;
int segment_count;
- int hash_table_size;
u32 *pbl;
segment_count = hba->hash_tbl_segment_count;
- hash_table_size = BNX2FC_NUM_MAX_SESS * BNX2FC_MAX_ROWS_IN_HASH_TBL *
- sizeof(struct fcoe_hash_table_entry);
pbl = hba->hash_tbl_pbl;
for (i = 0; i < segment_count; ++i) {
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] bnx2fc: fix memory leak and potential NULL pointer dereference.
Hi Eddie,
On Fri, Mar 07, 2014 at 11:21:45AM -0800, Eddie Wai wrote:
> Hello Maurizio,
>
> Thanks for looking into this. The patch looks good, but I would like to
> extend it with the following allocation failure portion to completely
> remove the memory leak. Please incorporate this into your next
> submission.
Thanks for the patch you sent,
maybe it is even better if I modify bnx2fc_allocate_hash_table() in such a way
that, in case of error, it frees all the memory it has allocated.
Note that your patch does not free the table if
"hba->hash_tbl_pbl = dma_alloc_coherent(...)" fails (line 2058).
It just requires a very little change to your patch, I'll send it tomorrow.
Regards,
Maurizio Lombardi
>
> Thanks,
> Eddie
>
> @@ -2020,7 +2026,7 @@ static int bnx2fc_allocate_hash_table(struct
> bnx2fc_hba *hba)
> dma_segment_array = kzalloc(dma_segment_array_size, GFP_KERNEL);
> if (!dma_segment_array) {
> printk(KERN_ERR PFX "hash table pointers (dma) alloc
> failed\n");
> - return -ENOMEM;
> + goto free_hash_tbl_seg;
> }
>
> for (i = 0; i < segment_count; ++i) {
> @@ -2039,7 +2045,7 @@ static int bnx2fc_allocate_hash_table(struct
> bnx2fc_hba *hba)
> hba->hash_tbl_segments[i] = NULL;
> }
> kfree(dma_segment_array);
> - return -ENOMEM;
> + goto free_hash_tbl_seg;
> }
> memset(hba->hash_tbl_segments[i], 0,
>BNX2FC_HASH_TBL_CHUNK_SIZE);
> @@ -2077,6 +2083,11 @@ static int bnx2fc_allocate_hash_table(struct
> bnx2fc_hba *hba)
> }
> kfree(dma_segment_array);
> return 0;
> +
> +free_hash_tbl_seg:
> + kfree(hba->hash_tbl_segments);
> + hba->hash_tbl_segments = NULL;
> + return -ENOMEM;
> }
>
>
> On Fri, 2014-03-07 at 15:37 +0100, Maurizio Lombardi wrote:
> > If bnx2fc_allocate_hash_table() for some reasons fails, it is possible that
> > the
> > hash_tbl_segments or the hash_tbl_pbl pointers are NULL.
> > In this case bnx2fc_free_hash_table() will panic the system.
> >
> > this patch also fixes a memory leak, the hash_tbl_segments pointer was never
> > freed.
> >
> > Signed-off-by: Maurizio Lombardi
> > ---
> > drivers/scsi/bnx2fc/bnx2fc_hwi.c | 34 --
> > 1 file changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > index 261af2a..f83bae4 100644
> > --- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > +++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> > @@ -1968,21 +1968,27 @@ static void bnx2fc_free_hash_table(struct
> > bnx2fc_hba *hba)
> > int segment_count;
> > u32 *pbl;
> >
> > - segment_count = hba->hash_tbl_segment_count;
> > -
> > - pbl = hba->hash_tbl_pbl;
> > - for (i = 0; i < segment_count; ++i) {
> > - dma_addr_t dma_address;
> > -
> > - dma_address = le32_to_cpu(*pbl);
> > - ++pbl;
> > - dma_address += ((u64)le32_to_cpu(*pbl)) << 32;
> > - ++pbl;
> > - dma_free_coherent(&hba->pcidev->dev,
> > - BNX2FC_HASH_TBL_CHUNK_SIZE,
> > - hba->hash_tbl_segments[i],
> > - dma_address);
> > + if (hba->hash_tbl_segments) {
> > +
> > + pbl = hba->hash_tbl_pbl;
> > + if (pbl) {
> > + segment_count = hba->hash_tbl_segment_count;
> > + for (i = 0; i < segment_count; ++i) {
> > + dma_addr_t dma_address;
> > +
> > + dma_address = le32_to_cpu(*pbl);
> > + ++pbl;
> > + dma_address += ((u64)le32_to_cpu(*pbl)) << 32;
> > + ++pbl;
> > + dma_free_coherent(&hba->pcidev->dev,
> > + BNX2FC_HASH_TBL_CHUNK_SIZE,
> > + hba->hash_tbl_segments[i],
> > + dma_address);
> > + }
> > + }
> >
> > + kfree(hba->hash_tbl_segments);
> > + hba->hash_tbl_segments = NULL;
> > }
> >
> > if (hba->hash_tbl_pbl) {
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/3] bnx2fc: fix memory leaks and NULL pointer dereferences
PATCH 1/3 removes a unused variable from the bnx2fc_free_hash_table() function, it was already posted on the linux-scsi mailing list: http://www.spinics.net/lists/linux-scsi/msg72740.html but has not been picked up yet. PATCH 2/3 fixes a memory leak and some NULL pointer dereferences in the bnx2fc_free_hash_table() function that may happen if bnx2fc_allocate_hash_table() fails. PATCH 3/3 fixes a memory leak in the bnx2fc_allocate_hash_table() function. Maurizio Lombardi (3): bnx2fc: remove unused variable hash_table_size bnx2fc: fix memory leak and potential NULL pointer dereference. bnx2fc: fix memory leak in bnx2fc_allocate_hash_table() drivers/scsi/bnx2fc/bnx2fc_hwi.c | 64 +++- 1 file changed, 37 insertions(+), 27 deletions(-) -- Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] bnx2fc: fix memory leak in bnx2fc_allocate_hash_table()
In case of error, the bnx2fc_allocate_hash_table() didn't free
all the memory it allocated.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_hwi.c | 31 +++
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index f83bae4..5a7865f 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -2026,7 +2026,7 @@ static int bnx2fc_allocate_hash_table(struct bnx2fc_hba
*hba)
dma_segment_array = kzalloc(dma_segment_array_size, GFP_KERNEL);
if (!dma_segment_array) {
printk(KERN_ERR PFX "hash table pointers (dma) alloc failed\n");
- return -ENOMEM;
+ goto fail;
}
for (i = 0; i < segment_count; ++i) {
@@ -2037,15 +2037,7 @@ static int bnx2fc_allocate_hash_table(struct bnx2fc_hba
*hba)
GFP_KERNEL);
if (!hba->hash_tbl_segments[i]) {
printk(KERN_ERR PFX "hash segment alloc failed\n");
- while (--i >= 0) {
- dma_free_coherent(&hba->pcidev->dev,
- BNX2FC_HASH_TBL_CHUNK_SIZE,
- hba->hash_tbl_segments[i],
- dma_segment_array[i]);
- hba->hash_tbl_segments[i] = NULL;
- }
- kfree(dma_segment_array);
- return -ENOMEM;
+ goto fail;
}
memset(hba->hash_tbl_segments[i], 0,
BNX2FC_HASH_TBL_CHUNK_SIZE);
@@ -2057,8 +2049,7 @@ static int bnx2fc_allocate_hash_table(struct bnx2fc_hba
*hba)
GFP_KERNEL);
if (!hba->hash_tbl_pbl) {
printk(KERN_ERR PFX "hash table pbl alloc failed\n");
- kfree(dma_segment_array);
- return -ENOMEM;
+ goto fail;
}
memset(hba->hash_tbl_pbl, 0, PAGE_SIZE);
@@ -2083,6 +2074,22 @@ static int bnx2fc_allocate_hash_table(struct bnx2fc_hba
*hba)
}
kfree(dma_segment_array);
return 0;
+
+fail:
+ for (i = 0; i < segment_count; ++i) {
+ if (hba->hash_tbl_segments[i])
+ dma_free_coherent(&hba->pcidev->dev,
+ BNX2FC_HASH_TBL_CHUNK_SIZE,
+ hba->hash_tbl_segments[i],
+ dma_segment_array[i]);
+ }
+
+ if (dma_segment_array)
+ kfree(dma_segment_array);
+
+ kfree(hba->hash_tbl_segments);
+ hba->hash_tbl_segments = NULL;
+ return -ENOMEM;
}
/**
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] bnx2fc: remove unused variable hash_table_size
hash_table_size is not used by the bnx2fc_free_hash_table() function.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_hwi.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index 46a3765..261af2a 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -1966,12 +1966,9 @@ static void bnx2fc_free_hash_table(struct bnx2fc_hba
*hba)
{
int i;
int segment_count;
- int hash_table_size;
u32 *pbl;
segment_count = hba->hash_tbl_segment_count;
- hash_table_size = BNX2FC_NUM_MAX_SESS * BNX2FC_MAX_ROWS_IN_HASH_TBL *
- sizeof(struct fcoe_hash_table_entry);
pbl = hba->hash_tbl_pbl;
for (i = 0; i < segment_count; ++i) {
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] bnx2fc: fix memory leak and potential NULL pointer dereference.
If bnx2fc_allocate_hash_table() for some reasons fails, it is possible that the
hash_tbl_segments or the hash_tbl_pbl pointers are NULL.
In this case bnx2fc_free_hash_table() will panic the system.
this patch also fixes a memory leak, the hash_tbl_segments pointer was never
freed.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_hwi.c | 34 --
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index 261af2a..f83bae4 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -1968,21 +1968,27 @@ static void bnx2fc_free_hash_table(struct bnx2fc_hba
*hba)
int segment_count;
u32 *pbl;
- segment_count = hba->hash_tbl_segment_count;
-
- pbl = hba->hash_tbl_pbl;
- for (i = 0; i < segment_count; ++i) {
- dma_addr_t dma_address;
-
- dma_address = le32_to_cpu(*pbl);
- ++pbl;
- dma_address += ((u64)le32_to_cpu(*pbl)) << 32;
- ++pbl;
- dma_free_coherent(&hba->pcidev->dev,
- BNX2FC_HASH_TBL_CHUNK_SIZE,
- hba->hash_tbl_segments[i],
- dma_address);
+ if (hba->hash_tbl_segments) {
+
+ pbl = hba->hash_tbl_pbl;
+ if (pbl) {
+ segment_count = hba->hash_tbl_segment_count;
+ for (i = 0; i < segment_count; ++i) {
+ dma_addr_t dma_address;
+
+ dma_address = le32_to_cpu(*pbl);
+ ++pbl;
+ dma_address += ((u64)le32_to_cpu(*pbl)) << 32;
+ ++pbl;
+ dma_free_coherent(&hba->pcidev->dev,
+ BNX2FC_HASH_TBL_CHUNK_SIZE,
+ hba->hash_tbl_segments[i],
+ dma_address);
+ }
+ }
+ kfree(hba->hash_tbl_segments);
+ hba->hash_tbl_segments = NULL;
}
if (hba->hash_tbl_pbl) {
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] scsi: fix potential NULL pointer dereference.
The scsi_get_command() function returns NULL if it fails to allocate the scsi_cmnd structure. If this happens, a NULL pointer will be dereferenced. Signed-off-by: Maurizio Lombardi --- drivers/scsi/scsi_error.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index 78b004d..4021849 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -2289,6 +2289,9 @@ scsi_reset_provider(struct scsi_device *dev, int flag) return FAILED; scmd = scsi_get_command(dev, GFP_KERNEL); + if (!scmd) + return FAILED; + blk_rq_init(NULL, &req); scmd->request = &req; -- Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: fix potential NULL pointer dereference.
On Tue, Mar 25, 2014 at 06:13:06AM -0700, Christoph Hellwig wrote: > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > > index 78b004d..4021849 100644 > > --- a/drivers/scsi/scsi_error.c > > +++ b/drivers/scsi/scsi_error.c > > @@ -2289,6 +2289,9 @@ scsi_reset_provider(struct scsi_device *dev, int flag) > > return FAILED; > > > > scmd = scsi_get_command(dev, GFP_KERNEL); > > + if (!scmd) > > + return FAILED; > > + > > Fails to call scsi_autopm_put_host, or in Jame's latest tree that has > my changes put_device on the sdev gendev. I sent a correct version > earlier, but there has been very little enthusiasm for it. > Yes you are right, it should call scsi_autopm_put_host before returning. So if you already have a correct version for it then I drop this patch. Maurizio. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] bnx2fc: fix memory leak in bnx2fc_allocate_hash_table()
In case of error, the bnx2fc_allocate_hash_table() didn't free
all the memory it allocated.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_hwi.c | 31 +++
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index f83bae4..512aed3 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -2026,7 +2026,7 @@ static int bnx2fc_allocate_hash_table(struct bnx2fc_hba
*hba)
dma_segment_array = kzalloc(dma_segment_array_size, GFP_KERNEL);
if (!dma_segment_array) {
printk(KERN_ERR PFX "hash table pointers (dma) alloc failed\n");
- return -ENOMEM;
+ goto cleanup_ht;
}
for (i = 0; i < segment_count; ++i) {
@@ -2037,15 +2037,7 @@ static int bnx2fc_allocate_hash_table(struct bnx2fc_hba
*hba)
GFP_KERNEL);
if (!hba->hash_tbl_segments[i]) {
printk(KERN_ERR PFX "hash segment alloc failed\n");
- while (--i >= 0) {
- dma_free_coherent(&hba->pcidev->dev,
- BNX2FC_HASH_TBL_CHUNK_SIZE,
- hba->hash_tbl_segments[i],
- dma_segment_array[i]);
- hba->hash_tbl_segments[i] = NULL;
- }
- kfree(dma_segment_array);
- return -ENOMEM;
+ goto cleanup_dma;
}
memset(hba->hash_tbl_segments[i], 0,
BNX2FC_HASH_TBL_CHUNK_SIZE);
@@ -2057,8 +2049,7 @@ static int bnx2fc_allocate_hash_table(struct bnx2fc_hba
*hba)
GFP_KERNEL);
if (!hba->hash_tbl_pbl) {
printk(KERN_ERR PFX "hash table pbl alloc failed\n");
- kfree(dma_segment_array);
- return -ENOMEM;
+ goto cleanup_dma;
}
memset(hba->hash_tbl_pbl, 0, PAGE_SIZE);
@@ -2083,6 +2074,22 @@ static int bnx2fc_allocate_hash_table(struct bnx2fc_hba
*hba)
}
kfree(dma_segment_array);
return 0;
+
+cleanup_dma:
+ for (i = 0; i < segment_count; ++i) {
+ if (hba->hash_tbl_segments[i])
+ dma_free_coherent(&hba->pcidev->dev,
+ BNX2FC_HASH_TBL_CHUNK_SIZE,
+ hba->hash_tbl_segments[i],
+ dma_segment_array[i]);
+ }
+
+ kfree(dma_segment_array);
+
+cleanup_ht:
+ kfree(hba->hash_tbl_segments);
+ hba->hash_tbl_segments = NULL;
+ return -ENOMEM;
}
/**
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] bnx2fc: remove unused variable hash_table_size
hash_table_size is not used by the bnx2fc_free_hash_table() function.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_hwi.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index 46a3765..261af2a 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -1966,12 +1966,9 @@ static void bnx2fc_free_hash_table(struct bnx2fc_hba
*hba)
{
int i;
int segment_count;
- int hash_table_size;
u32 *pbl;
segment_count = hba->hash_tbl_segment_count;
- hash_table_size = BNX2FC_NUM_MAX_SESS * BNX2FC_MAX_ROWS_IN_HASH_TBL *
- sizeof(struct fcoe_hash_table_entry);
pbl = hba->hash_tbl_pbl;
for (i = 0; i < segment_count; ++i) {
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] bnx2fc: fix memory leak and potential NULL pointer dereference.
If bnx2fc_allocate_hash_table() for some reasons fails, it is possible that the
hash_tbl_segments or the hash_tbl_pbl pointers are NULL.
In this case bnx2fc_free_hash_table() will panic the system.
this patch also fixes a memory leak, the hash_tbl_segments pointer was never
freed.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_hwi.c | 34 --
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index 261af2a..f83bae4 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -1968,21 +1968,27 @@ static void bnx2fc_free_hash_table(struct bnx2fc_hba
*hba)
int segment_count;
u32 *pbl;
- segment_count = hba->hash_tbl_segment_count;
-
- pbl = hba->hash_tbl_pbl;
- for (i = 0; i < segment_count; ++i) {
- dma_addr_t dma_address;
-
- dma_address = le32_to_cpu(*pbl);
- ++pbl;
- dma_address += ((u64)le32_to_cpu(*pbl)) << 32;
- ++pbl;
- dma_free_coherent(&hba->pcidev->dev,
- BNX2FC_HASH_TBL_CHUNK_SIZE,
- hba->hash_tbl_segments[i],
- dma_address);
+ if (hba->hash_tbl_segments) {
+
+ pbl = hba->hash_tbl_pbl;
+ if (pbl) {
+ segment_count = hba->hash_tbl_segment_count;
+ for (i = 0; i < segment_count; ++i) {
+ dma_addr_t dma_address;
+
+ dma_address = le32_to_cpu(*pbl);
+ ++pbl;
+ dma_address += ((u64)le32_to_cpu(*pbl)) << 32;
+ ++pbl;
+ dma_free_coherent(&hba->pcidev->dev,
+ BNX2FC_HASH_TBL_CHUNK_SIZE,
+ hba->hash_tbl_segments[i],
+ dma_address);
+ }
+ }
+ kfree(hba->hash_tbl_segments);
+ hba->hash_tbl_segments = NULL;
}
if (hba->hash_tbl_pbl) {
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/3] bnx2fc: fix memory leaks and NULL pointer dereferences
PATCH 1/3 removes a unused variable from the bnx2fc_free_hash_table() function, PATCH 2/3 fixes a memory leak and some NULL pointer dereferences in the bnx2fc_free_hash_table() function that may happen if bnx2fc_allocate_hash_table() fails. PATCH 3/3 fixes a memory leak in the bnx2fc_allocate_hash_table() function. Maurizio Lombardi (3): bnx2fc: remove unused variable hash_table_size bnx2fc: fix memory leak and potential NULL pointer dereference. bnx2fc: fix memory leak in bnx2fc_allocate_hash_table() drivers/scsi/bnx2fc/bnx2fc_hwi.c | 64 +++- 1 file changed, 37 insertions(+), 27 deletions(-) -- Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/device_handler/scsi_dh_alua.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c
b/drivers/scsi/device_handler/scsi_dh_alua.c
index 5248c88..1903ae5 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -25,6 +25,7 @@
#include
#include
#include
+#include
#define ALUA_DH_NAME "alua"
#define ALUA_DH_VER "1.3"
@@ -163,9 +164,12 @@ static int submit_vpd_inquiry(struct scsi_device *sdev,
struct alua_dh_data *h)
err = blk_execute_rq(rq->q, NULL, rq, 1);
if (err == -EIO) {
- sdev_printk(KERN_INFO, sdev,
- "%s: evpd inquiry failed with %x\n",
- ALUA_DH_NAME, rq->errors);
+ sdev_printk(KERN_INFO, sdev, "%s: evpd inquiry failed\n",
+ ALUA_DH_NAME);
+ scsi_show_result(rq->errors);
+ if (driver_byte(rq->errors) & DRIVER_SENSE)
+ __scsi_print_sense("alua vpd_inquiry", rq->sense,
+ rq->sense_len);
h->senselen = rq->sense_len;
err = SCSI_DH_IO;
}
@@ -206,9 +210,11 @@ static unsigned submit_rtpg(struct scsi_device *sdev,
struct alua_dh_data *h,
err = blk_execute_rq(rq->q, NULL, rq, 1);
if (err == -EIO) {
- sdev_printk(KERN_INFO, sdev,
- "%s: rtpg failed with %x\n",
- ALUA_DH_NAME, rq->errors);
+ sdev_printk(KERN_INFO, sdev, "%s: rtpg failed\n", ALUA_DH_NAME);
+ scsi_show_result(rq->errors);
+ if (driver_byte(rq->errors) & DRIVER_SENSE)
+ __scsi_print_sense("alua submit_rtpg", rq->sense,
+ rq->sense_len);
h->senselen = rq->sense_len;
err = SCSI_DH_IO;
}
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.
Hi, On Fri, Apr 25, 2014 at 03:12:19PM +0200, Christoph Hellwig wrote: > > Looks useful, but why do we have basically two copies of the same > code for different commands? Looking at them in detail they mostly look > like copies of scsi_execute/scsi_execute_req_flags and should be switched > to that. Yes I think you are right, I'm trying to write a patch to get rid of all this duplicated code. I'll publish a patchset next week. Thanks, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.
Hi Christoph,
On Fri, Apr 25, 2014 at 03:12:19PM +0200, Christoph Hellwig wrote:
>
> Looks useful, but why do we have basically two copies of the same
> code for different commands? Looking at them in detail they mostly look
> like copies of scsi_execute/scsi_execute_req_flags and should be switched
> to that.
I've tried to write a patch but unfortunately there are some problems with that.
For example, look at the submit_rtpg() function:
If the blk_execute_rq() function returns an error, the h->sense buffer
is updated.
rq->sense = h->sense; <-
memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
rq->sense_len = h->senselen = 0;
err = blk_execute_rq(rq->q, NULL, rq, 1);
if (err == -EIO) {
sdev_printk(KERN_INFO, sdev,
"%s: rtpg failed with %x\n",
ALUA_DH_NAME, rq->errors);
h->senselen = rq->sense_len; <
err = SCSI_DH_IO;
}
If I convert submit_rtpg() to make use of scsi_execute() then I'll not be
able to read the updated sense buffer.
scsi_execute() accepts a pointer to the sense buffer as parameter, but
it does not update its content in case blk_execute_rq() returns an error.
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] bio: modify __bio_add_page() to accept pages that don't start a new segment
The original behaviour is to refuse to add a new page if the maximum number
of segments has been reached, regardless of the fact the page we are
going to add can be merged into the last segment or not.
Unfortunately, when the system runs under heavy memory fragmentation conditions,
a driver may try to add multiple pages to the last segment.
The original code won't accept them and EBUSY will be reported to
userspace.
This patch modifies the function so it refuses to add a page
only in case the latter starts a new segment and the maximum number
of segments has already been reached.
The bug can be easily reproduced with the st driver:
1) set CONFIG_SCSI_MPT2SAS_MAX_SGE or CONFIG_SCSI_MPT3SAS_MAX_SGE to 16
2) modprobe st buffer_kbs=1024
3) #dd if=/dev/zero of=/dev/st0 bs=1M count=10
dd: error writing ‘/dev/st0’: Device or resource busy
Signed-off-by: Maurizio Lombardi
---
fs/bio.c | 50 --
1 file changed, 28 insertions(+), 22 deletions(-)
diff --git a/fs/bio.c b/fs/bio.c
index 6f0362b..9a3a0b1 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -750,29 +750,31 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
return 0;
/*
-* we might lose a segment or two here, but rather that than
-* make this too complex.
+* setup the new entry, we might clear it again later if we
+* cannot add the page
+*/
+ bvec = &bio->bi_io_vec[bio->bi_vcnt];
+ bvec->bv_page = page;
+ bvec->bv_len = len;
+ bvec->bv_offset = offset;
+ bio->bi_vcnt++;
+ bio->bi_phys_segments++;
+
+ /*
+* Perform a recount if the number of segments is greater
+* than queue_max_segments(q).
*/
- while (bio->bi_phys_segments >= queue_max_segments(q)) {
+ while (bio->bi_phys_segments > queue_max_segments(q)) {
if (retried_segments)
- return 0;
+ goto failed;
retried_segments = 1;
blk_recount_segments(q, bio);
}
/*
-* setup the new entry, we might clear it again later if we
-* cannot add the page
-*/
- bvec = &bio->bi_io_vec[bio->bi_vcnt];
- bvec->bv_page = page;
- bvec->bv_len = len;
- bvec->bv_offset = offset;
-
- /*
* if queue has other restrictions (eg varying max sector size
* depending on offset), it can specify a merge_bvec_fn in the
* queue to get further control
@@ -789,23 +791,27 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
* merge_bvec_fn() returns number of bytes it can accept
* at this offset
*/
- if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len) {
- bvec->bv_page = NULL;
- bvec->bv_len = 0;
- bvec->bv_offset = 0;
- return 0;
- }
+ if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len)
+ goto failed;
}
/* If we may be able to merge these biovecs, force a recount */
- if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
+ if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
bio->bi_flags &= ~(1 << BIO_SEG_VALID);
- bio->bi_vcnt++;
- bio->bi_phys_segments++;
done:
bio->bi_iter.bi_size += len;
return len;
+
+ failed:
+ bvec->bv_page = NULL;
+ bvec->bv_len = 0;
+ bvec->bv_offset = 0;
+ bio->bi_vcnt--;
+ if (!retried_segments)
+ bio->bi_phys_segments--;
+
+ return 0;
}
/**
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V2] bio: modify __bio_add_page() to accept pages that don't start a new segment
The original behaviour is to refuse to add a new page if the maximum number
of segments has been reached, regardless of the fact the page we are
going to add can be merged into the last segment or not.
Unfortunately, when the system runs under heavy memory fragmentation conditions,
a driver may try to add multiple pages to the last segment.
The original code won't accept them and EBUSY will be reported to
userspace.
This patch modifies the function so it refuses to add a page
only in case the latter starts a new segment and the maximum number
of segments has already been reached.
The bug can be easily reproduced with the st driver:
1) set CONFIG_SCSI_MPT2SAS_MAX_SGE or CONFIG_SCSI_MPT3SAS_MAX_SGE to 16
2) modprobe st buffer_kbs=1024
3) #dd if=/dev/zero of=/dev/st0 bs=1M count=10
dd: error writing ‘/dev/st0’: Device or resource busy
Signed-off-by: Maurizio Lombardi
---
fs/bio.c | 51 +--
1 file changed, 29 insertions(+), 22 deletions(-)
diff --git a/fs/bio.c b/fs/bio.c
index 6f0362b..a31e12b 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -699,6 +699,7 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
unsigned int max_sectors)
{
int retried_segments = 0;
+ unsigned int bi_phys_segments_orig;
struct bio_vec *bvec;
/*
@@ -750,29 +751,32 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
return 0;
/*
-* we might lose a segment or two here, but rather that than
-* make this too complex.
+* setup the new entry, we might clear it again later if we
+* cannot add the page
+*/
+ bvec = &bio->bi_io_vec[bio->bi_vcnt];
+ bvec->bv_page = page;
+ bvec->bv_len = len;
+ bvec->bv_offset = offset;
+ bio->bi_vcnt++;
+ bi_phys_segments_orig = bio->bi_phys_segments;
+ bio->bi_phys_segments++;
+
+ /*
+* Perform a recount if the number of segments is greater
+* than queue_max_segments(q).
*/
- while (bio->bi_phys_segments >= queue_max_segments(q)) {
+ while (bio->bi_phys_segments > queue_max_segments(q)) {
if (retried_segments)
- return 0;
+ goto failed;
retried_segments = 1;
blk_recount_segments(q, bio);
}
/*
-* setup the new entry, we might clear it again later if we
-* cannot add the page
-*/
- bvec = &bio->bi_io_vec[bio->bi_vcnt];
- bvec->bv_page = page;
- bvec->bv_len = len;
- bvec->bv_offset = offset;
-
- /*
* if queue has other restrictions (eg varying max sector size
* depending on offset), it can specify a merge_bvec_fn in the
* queue to get further control
@@ -789,23 +793,26 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
* merge_bvec_fn() returns number of bytes it can accept
* at this offset
*/
- if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len) {
- bvec->bv_page = NULL;
- bvec->bv_len = 0;
- bvec->bv_offset = 0;
- return 0;
- }
+ if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len)
+ goto failed;
}
/* If we may be able to merge these biovecs, force a recount */
- if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
+ if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
bio->bi_flags &= ~(1 << BIO_SEG_VALID);
- bio->bi_vcnt++;
- bio->bi_phys_segments++;
done:
bio->bi_iter.bi_size += len;
return len;
+
+ failed:
+ bvec->bv_page = NULL;
+ bvec->bv_len = 0;
+ bvec->bv_offset = 0;
+ bio->bi_vcnt--;
+ bio->bi_phys_segments = bi_phys_segments_orig;
+
+ return 0;
}
/**
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bio: modify __bio_add_page() to accept pages that don't start a new segment
Sorry I did a mistake in this patch: on failure I should restore the original
value
of bi_phys_segments.
I'm going to send a new version.
Maurizio Lombardi
On Tue, Apr 29, 2014 at 04:58:18PM +0200, Maurizio Lombardi wrote:
> The original behaviour is to refuse to add a new page if the maximum number
> of segments has been reached, regardless of the fact the page we are
> going to add can be merged into the last segment or not.
>
> Unfortunately, when the system runs under heavy memory fragmentation
> conditions,
> a driver may try to add multiple pages to the last segment.
> The original code won't accept them and EBUSY will be reported to
> userspace.
>
> This patch modifies the function so it refuses to add a page
> only in case the latter starts a new segment and the maximum number
> of segments has already been reached.
>
> The bug can be easily reproduced with the st driver:
>
> 1) set CONFIG_SCSI_MPT2SAS_MAX_SGE or CONFIG_SCSI_MPT3SAS_MAX_SGE to 16
> 2) modprobe st buffer_kbs=1024
> 3) #dd if=/dev/zero of=/dev/st0 bs=1M count=10
>dd: error writing ‘/dev/st0’: Device or resource busy
>
> Signed-off-by: Maurizio Lombardi
> ---
> fs/bio.c | 50 --
> 1 file changed, 28 insertions(+), 22 deletions(-)
>
> diff --git a/fs/bio.c b/fs/bio.c
> index 6f0362b..9a3a0b1 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -750,29 +750,31 @@ static int __bio_add_page(struct request_queue *q,
> struct bio *bio, struct page
> return 0;
>
> /*
> - * we might lose a segment or two here, but rather that than
> - * make this too complex.
> + * setup the new entry, we might clear it again later if we
> + * cannot add the page
> + */
> + bvec = &bio->bi_io_vec[bio->bi_vcnt];
> + bvec->bv_page = page;
> + bvec->bv_len = len;
> + bvec->bv_offset = offset;
> + bio->bi_vcnt++;
> + bio->bi_phys_segments++;
> +
> + /*
> + * Perform a recount if the number of segments is greater
> + * than queue_max_segments(q).
>*/
>
> - while (bio->bi_phys_segments >= queue_max_segments(q)) {
> + while (bio->bi_phys_segments > queue_max_segments(q)) {
>
> if (retried_segments)
> - return 0;
> + goto failed;
>
> retried_segments = 1;
> blk_recount_segments(q, bio);
> }
>
> /*
> - * setup the new entry, we might clear it again later if we
> - * cannot add the page
> - */
> - bvec = &bio->bi_io_vec[bio->bi_vcnt];
> - bvec->bv_page = page;
> - bvec->bv_len = len;
> - bvec->bv_offset = offset;
> -
> - /*
>* if queue has other restrictions (eg varying max sector size
>* depending on offset), it can specify a merge_bvec_fn in the
>* queue to get further control
> @@ -789,23 +791,27 @@ static int __bio_add_page(struct request_queue *q,
> struct bio *bio, struct page
>* merge_bvec_fn() returns number of bytes it can accept
>* at this offset
>*/
> - if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len) {
> - bvec->bv_page = NULL;
> - bvec->bv_len = 0;
> - bvec->bv_offset = 0;
> - return 0;
> - }
> + if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len)
> + goto failed;
> }
>
> /* If we may be able to merge these biovecs, force a recount */
> - if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
> + if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
> bio->bi_flags &= ~(1 << BIO_SEG_VALID);
>
> - bio->bi_vcnt++;
> - bio->bi_phys_segments++;
> done:
> bio->bi_iter.bi_size += len;
> return len;
> +
> + failed:
> + bvec->bv_page = NULL;
> + bvec->bv_len = 0;
> + bvec->bv_offset = 0;
> + bio->bi_vcnt--;
> + if (!retried_segments)
> + bio->bi_phys_segments--;
> +
> + return 0;
> }
>
> /**
> --
> Maurizio Lombardi
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3] bio: modify __bio_add_page() to accept pages that don't start a new segment
The original behaviour is to refuse to add a new page if the maximum number
of segments has been reached, regardless of the fact the page we are
going to add can be merged into the last segment or not.
Unfortunately, when the system runs under heavy memory fragmentation conditions,
a driver may try to add multiple pages to the last segment.
The original code won't accept them and EBUSY will be reported to
userspace.
This patch modifies the function so it refuses to add a page
only in case the latter starts a new segment and the maximum number
of segments has already been reached.
The bug can be easily reproduced with the st driver:
1) set CONFIG_SCSI_MPT2SAS_MAX_SGE or CONFIG_SCSI_MPT3SAS_MAX_SGE to 16
2) modprobe st buffer_kbs=1024
3) #dd if=/dev/zero of=/dev/st0 bs=1M count=10
dd: error writing ‘/dev/st0’: Device or resource busy
Changes in V3:
In case of error, V2 restored the previous number of segments but left
the BIO_SEG_FLAG set.
To avoid problems, after the page is removed from the bio vec,
V3 performs a recount of the segments in the error code path.
Signed-off-by: Maurizio Lombardi
---
fs/bio.c | 48 ++--
1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/fs/bio.c b/fs/bio.c
index 6f0362b..9bf512e 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -750,29 +750,31 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
return 0;
/*
-* we might lose a segment or two here, but rather that than
-* make this too complex.
+* setup the new entry, we might clear it again later if we
+* cannot add the page
+*/
+ bvec = &bio->bi_io_vec[bio->bi_vcnt];
+ bvec->bv_page = page;
+ bvec->bv_len = len;
+ bvec->bv_offset = offset;
+ bio->bi_vcnt++;
+ bio->bi_phys_segments++;
+
+ /*
+* Perform a recount if the number of segments is greater
+* than queue_max_segments(q).
*/
- while (bio->bi_phys_segments >= queue_max_segments(q)) {
+ while (bio->bi_phys_segments > queue_max_segments(q)) {
if (retried_segments)
- return 0;
+ goto failed;
retried_segments = 1;
blk_recount_segments(q, bio);
}
/*
-* setup the new entry, we might clear it again later if we
-* cannot add the page
-*/
- bvec = &bio->bi_io_vec[bio->bi_vcnt];
- bvec->bv_page = page;
- bvec->bv_len = len;
- bvec->bv_offset = offset;
-
- /*
* if queue has other restrictions (eg varying max sector size
* depending on offset), it can specify a merge_bvec_fn in the
* queue to get further control
@@ -789,23 +791,25 @@ static int __bio_add_page(struct request_queue *q, struct
bio *bio, struct page
* merge_bvec_fn() returns number of bytes it can accept
* at this offset
*/
- if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len) {
- bvec->bv_page = NULL;
- bvec->bv_len = 0;
- bvec->bv_offset = 0;
- return 0;
- }
+ if (q->merge_bvec_fn(q, &bvm, bvec) < bvec->bv_len)
+ goto failed;
}
/* If we may be able to merge these biovecs, force a recount */
- if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
+ if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec)))
bio->bi_flags &= ~(1 << BIO_SEG_VALID);
- bio->bi_vcnt++;
- bio->bi_phys_segments++;
done:
bio->bi_iter.bi_size += len;
return len;
+
+ failed:
+ bvec->bv_page = NULL;
+ bvec->bv_len = 0;
+ bvec->bv_offset = 0;
+ bio->bi_vcnt--;
+ blk_recount_segments(q, bio);
+ return 0;
}
/**
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.
Hi, On Tue, Apr 29, 2014 at 08:00:41PM +0200, Christoph Hellwig wrote: > > What kind of update do you want? All the sense buffer updates are done > in the low-level code, and then you could update anything additional > you would need in the caller as you get the whole sense buffer when > using scsi_execute() (for scsi_execute_req_flags it's harder due to the > normalized sense buffer). Hannes Reinecke is also working on those functions, I'll have to collaborate with him to minimize the conflicts with the patchset he is preparing. In the meantime, do you think it is possible to merge the first version of the patch (http://www.spinics.net/lists/linux-scsi/msg73984.html)? We could proceed to rewrite the functions later. Thanks, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.
On Tue, May 13, 2014 at 04:14:47PM +0200, Hannes Reinecke wrote: > If you were to go ahead with the patch please strip the first > scsi_execute() altogether and use the VPD information already > provided by the scsi device. The patch I'm asking to merge just adds some printks, nothing else, it does not make use of scsi_execute(): http://www.spinics.net/lists/linux-scsi/msg73984.html Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] scsi_lib: replace ambiguous "Unhandled error code" messages.
During IO with fabric faults, one generally sees several "Unhandled error
code" messages in the syslog as shown below:
sd 4:0:6:2: [sdbw] Unhandled error code
sd 4:0:6:2: [sdbw] Result: hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
sd 4:0:6:2: [sdbw] CDB: Read(10): 28 00 00 00 00 00 00 00 08 00
end_request: I/O error, dev sdbw, sector 0
This comes from scsi_io_completion (in scsi_lib.c) while handling error
codes other than DID_RESET or not deferred sense keys i.e. this is
actually handled by the SCSI mid layer. But what gets displayed here is
"Unhandled error code" which is quite misleading as it indicates
something that is not addressed by the mid layer.
This patch replaces "Unhandled error code" with "Extended error description
not available".
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/scsi_lib.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9db097a..f2a1e13 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -955,12 +955,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
action = ACTION_FAIL;
break;
default:
- description = "Unhandled sense code";
+ description = "Extended sense code description not
available";
action = ACTION_FAIL;
break;
}
} else {
- description = "Unhandled error code";
+ description = "Extended error code description not available";
action = ACTION_FAIL;
}
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi_lib: replace ambiguous "Unhandled error code" messages.
Hi, On Mon, May 26, 2014 at 09:25:06AM -0700, Christoph Hellwig wrote: > On Mon, May 26, 2014 at 12:13:24PM +0200, Maurizio Lombardi wrote: > > During IO with fabric faults, one generally sees several "Unhandled error > > code" messages in the syslog as shown below: > > > > sd 4:0:6:2: [sdbw] Unhandled error code > > sd 4:0:6:2: [sdbw] Result: hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK > > sd 4:0:6:2: [sdbw] CDB: Read(10): 28 00 00 00 00 00 00 00 08 00 > > end_request: I/O error, dev sdbw, sector 0 > > > > This comes from scsi_io_completion (in scsi_lib.c) while handling error > > codes other than DID_RESET or not deferred sense keys i.e. this is > > actually handled by the SCSI mid layer. But what gets displayed here is > > "Unhandled error code" which is quite misleading as it indicates > > something that is not addressed by the mid layer. > > > > This patch replaces "Unhandled error code" with "Extended error description > > not available". > > How about simple not setting description at all for this case? > It has already been proposed before but James didn't like the idea. http://markmail.org/message/dumujpz4gfp3s4fp#query:+page:1+mid:dumujpz4gfp3s4fp+state:results Regards, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3] bio: modify __bio_add_page() to accept pages that don't start a new segment
On Tue, May 27, 2014 at 06:15:45PM +0800, Ming Lei wrote: > > If the page can be merged to last segment, it should have been > covered by code in branch of 'if (bio->bi_vcnt > 0) ...', shouldn't it? > > Or maybe it is better to make that code cover your case since > looks your case is similar with that one according to your commit > log. the code in this branch does not cover our case, it is intended to cover the case where __bio_add_page() is called multiple times with the *same* page as parameter. My patch deals with the case when __bio_add_page() is called with *different* pages as parameter but physically adjacent to each other. That said it is true that maybe this branch can be extended to also cover the case I'm dealing with and try to avoid the problem that commit 3979ef4dcf introduced. Thanks, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3] bio: modify __bio_add_page() to accept pages that don't start a new segment
On Tue, May 27, 2014 at 06:15:45PM +0800, Ming Lei wrote: > > If the page can be merged to last segment, it should have been > covered by code in branch of 'if (bio->bi_vcnt > 0) ...', shouldn't it? > > Or maybe it is better to make that code cover your case since > looks your case is similar with that one according to your commit > log. > I realized that maybe you mean this branch: if (bio->bi_vcnt > 1 && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec))) right? In this case the branch is not even reached in the original code because the function returned an error way before executing it (line 760) There was already a patchset trying to modify the code in a different way than mine: https://groups.google.com/forum/#!msg/linux.kernel/3IanUpBVhFQ/3Xbg3yLRFp4J but it has been ignored and in my opinion it takes a more complicated approach. Regards, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3] bio: modify __bio_add_page() to accept pages that don't start a new segment
On Tue, May 27, 2014 at 10:04:58PM +0800, Ming Lei wrote: > > Looks your approach is simpler. > > But looks there is one problem if I understand correctly: > __blk_recalc_rq_segments() may not cover the last vector > because bio->bi_iter.bi_size isn't updated until the end of > __bio_add_page(). > > But it shouldn't have been related with current virtio-blk problem. > This is a valid point, bi_iter.bi_size influences the behaviour of blk_recount_segments(). Maybe Jens can confirm your observation. Anyway it doesn't explain the reason behind the regression introduced by commit 3979ef4dcf Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: replace numeric messages with string error messages when blk_execute_rq fails. Also add printing of sense info.
On Thu, May 15, 2014 at 08:41:10AM +0200, Christoph Hellwig wrote: > > We now have a readily available copy of EVPD page 0x83 hanging off > the scsi device, so the alua device handler should use it. > > Hannes, how about you handle that if you're doing major surgery in that > area anwyay? Hannes? Did you notice this email? Thanks, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi_lib: replace ambiguous "Unhandled error code" messages.
On Wed, May 28, 2014 at 06:27:52PM +0400, James Bottomley wrote: > > I'm happy with eliminating "Unhanndled Error Code" because that is > misleading ... we should only get there if we have a DID_X return and > they're all fatal errors which will be printed. > > I'm less happy removing "Unhandled Sense Code". The danger is that we > get some harmless sense code we should have handled and instead error > the command. Since for this error we know the next print will be the > sense code, what about changing it to "Failing Command with sense code:" It's ok with me, I'm going to resend the patch in a few minutes. Thanks, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V2] scsi_lib: removes ambiguous "Unhandled error code" messages.
During IO with fabric faults, one generally sees several "Unhandled error
code" messages in the syslog as shown below:
sd 4:0:6:2: [sdbw] Unhandled error code
sd 4:0:6:2: [sdbw] Result: hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
sd 4:0:6:2: [sdbw] CDB: Read(10): 28 00 00 00 00 00 00 00 08 00
end_request: I/O error, dev sdbw, sector 0
This comes from scsi_io_completion (in scsi_lib.c) while handling error
codes other than DID_RESET or not deferred sense keys i.e. this is
actually handled by the SCSI mid layer. But what gets displayed here is
"Unhandled error code" which is quite misleading as it indicates
something that is not addressed by the mid layer.
This patch removes "Unhandled error code" and replaces "Unhandled sense code"
with "Failing command with sense code:".
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/scsi_lib.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9db097a..b3c25cd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -955,14 +955,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
action = ACTION_FAIL;
break;
default:
- description = "Unhandled sense code";
+ description = "Failing command with sense code:";
action = ACTION_FAIL;
break;
}
- } else {
- description = "Unhandled error code";
+ } else
action = ACTION_FAIL;
- }
if (action != ACTION_FAIL &&
time_before(cmd->jiffies_at_alloc + wait_for, jiffies)) {
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] pm8001: Fix potential null pointer dereference and memory leak.
The pm8001_get_phy_settings_info() function does not check
the kzalloc() return value and does not free the allocated memory.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/pm8001/pm8001_init.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_init.c
b/drivers/scsi/pm8001/pm8001_init.c
index c4f31b21..e90c89f 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -677,7 +677,7 @@ static void pm8001_init_sas_add(struct pm8001_hba_info
*pm8001_ha)
* pm8001_get_phy_settings_info : Read phy setting values.
* @pm8001_ha : our hba.
*/
-void pm8001_get_phy_settings_info(struct pm8001_hba_info *pm8001_ha)
+static int pm8001_get_phy_settings_info(struct pm8001_hba_info *pm8001_ha)
{
#ifdef PM8001_READ_VPD
@@ -691,11 +691,15 @@ void pm8001_get_phy_settings_info(struct pm8001_hba_info
*pm8001_ha)
payload.offset = 0;
payload.length = 4096;
payload.func_specific = kzalloc(4096, GFP_KERNEL);
+ if (!payload.func_specific)
+ return -ENOMEM;
/* Read phy setting values from flash */
PM8001_CHIP_DISP->get_nvmd_req(pm8001_ha, &payload);
wait_for_completion(&completion);
pm8001_set_phy_profile(pm8001_ha, sizeof(u8), payload.func_specific);
+ kfree(payload.func_specific);
#endif
+ return 0;
}
#ifdef PM8001_USE_MSIX
@@ -879,8 +883,11 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
pm8001_init_sas_add(pm8001_ha);
/* phy setting support for motherboard controller */
if (pdev->subsystem_vendor != PCI_VENDOR_ID_ADAPTEC2 &&
- pdev->subsystem_vendor != 0)
- pm8001_get_phy_settings_info(pm8001_ha);
+ pdev->subsystem_vendor != 0) {
+ rc = pm8001_get_phy_settings_info(pm8001_ha);
+ if (rc)
+ goto err_out_shost;
+ }
pm8001_post_sas_ha_init(shost, chip);
rc = sas_register_ha(SHOST_TO_SAS_HA(shost));
if (rc)
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] bnx2fc: do not scan uninitialized lists in case of error.
In case of of error, the bnx2fc_cmd_mgr_alloc() function will call
the bnx2fc_cmd_mgr_free() to perform the cleanup.
The problem is that in one case the latter may try to scan
some not-yet initialized lists, resulting in a kernel panic.
This patch prevents this from happening by freeing the lists
before calling bnx2fc_cmd_mgr_free().
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/bnx2fc/bnx2fc_io.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
index 32a5e0a..7bc47fc 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -282,6 +282,8 @@ struct bnx2fc_cmd_mgr *bnx2fc_cmd_mgr_alloc(struct
bnx2fc_hba *hba)
arr_sz, GFP_KERNEL);
if (!cmgr->free_list_lock) {
printk(KERN_ERR PFX "failed to alloc free_list_lock\n");
+ kfree(cmgr->free_list);
+ cmgr->free_list = NULL;
goto mem_err;
}
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Possible use after free in scsi_put_command()?
Hi Hannes,
I've a question regarding the asynchronous scsi abort handler,
look at the scsi_put_command() function:
void scsi_put_command(struct scsi_cmnd *cmd)
{
unsigned long flags;
[...]
cancel_delayed_work(&cmd->abort_work);
__scsi_put_command(cmd->device->host, cmd);
}
cancel_delayed_work() may return while the abort handler is still running,
the problem is that __scsi_put_command() frees the cmd pointer that
is still used by the abort handler.
Is it correct? Isn't safer to use cancel_delayed_work_sync() here?
Thanks,
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Possible use after free in scsi_put_command()?
Hi Bart, On 06/25/2014 08:52 AM, Bart Van Assche wrote: > Hello Maurizio, > > I agree that that cancel_delayed_work() call is confusing. Hence > "[PATCH] Remove two cancel_delayed_work() calls from the mid-layer" > (http://thread.gmane.org/gmane.linux.scsi/91027). Had you already > noticed that patch ? > No, I missed it, thanks for pointing it out to me. Thanks, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: bnx2i: bnx2i_iscsi.c: Cleaning up variable is set more than once
Hi, On 06/25/2014 04:04 PM, Rickard Strandqvist wrote: > A struct member variable is set to different values without having used in > between. > > This was found using a static code analysis program called cppcheck > > Signed-off-by: Rickard Strandqvist > --- > drivers/scsi/bnx2i/bnx2i_iscsi.c |1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c > b/drivers/scsi/bnx2i/bnx2i_iscsi.c > index 166543f..fdf7bc3 100644 > --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c > +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c > @@ -1643,7 +1643,6 @@ static void bnx2i_conn_get_stats(struct iscsi_cls_conn > *cls_conn, > stats->r2t_pdus = conn->r2t_pdus_cnt; > stats->tmfcmd_pdus = conn->tmfcmd_pdus_cnt; > stats->tmfrsp_pdus = conn->tmfrsp_pdus_cnt; > - stats->custom_length = 3; > strcpy(stats->custom[2].desc, "eh_abort_cnt"); > stats->custom[2].value = conn->eh_abort_cnt; > stats->digest_err = 0; > Eddie, The code modifies the content of stats->custom[2], so shouldn't custom_length be set to 3? Why is it set to zero at the end of this function? Regards, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: pm8001: pm80xx_hwi.c: Cleaning up variable is set more than once
This one looks good to me, Reviewed-by: Maurizio Lombardi On 06/25/2014 04:01 PM, Rickard Strandqvist wrote: > A struct member variable is set to different values without having used in > between. > > This was found using a static code analysis program called cppcheck > > Signed-off-by: Rickard Strandqvist > --- > drivers/scsi/pm8001/pm80xx_hwi.c |1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c > b/drivers/scsi/pm8001/pm80xx_hwi.c > index d70587f..2698227 100644 > --- a/drivers/scsi/pm8001/pm80xx_hwi.c > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c > @@ -249,7 +249,6 @@ moreData: > sprintf(pm8001_ha-> > forensic_info.data_buf.direct_data, > "%08x ", 4); > - pm8001_ha->forensic_info.data_buf.read_len = 0x; > pm8001_ha->forensic_info.data_buf.direct_len = 0; > pm8001_ha->forensic_info.data_buf.direct_offset = 0; > pm8001_ha->forensic_info.data_buf.read_len = 0; > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: pm8001: pm80xx_hwi.c: Cleaning up variable is set more than once
Hi, On 06/25/2014 05:41 PM, Purush Gupta wrote: > Its possible HW may require programming those fields? I'm looking at the code and it doesn't look so, did you see something suspicious? > May be original > contributor of the driver should review...No offense! I believe it requires the maintainer ACK and it takes precedence in any case, am I wrong? Regards, Maurizio Lombardi > > thanks, > Purush > > > On Wed, Jun 25, 2014 at 8:34 AM, Maurizio Lombardi > wrote: > >> This one looks good to me, >> >> Reviewed-by: Maurizio Lombardi >> >> On 06/25/2014 04:01 PM, Rickard Strandqvist wrote: >>> A struct member variable is set to different values without having used >> in between. >>> >>> This was found using a static code analysis program called cppcheck >>> >>> Signed-off-by: Rickard Strandqvist < >> rickard_strandqv...@spectrumdigital.se> >>> --- >>> drivers/scsi/pm8001/pm80xx_hwi.c |1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c >> b/drivers/scsi/pm8001/pm80xx_hwi.c >>> index d70587f..2698227 100644 >>> --- a/drivers/scsi/pm8001/pm80xx_hwi.c >>> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c >>> @@ -249,7 +249,6 @@ moreData: >>> sprintf(pm8001_ha-> >>> forensic_info.data_buf.direct_data, >>> "%08x ", 4); >>> - pm8001_ha->forensic_info.data_buf.read_len = >> 0x; >>> pm8001_ha->forensic_info.data_buf.direct_len = 0; >>> pm8001_ha->forensic_info.data_buf.direct_offset = >> 0; >>> pm8001_ha->forensic_info.data_buf.read_len = 0; >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: bnx2i: bnx2i_iscsi.c: Cleaning up variable is set more than once
Hi, On 06/26/2014 02:28 AM, Rickard Strandqvist wrote: > > If it's not obvious, I do all my fixes without changing the previous > intent. But obviously it is not always right, rather it is one of main > reasons to fix this type of error :) Yes it is obvious, your patch was correct (it didn't modify the function behaviour) but helped to spot a defect. > > But I'll make a new patch then, with = 3 ? Yes, please submit a new patch which sets custom_length = 3 at the end of the function. Thanks, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi: pm8001: pm80xx_hwi.c: Cleaning up variable is set more than once
On 06/26/2014 10:09 AM, Jack Wang wrote: > Thanks Rickard, > > From my point of view, looks good, but I'd like to get review from Anand > (cc-ed). I would like to add that I noticed that this fields is only set and appears to be never used, maybe it could be completely removed. Regards, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] beiscsi: the custom_length field has been set to a wrong value.
In the beiscsi_conn_get_stats() function, custom_length should be set to 1 to take into account the "eh_abort_cnt" field at custom[0]. Signed-off-by: Maurizio Lombardi --- drivers/scsi/be2iscsi/be_iscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c index fd284ff..8616281 100644 --- a/drivers/scsi/be2iscsi/be_iscsi.c +++ b/drivers/scsi/be2iscsi/be_iscsi.c @@ -914,7 +914,7 @@ void beiscsi_conn_get_stats(struct iscsi_cls_conn *cls_conn, stats->r2t_pdus = conn->r2t_pdus_cnt; stats->digest_err = 0; stats->timeout_err = 0; - stats->custom_length = 0; + stats->custom_length = 1; strcpy(stats->custom[0].desc, "eh_abort_cnt"); stats->custom[0].value = conn->eh_abort_cnt; } -- Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] scsi: bnx2i: bnx2i_iscsi.c: Cleaning up variable is set more than once
On 06/26/2014 02:05 PM, Joe Perches wrote: > On Thu, 2014-06-26 at 13:54 +0200, Rickard Strandqvist wrote: >> A struct member variable is set to different values without having used in >> between. > [] >> diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c >> b/drivers/scsi/bnx2i/bnx2i_iscsi.c > [] >> @@ -1643,12 +1643,11 @@ static void bnx2i_conn_get_stats(struct >> iscsi_cls_conn *cls_conn, >> stats->r2t_pdus = conn->r2t_pdus_cnt; >> stats->tmfcmd_pdus = conn->tmfcmd_pdus_cnt; >> stats->tmfrsp_pdus = conn->tmfrsp_pdus_cnt; >> -stats->custom_length = 3; >> strcpy(stats->custom[2].desc, "eh_abort_cnt"); >> stats->custom[2].value = conn->eh_abort_cnt; >> stats->digest_err = 0; >> stats->timeout_err = 0; >> -stats->custom_length = 0; >> +stats->custom_length = 3; > > You are changing custom_length from 0 to 3. > > Why is this correct? http://marc.info/?l=linux-scsi&m=140371670511706&w=2 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] scsi: bnx2i: bnx2i_iscsi.c: Cleaning up variable is set more than once
On 06/26/2014 01:54 PM, Rickard Strandqvist wrote: > A struct member variable is set to different values without having used in > between. It is almost ok for me but I think you should mention that it also fixes a bug, or the commit message will be misleading. > > This was found using a static code analysis program called cppcheck > > Signed-off-by: Rickard Strandqvist > --- > drivers/scsi/bnx2i/bnx2i_iscsi.c |3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c > b/drivers/scsi/bnx2i/bnx2i_iscsi.c > index 166543f..4e17a7f 100644 > --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c > +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c > @@ -1643,12 +1643,11 @@ static void bnx2i_conn_get_stats(struct > iscsi_cls_conn *cls_conn, > stats->r2t_pdus = conn->r2t_pdus_cnt; > stats->tmfcmd_pdus = conn->tmfcmd_pdus_cnt; > stats->tmfrsp_pdus = conn->tmfrsp_pdus_cnt; > - stats->custom_length = 3; > strcpy(stats->custom[2].desc, "eh_abort_cnt"); > stats->custom[2].value = conn->eh_abort_cnt; > stats->digest_err = 0; > stats->timeout_err = 0; > - stats->custom_length = 0; > + stats->custom_length = 3; > } > > > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] b2iscsi: Fix memory leak in mgmt_set_ip()
The if_info pointer is not released by the mgmt_set_ip() function
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/be2iscsi/be_mgmt.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/be2iscsi/be_mgmt.c b/drivers/scsi/be2iscsi/be_mgmt.c
index 07934b0..a3e5648 100644
--- a/drivers/scsi/be2iscsi/be_mgmt.c
+++ b/drivers/scsi/be2iscsi/be_mgmt.c
@@ -1015,7 +1015,7 @@ int mgmt_set_ip(struct beiscsi_hba *phba,
if (if_info->dhcp_state) {
beiscsi_log(phba, KERN_WARNING, BEISCSI_LOG_CONFIG,
"BG_%d : DHCP Already Enabled\n");
- return 0;
+ goto exit;
}
/* The ip_param->len is 1 in DHCP case. Setting
proper IP len as this it is used while
@@ -1033,7 +1033,7 @@ int mgmt_set_ip(struct beiscsi_hba *phba,
sizeof(*reldhcp));
if (rc)
- return rc;
+ goto exit;
reldhcp = nonemb_cmd.va;
reldhcp->interface_hndl = phba->interface_handle;
@@ -1044,7 +1044,7 @@ int mgmt_set_ip(struct beiscsi_hba *phba,
beiscsi_log(phba, KERN_WARNING,
BEISCSI_LOG_CONFIG,
"BG_%d : Failed to Delete existing
dhcp\n");
- return rc;
+ goto exit;
}
}
}
@@ -1054,7 +1054,7 @@ int mgmt_set_ip(struct beiscsi_hba *phba,
rc = mgmt_static_ip_modify(phba, if_info, ip_param, NULL,
IP_ACTION_DEL);
if (rc)
- return rc;
+ goto exit;
}
/* Delete the Gateway settings if mode change is to DHCP */
@@ -1064,7 +1064,7 @@ int mgmt_set_ip(struct beiscsi_hba *phba,
if (rc) {
beiscsi_log(phba, KERN_WARNING, BEISCSI_LOG_CONFIG,
"BG_%d : Failed to Get Gateway Addr\n");
- return rc;
+ goto exit;
}
if (gtway_addr_set.ip_addr.addr[0]) {
@@ -1076,7 +1076,7 @@ int mgmt_set_ip(struct beiscsi_hba *phba,
beiscsi_log(phba, KERN_WARNING,
BEISCSI_LOG_CONFIG,
"BG_%d : Failed to clear Gateway
Addr Set\n");
- return rc;
+ goto exit;
}
}
}
@@ -1087,7 +1087,7 @@ int mgmt_set_ip(struct beiscsi_hba *phba,
OPCODE_COMMON_ISCSI_NTWK_CONFIG_STATELESS_IP_ADDR,
sizeof(*dhcpreq));
if (rc)
- return rc;
+ goto exit;
dhcpreq = nonemb_cmd.va;
dhcpreq->flags = BLOCKING;
@@ -1095,12 +1095,14 @@ int mgmt_set_ip(struct beiscsi_hba *phba,
dhcpreq->interface_hndl = phba->interface_handle;
dhcpreq->ip_type = BE2_DHCP_V4;
- return mgmt_exec_nonemb_cmd(phba, &nonemb_cmd, NULL, 0);
+ rc = mgmt_exec_nonemb_cmd(phba, &nonemb_cmd, NULL, 0);
} else {
- return mgmt_static_ip_modify(phba, if_info, ip_param,
+ rc = mgmt_static_ip_modify(phba, if_info, ip_param,
subnet_param, IP_ACTION_ADD);
}
+exit:
+ kfree(if_info);
return rc;
}
--
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] b2iscsi: Fix memory leak in mgmt_set_ip()
>> diff --git a/drivers/scsi/be2iscsi/be_mgmt.c
>> b/drivers/scsi/be2iscsi/be_mgmt.c
>> index 07934b0..a3e5648 100644
>> --- a/drivers/scsi/be2iscsi/be_mgmt.c
>> +++ b/drivers/scsi/be2iscsi/be_mgmt.c
>> @@ -1015,7 +1015,7 @@ int mgmt_set_ip(struct beiscsi_hba *phba,
>> if (if_info->dhcp_state) {
>> beiscsi_log(phba, KERN_WARNING, BEISCSI_LOG_CONFIG,
>> "BG_%d : DHCP Already Enabled\n");
>> -return 0;
>> +goto exit;
>
> rc should be initialised - so this needs to be added to your patch
It is initialized at the beginning of the function, and it must contain zero
when the "goto exit" is executed.
int mgmt_set_ip(struct beiscsi_hba *phba,
struct iscsi_iface_param_info *ip_param,
struct iscsi_iface_param_info *subnet_param,
uint32_t boot_proto)
{
struct be_cmd_get_def_gateway_resp gtway_addr_set;
struct be_cmd_get_if_info_resp *if_info;
struct be_cmd_set_dhcp_req *dhcpreq;
struct be_cmd_rel_dhcp_req *reldhcp;
struct be_dma_mem nonemb_cmd;
uint8_t *gtway_addr;
uint32_t ip_type;
int rc;
if (mgmt_get_all_if_id(phba))
return -EIO;
ip_type = (ip_param->param == ISCSI_NET_PARAM_IPV6_ADDR) ?
BE2_IPV6 : BE2_IPV4 ;
rc = mgmt_get_if_info(phba, ip_type, &if_info); <-- here
if (rc)
return rc;
if (boot_proto == ISCSI_BOOTPROTO_DHCP) {
if (if_info->dhcp_state) {
beiscsi_log(phba, KERN_WARNING, BEISCSI_LOG_CONFIG,
"BG_%d : DHCP Already Enabled\n");
goto exit;
}
Regards,
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 1/8] target: Add locking to some accesses to nacl.device_list
Hi Andy, On 07/01/2014 01:39 AM, Andy Grover wrote: > diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c > index 101858e..5c9d980 100644 > --- a/drivers/target/target_core_ua.c > +++ b/drivers/target/target_core_ua.c > @@ -51,9 +51,12 @@ target_scsi3_ua_check(struct se_cmd *cmd) > if (!nacl) > return 0; > > + spin_lock_irq(&nacl->device_list_lock); > deve = nacl->device_list[cmd->orig_fe_lun]; > if (!atomic_read(&deve->ua_count)) > return 0; > + spin_unlock_irq(&nacl->device_list_lock); > + Shouldn't the spinlock unlocked before the "return 0;" ? Regards, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Some question about usb scsi storage driver
On 07/04/2014 04:44 PM, loody wrote: > > 2. at the end of sd.c -> sd_probe, why we call async_schedule_domain like > below > async_schedule_domain(sd_probe_async, sdkp, &scsi_sd_probe_domain); > to finish scsi device initialization? > Couldn't we put what sd_probe_async directly in sd_probe? > is there any benefit to do so? A possible explanation may be that sd_probe_async() calls sd_spinup_disk(), this function spins up the drive and may block for some seconds, so it is better to do that asynchronously. Regards, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] scsi_lib: removes ambiguous "Unhandled error code" messages.
On 07/03/2014 10:41 AM, Christoph Hellwig wrote: > On Wed, Jun 25, 2014 at 08:38:47PM +, Elliott, Robert (Server Storage) > wrote: >> Since the ACTION_FAIL case always prints the sense key >> and additional sense code: > >> perhaps the description string should be removed altogether? > > > I'm tempted to agree and just remove the description. Do you want to > send a patch for this? So I'll get rid of the "description" string completely... I'm going to send a new patch later today. Regards, Maurizio Lombardi -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/8][RESEND] pm8001: Fix potential null pointer dereference and memory leak.
Hi,
On 07/09/2014 01:50 PM, Suresh Thiagarajan wrote:
> From: Suresh Thiagarajan
>
> The pm8001_get_phy_settings_info() function does not check the
> kzalloc() return value and does not free the allocated memory.
This patch is already in mainline.
Regards,
Maurizio Lombardi
>
> Signed-off-by: Maurizio Lombardi
> Acked-by: Jack Wang
> Acked-by: Suresh Thiagarajan
> ---
> drivers/scsi/pm8001/pm8001_init.c | 13 ++---
> 1 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c
> b/drivers/scsi/pm8001/pm8001_init.c
> index 86cf03a..aad060a 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -677,7 +677,7 @@ static void pm8001_init_sas_add(struct pm8001_hba_info
> *pm8001_ha)
> * pm8001_get_phy_settings_info : Read phy setting values.
> * @pm8001_ha : our hba.
> */
> -void pm8001_get_phy_settings_info(struct pm8001_hba_info *pm8001_ha)
> +static int pm8001_get_phy_settings_info(struct pm8001_hba_info *pm8001_ha)
> {
>
> #ifdef PM8001_READ_VPD
> @@ -691,11 +691,15 @@ void pm8001_get_phy_settings_info(struct
> pm8001_hba_info *pm8001_ha)
> payload.offset = 0;
> payload.length = 4096;
> payload.func_specific = kzalloc(4096, GFP_KERNEL);
> + if (!payload.func_specific)
> + return -ENOMEM;
> /* Read phy setting values from flash */
> PM8001_CHIP_DISP->get_nvmd_req(pm8001_ha, &payload);
> wait_for_completion(&completion);
> pm8001_set_phy_profile(pm8001_ha, sizeof(u8), payload.func_specific);
> + kfree(payload.func_specific);
> #endif
> + return 0;
> }
>
> #ifdef PM8001_USE_MSIX
> @@ -879,8 +883,11 @@ static int pm8001_pci_probe(struct pci_dev *pdev,
> pm8001_init_sas_add(pm8001_ha);
> /* phy setting support for motherboard controller */
> if (pdev->subsystem_vendor != PCI_VENDOR_ID_ADAPTEC2 &&
> - pdev->subsystem_vendor != 0)
> - pm8001_get_phy_settings_info(pm8001_ha);
> + pdev->subsystem_vendor != 0) {
> + rc = pm8001_get_phy_settings_info(pm8001_ha);
> + if (rc)
> + goto err_out_shost;
> + }
> pm8001_post_sas_ha_init(shost, chip);
> rc = sas_register_ha(SHOST_TO_SAS_HA(shost));
> if (rc)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3] scsi_lib: remove the description string in scsi_io_completion()
During IO with fabric faults, one generally sees several "Unhandled error
code" messages in the syslog as shown below:
sd 4:0:6:2: [sdbw] Unhandled error code
sd 4:0:6:2: [sdbw] Result: hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
sd 4:0:6:2: [sdbw] CDB: Read(10): 28 00 00 00 00 00 00 00 08 00
end_request: I/O error, dev sdbw, sector 0
This comes from scsi_io_completion (in scsi_lib.c) while handling error
codes other than DID_RESET or not deferred sense keys i.e. this is
actually handled by the SCSI mid layer. But what gets displayed here is
"Unhandled error code" which is quite misleading as it indicates
something that is not addressed by the mid layer.
The description string is based on the sense key and sometimes on the
additional sense code;
since the ACTION_FAIL case always prints the sense key and the
additional sense code, this patch removes the description string
completely because it does not add useful information.
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/scsi_lib.c | 40
1 file changed, 4 insertions(+), 36 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f7e3163..e953b9b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -686,7 +686,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int
good_bytes)
int sense_deferred = 0;
enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
ACTION_DELAYED_RETRY} action;
- char *description = NULL;
unsigned long wait_for = (cmd->allowed + 1) * req->timeout;
if (result) {
@@ -803,7 +802,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int
good_bytes)
* and quietly refuse further access.
*/
cmd->device->changed = 1;
- description = "Media Changed";
action = ACTION_FAIL;
} else {
/* Must have been a power glitch, or a
@@ -831,27 +829,10 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
cmd->device->use_10_for_rw = 0;
action = ACTION_REPREP;
} else if (sshdr.asc == 0x10) /* DIX */ {
- description = "Host Data Integrity Failure";
action = ACTION_FAIL;
error = -EILSEQ;
/* INVALID COMMAND OPCODE or INVALID FIELD IN CDB */
} else if (sshdr.asc == 0x20 || sshdr.asc == 0x24) {
- switch (cmd->cmnd[0]) {
- case UNMAP:
- description = "Discard failure";
- break;
- case WRITE_SAME:
- case WRITE_SAME_16:
- if (cmd->cmnd[1] & 0x8)
- description = "Discard failure";
- else
- description =
- "Write same failure";
- break;
- default:
- description = "Invalid command failure";
- break;
- }
action = ACTION_FAIL;
error = -EREMOTEIO;
} else
@@ -859,10 +840,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
break;
case ABORTED_COMMAND:
action = ACTION_FAIL;
- if (sshdr.asc == 0x10) { /* DIF */
- description = "Target Data Integrity Failure";
+ if (sshdr.asc == 0x10) /* DIF */
error = -EILSEQ;
- }
break;
case NOT_READY:
/* If the device is in the process of becoming
@@ -881,42 +860,31 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
action = ACTION_DELAYED_RETRY;
break;
default:
- description = "Device not ready";
action = ACTION_FAIL;
break;
}
- } else {
-
[PATCH] scsi: iscsi: flush running unbind operations when removing a session
In some cases, the iscsi_remove_session() function is called while an unbind_work operation is still running. This may cause a situation where sysfs objects are removed in an incorrect order, triggering a kernel warning. [ 605.249442] [ cut here ] [ 605.259180] sysfs group 'power' not found for kobject 'target2:0:0' [ 605.321371] WARNING: CPU: 1 PID: 26794 at fs/sysfs/group.c:235 sysfs_remove_group+0x76/0x80 [ 605.341266] Modules linked in: dm_service_time target_core_user target_core_pscsi target_core_file target_core_iblock iscsi_target_mod target_core_mod nls_utf8 isofs ppdev bochs_drm nfit ttm libnvdimm drm_kms_helper syscopyarea sysfillrect sysimgblt joydev pcspkr fb_sys_fops drm i2c_piix4 sg parport_pc parport xfs libcrc32c dm_multipath sr_mod sd_mod cdrom ata_generic 8021q garp mrp ata_piix stp crct10dif_pclmul crc32_pclmul llc libata crc32c_intel virtio_net net_failover ghash_clmulni_intel serio_raw failover sunrpc dm_mirror dm_region_hash dm_log dm_mod be2iscsi bnx2i cnic uio cxgb4i cxgb4 libcxgbi libcxgb qla4xxx iscsi_boot_sysfs iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi [ 605.627479] CPU: 1 PID: 26794 Comm: kworker/u32:2 Not tainted 4.18.0-60.el8.x86_64 #1 [ 605.721401] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20180724_192412-buildhw-07.phx2.fedoraproject.org-1.fc29 04/01/2014 [ 605.823651] Workqueue: scsi_wq_2 __iscsi_unbind_session [scsi_transport_iscsi] [ 605.830940] RIP: 0010:sysfs_remove_group+0x76/0x80 [ 605.922907] Code: 48 89 df 5b 5d 41 5c e9 38 c4 ff ff 48 89 df e8 e0 bf ff ff eb cb 49 8b 14 24 48 8b 75 00 48 c7 c7 38 73 cb a7 e8 24 77 d7 ff <0f> 0b 5b 5d 41 5c c3 0f 1f 00 0f 1f 44 00 00 41 56 41 55 41 54 55 [ 606.122304] RSP: 0018:badcc8d1bda8 EFLAGS: 00010286 [ 606.218492] RAX: RBX: RCX: [ 606.326381] RDX: 98bdfe85eb40 RSI: 98bdfe856818 RDI: 98bdfe856818 [ 606.514498] RBP: a7ab73e0 R08: 0268 R09: 0007 [ 606.529469] R10: R11: a860d9ad R12: 98bdf978e838 [ 606.630535] R13: 98bdc2cd4010 R14: 98bdc2cd3ff0 R15: 98bdc2cd4000 [ 606.824707] FS: () GS:98bdfe84() knlGS: [ 607.018333] CS: 0010 DS: ES: CR0: 80050033 [ 607.117844] CR2: 7f84b78ac024 CR3: 2c00a003 CR4: 003606e0 [ 607.117844] DR0: DR1: DR2: [ 607.420926] DR3: DR6: fffe0ff0 DR7: 0400 [ 607.524236] Call Trace: [ 607.530591] device_del+0x56/0x350 [ 607.624393] ? ata_tlink_match+0x30/0x30 [libata] [ 607.727805] ? attribute_container_device_trigger+0xb4/0xf0 [ 607.829911] scsi_target_reap_ref_release+0x39/0x50 [ 607.928572] scsi_remove_target+0x1a2/0x1d0 [ 608.017350] __iscsi_unbind_session+0xb3/0x160 [scsi_transport_iscsi] [ 608.117435] process_one_work+0x1a7/0x360 [ 608.132917] worker_thread+0x30/0x390 [ 608.222900] ? pwq_unbound_release_workfn+0xd0/0xd0 [ 608.323989] kthread+0x112/0x130 [ 608.418318] ? kthread_bind+0x30/0x30 [ 608.513821] ret_from_fork+0x35/0x40 [ 608.613909] ---[ end trace 0b98c310c8a6138c ]--- Signed-off-by: Maurizio Lombardi --- drivers/scsi/scsi_transport_iscsi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c index ff12302..1e872ab 100644 --- a/drivers/scsi/scsi_transport_iscsi.c +++ b/drivers/scsi/scsi_transport_iscsi.c @@ -2182,6 +2182,8 @@ void iscsi_remove_session(struct iscsi_cls_session *session) scsi_target_unblock(&session->dev, SDEV_TRANSPORT_OFFLINE); /* flush running scans then delete devices */ flush_work(&session->scan_work); + /* flush running unbind operations */ + flush_work(&session->unbind_work); __iscsi_unbind_session(&session->unbind_work); /* hw iscsi may not have removed all connections from session */ -- Maurizio Lombardi
[PATCH] cdrom: do not call check_disk_change() inside cdrom_open()
when mounting an ISO filesystem sometimes (very rarely) the system hangs because of a race condition between two tasks. PID: 6766 TASK: 88007b2a6dd0 CPU: 0 COMMAND: "mount" #0 [880078447ae0] __schedule at 8168d605 #1 [880078447b48] schedule_preempt_disabled at 8168ed49 #2 [880078447b58] __mutex_lock_slowpath at 8168c995 #3 [880078447bb8] mutex_lock at 8168bdef #4 [880078447bd0] sr_block_ioctl at a00b6818 [sr_mod] #5 [880078447c10] blkdev_ioctl at 812fea50 #6 [880078447c70] ioctl_by_bdev at 8123a8b3 #7 [880078447c90] isofs_fill_super at a04fb1e1 [isofs] #8 [880078447da8] mount_bdev at 81202570 #9 [880078447e18] isofs_mount at a04f9828 [isofs] #10 [880078447e28] mount_fs at 81202d09 #11 [880078447e70] vfs_kern_mount at 8121ea8f #12 [880078447ea8] do_mount at 81220fee #13 [880078447f28] sys_mount at 812218d6 #14 [880078447f80] system_call_fastpath at 81698c49 RIP: 7fd9ea914e9a RSP: 7ffd5d9bf648 RFLAGS: 00010246 RAX: 00a5 RBX: 81698c49 RCX: 0010 RDX: 7fd9ec2bc210 RSI: 7fd9ec2bc290 RDI: 7fd9ec2bcf30 RBP: R8: R9: 0010 R10: c0ed0001 R11: 0206 R12: 7fd9ec2bc040 R13: 7fd9eb6b2380 R14: 7fd9ec2bc210 R15: 7fd9ec2bcf30 ORIG_RAX: 00a5 CS: 0033 SS: 002b This task was trying to mount the cdrom. It allocated and configured a super_block struct and owned the write-lock for the super_block->s_umount rwsem. While exclusively owning the s_umount lock, it called sr_block_ioctl and waited to acquire the global sr_mutex lock. PID: 6785 TASK: 880078720fb0 CPU: 0 COMMAND: "systemd-udevd" #0 [880078417898] __schedule at 8168d605 #1 [880078417900] schedule at 8168dc59 #2 [880078417910] rwsem_down_read_failed at 8168f605 #3 [880078417980] call_rwsem_down_read_failed at 81328838 #4 [8800784179d0] down_read at 8168cde0 #5 [8800784179e8] get_super at 81201cc7 #6 [880078417a10] __invalidate_device at 8123a8de #7 [880078417a40] flush_disk at 8123a94b #8 [880078417a88] check_disk_change at 8123ab50 #9 [880078417ab0] cdrom_open at a00a29e1 [cdrom] #10 [880078417b68] sr_block_open at a00b6f9b [sr_mod] #11 [880078417b98] __blkdev_get at 8123ba86 #12 [880078417bf0] blkdev_get at 8123bd65 #13 [880078417c78] blkdev_open at 8123bf9b #14 [880078417c90] do_dentry_open at 811fc7f7 #15 [880078417cd8] vfs_open at 811fc9cf #16 [880078417d00] do_last at 8120d53d #17 [880078417db0] path_openat at 8120e6b2 #18 [880078417e48] do_filp_open at 8121082b #19 [880078417f18] do_sys_open at 811fdd33 #20 [880078417f70] sys_open at 811fde4e #21 [880078417f80] system_call_fastpath at 81698c49 RIP: 7f29438b0c20 RSP: 7ffc76624b78 RFLAGS: 00010246 RAX: 0002 RBX: 81698c49 RCX: RDX: 7f2944a5fa70 RSI: 000a0800 RDI: 7f2944a5fa70 RBP: 7f2944a5f540 R8: R9: 0020 R10: 7f2943614c40 R11: 0246 R12: 811fde4e R13: 880078417f78 R14: 000c R15: 7f2944a4b010 ORIG_RAX: 0002 CS: 0033 SS: 002b This task tried to open the cdrom device, the sr_block_open function acquired the global sr_mutex lock. The call to check_disk_change() then saw an event flag indicating a possible media change and tried to flush any cached data for the device. As part of the flush, it tried to acquire the super_block->s_umount lock associated with the cdrom device. This was the same super_block as created and locked by the previous task. The first task acquires the s_umount lock and then the sr_mutex_lock; the second task acquires the sr_mutex_lock and then the s_umount lock. This patch fixes the issue by moving check_disk_change() out of cdrom_open() and let the caller take care of it. Signed-off-by: Maurizio Lombardi --- drivers/block/paride/pcd.c | 2 ++ drivers/cdrom/cdrom.c | 3 --- drivers/cdrom/gdrom.c | 3 +++ drivers/ide/ide-cd.c | 2 ++ drivers/scsi/sr.c | 2 ++ 5 files changed, 9 insertions(+), 3 deletions(-) diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c index 7b8c636..a026211 100644 --- a/drivers/block/paride/pcd.c +++ b/drivers/block/paride/pcd.c @@ -230,6 +230,8 @@ static int pcd_block_open(struct block_device *bdev, fmode_t mode) struct pcd_unit *cd = bdev->bd_disk->private_data; int ret; + check_disk_change(bdev); + mut
[PATCH] scsi: use scmd_printk() to print which command timed out
With a possibly faulty disk the following messages may appear in the logs:
kernel: sd 0:0:9:0: timing out command, waited 180s
kernel: sd 0:0:9:0: timing out command, waited 20s
kernel: sd 0:0:9:0: timing out command, waited 20s
kernel: sd 0:0:9:0: timing out command, waited 60s
kernel: sd 0:0:9:0: timing out command, waited 20s
This is not very informative because it's not possible to identify
the command that timed out.
This patch replaces sdev_printk() with scmd_printk().
Signed-off-by: Maurizio Lombardi
---
drivers/scsi/scsi_lib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f6437b98296b..97ed233fa469 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1501,7 +1501,7 @@ static void scsi_softirq_done(struct request *rq)
disposition = scsi_decide_disposition(cmd);
if (disposition != SUCCESS &&
time_before(cmd->jiffies_at_alloc + wait_for, jiffies)) {
- sdev_printk(KERN_ERR, cmd->device,
+ scmd_printk(KERN_ERR, cmd,
"timing out command, waited %lus\n",
wait_for/HZ);
disposition = SUCCESS;
--
Maurizio Lombardi
[RFC PATCH 0/4] iscsi: chap: introduce support for SHA1 and SHA3-256
iSCSI with the Challenge-Handshake Authentication Protocol is not FIPS compliant. This is due to the fact that CHAP currently uses MD5 as the only supported digest algorithm and MD5 is not allowed by FIPS. When FIPS mode is enabled on the target server, the CHAP authentication won't work because the target driver will be prevented from using the MD5 module. Given that CHAP is agnostic regarding the algorithm it uses, this patchset introduce support for two new alternatives: SHA1 and SHA3-256. SHA1 has already its own assigned value for its use in CHAP, as reported by IANA: https://www.iana.org/assignments/ppp-numbers/ppp-numbers.xml#ppp-numbers-9 On the other hand the use of SHA1 on FIPS-enabled systems has been deprecated and therefore it's not a vialable long term option. We could consider introducing a more modern hash algorithm like SHA3-256, as this patchset does. A pull request for the open-iscsi initiator side implementation has been submitted here: https://github.com/open-iscsi/open-iscsi/pull/170 Maurizio Lombardi (4): target-iscsi: CHAP: add support to SHA1 and SHA3-256 hash functions target-iscsi: remove unneeded function target-iscsi: tie the challenge length to the hash digest size target-iscsi: rename some variables to avoid confusion. drivers/target/iscsi/iscsi_target_auth.c | 218 ++- drivers/target/iscsi/iscsi_target_auth.h | 13 +- 2 files changed, 147 insertions(+), 84 deletions(-) -- Maurizio Lombardi
[PATCH 1/4] target-iscsi: CHAP: add support to SHA1 and SHA3-256 hash functions
This patches modifies the chap_server_compute_hash() function
to make it agnostic to the choice of hash algorithm that is used.
It also adds support to two new hash algorithms: SHA1 and SHA3-256
Signed-off-by: Maurizio Lombardi
---
drivers/target/iscsi/iscsi_target_auth.c | 135 +--
drivers/target/iscsi/iscsi_target_auth.h | 9 +-
2 files changed, 108 insertions(+), 36 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target_auth.c
b/drivers/target/iscsi/iscsi_target_auth.c
index 51ddca2033e0..3d1e94333835 100644
--- a/drivers/target/iscsi/iscsi_target_auth.c
+++ b/drivers/target/iscsi/iscsi_target_auth.c
@@ -46,9 +46,22 @@ static int chap_gen_challenge(
return 0;
}
+static int chap_test_algorithm(const char *name)
+{
+ struct crypto_shash *tfm;
+
+ tfm = crypto_alloc_shash(name, 0, 0);
+ if (IS_ERR(tfm))
+ return -1;
+
+ crypto_free_shash(tfm);
+ return 0;
+}
+
static int chap_check_algorithm(const char *a_str)
{
char *tmp, *orig, *token;
+ int r = CHAP_DIGEST_UNKNOWN;
tmp = kstrdup(a_str, GFP_KERNEL);
if (!tmp) {
@@ -72,13 +85,33 @@ static int chap_check_algorithm(const char *a_str)
if (!strncmp(token, "5", 1)) {
pr_debug("Selected MD5 Algorithm\n");
- kfree(orig);
- return CHAP_DIGEST_MD5;
+ if (chap_test_algorithm("md5") < 0) {
+ pr_err("failed to allocate md5 algo\n");
+ continue;
+ }
+ r = CHAP_DIGEST_MD5;
+ goto out;
+ } else if (!strncmp(token, "6", 1)) {
+ pr_debug("Selected SHA1 Algorithm\n");
+ if (chap_test_algorithm("sha1") < 0) {
+ pr_err("failed to allocate sha1 algo\n");
+ continue;
+ }
+ r = CHAP_DIGEST_SHA;
+ goto out;
+ } else if (!strncmp(token, "7", 1)) {
+ pr_debug("Selected SHA3-256 Algorithm\n");
+ if (chap_test_algorithm("sha3-256") < 0) {
+ pr_err("failed to allocate sha3-256 algo\n");
+ continue;
+ }
+ r = CHAP_DIGEST_SHA3_256;
+ goto out;
}
}
out:
kfree(orig);
- return CHAP_DIGEST_UNKNOWN;
+ return r;
}
static void chap_close(struct iscsi_conn *conn)
@@ -94,7 +127,7 @@ static struct iscsi_chap *chap_server_open(
char *aic_str,
unsigned int *aic_len)
{
- int ret;
+ int digest_type;
struct iscsi_chap *chap;
if (!(auth->naf_flags & NAF_USERID_SET) ||
@@ -109,17 +142,19 @@ static struct iscsi_chap *chap_server_open(
return NULL;
chap = conn->auth_protocol;
- ret = chap_check_algorithm(a_str);
- switch (ret) {
+ digest_type = chap_check_algorithm(a_str);
+ switch (digest_type) {
case CHAP_DIGEST_MD5:
- pr_debug("[server] Got CHAP_A=5\n");
- /*
-* Send back CHAP_A set to MD5.
- */
- *aic_len = sprintf(aic_str, "CHAP_A=5");
- *aic_len += 1;
- chap->digest_type = CHAP_DIGEST_MD5;
- pr_debug("[server] Sending CHAP_A=%d\n", chap->digest_type);
+ chap->digest_size = MD5_SIGNATURE_SIZE;
+ chap->digest_name = "md5";
+ break;
+ case CHAP_DIGEST_SHA:
+ chap->digest_size = SHA_SIGNATURE_SIZE;
+ chap->digest_name = "sha1";
+ break;
+ case CHAP_DIGEST_SHA3_256:
+ chap->digest_size = SHA3_256_SIGNATURE_SIZE;
+ chap->digest_name = "sha3-256";
break;
case CHAP_DIGEST_UNKNOWN:
default:
@@ -128,6 +163,11 @@ static struct iscsi_chap *chap_server_open(
return NULL;
}
+ pr_debug("[server] Got CHAP_A=%d\n", digest_type);
+ *aic_len = sprintf(aic_str, "CHAP_A=%d", digest_type);
+ *aic_len += 1;
+ pr_debug("[server] Sending CHAP_A=%d\n", digest_type);
+
/*
* Set Identifier.
*/
@@ -146,7 +186,7 @@ static struct iscsi_chap *chap_server_open(
return chap;
}
-static int chap_server_compute_md5(
+static int chap_server_compute_hash(
struct iscsi_conn *conn,
struct iscsi_node_auth *auth,
char *nr_in_ptr,
@@ -155,12
[PATCH 2/4] target-iscsi: remove unneeded function
The digest type validy is already checked by chap_server_open(), therefore
we can remove the chap_got_response() function.
Signed-off-by: Maurizio Lombardi
---
drivers/target/iscsi/iscsi_target_auth.c | 26 +---
1 file changed, 1 insertion(+), 25 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target_auth.c
b/drivers/target/iscsi/iscsi_target_auth.c
index 3d1e94333835..77dac8086927 100644
--- a/drivers/target/iscsi/iscsi_target_auth.c
+++ b/drivers/target/iscsi/iscsi_target_auth.c
@@ -473,30 +473,6 @@ static int chap_server_compute_hash(
return auth_ret;
}
-static int chap_got_response(
- struct iscsi_conn *conn,
- struct iscsi_node_auth *auth,
- char *nr_in_ptr,
- char *nr_out_ptr,
- unsigned int *nr_out_len)
-{
- struct iscsi_chap *chap = conn->auth_protocol;
-
- switch (chap->digest_type) {
- case CHAP_DIGEST_MD5:
- case CHAP_DIGEST_SHA:
- case CHAP_DIGEST_SHA3_256:
- if (chap_server_compute_hash(conn, auth, nr_in_ptr,
- nr_out_ptr, nr_out_len) < 0)
- return -1;
- return 0;
- default:
- pr_err("Unknown CHAP digest type %d!\n",
- chap->digest_type);
- return -1;
- }
-}
-
u32 chap_main_loop(
struct iscsi_conn *conn,
struct iscsi_node_auth *auth,
@@ -515,7 +491,7 @@ u32 chap_main_loop(
return 0;
} else if (chap->chap_state == CHAP_STAGE_SERVER_AIC) {
convert_null_to_semi(in_text, *in_len);
- if (chap_got_response(conn, auth, in_text, out_text,
+ if (chap_server_compute_hash(conn, auth, in_text, out_text,
out_len) < 0) {
chap_close(conn);
return 2;
--
Maurizio Lombardi
[PATCH 3/4] target-iscsi: tie the challenge length to the hash digest size
Signed-off-by: Maurizio Lombardi
---
drivers/target/iscsi/iscsi_target_auth.c | 37 +---
drivers/target/iscsi/iscsi_target_auth.h | 4 +--
2 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target_auth.c
b/drivers/target/iscsi/iscsi_target_auth.c
index 77dac8086927..976c8c73d261 100644
--- a/drivers/target/iscsi/iscsi_target_auth.c
+++ b/drivers/target/iscsi/iscsi_target_auth.c
@@ -25,16 +25,21 @@ static int chap_gen_challenge(
unsigned int *c_len)
{
int ret;
- unsigned char challenge_asciihex[CHAP_CHALLENGE_LENGTH * 2 + 1];
+ unsigned char *challenge_asciihex;
struct iscsi_chap *chap = conn->auth_protocol;
- memset(challenge_asciihex, 0, CHAP_CHALLENGE_LENGTH * 2 + 1);
+ challenge_asciihex = kzalloc(chap->challenge_len * 2 + 1, GFP_KERNEL);
+ if (!challenge_asciihex)
+ return -ENOMEM;
- ret = get_random_bytes_wait(chap->challenge, CHAP_CHALLENGE_LENGTH);
+ memset(chap->challenge, 0, MAX_CHAP_CHALLENGE_LEN);
+
+ ret = get_random_bytes_wait(chap->challenge, chap->challenge_len);
if (unlikely(ret))
- return ret;
+ goto out;
+
bin2hex(challenge_asciihex, chap->challenge,
- CHAP_CHALLENGE_LENGTH);
+ chap->challenge_len);
/*
* Set CHAP_C, and copy the generated challenge into c_str.
*/
@@ -43,7 +48,10 @@ static int chap_gen_challenge(
pr_debug("[%s] Sending CHAP_C=0x%s\n\n", (caller) ? "server" : "client",
challenge_asciihex);
- return 0;
+
+out:
+ kfree(challenge_asciihex);
+ return ret;
}
static int chap_test_algorithm(const char *name)
@@ -163,6 +171,9 @@ static struct iscsi_chap *chap_server_open(
return NULL;
}
+ /* Tie the challenge length to the digest size */
+ chap->challenge_len = chap->digest_size;
+
pr_debug("[server] Got CHAP_A=%d\n", digest_type);
*aic_len = sprintf(aic_str, "CHAP_A=%d", digest_type);
*aic_len += 1;
@@ -326,21 +337,23 @@ static int chap_server_compute_hash(
}
ret = crypto_shash_finup(desc, chap->challenge,
-CHAP_CHALLENGE_LENGTH, server_digest);
+chap->challenge_len, server_digest);
if (ret < 0) {
pr_err("crypto_shash_finup() failed for challenge\n");
goto out;
}
bin2hex(response, server_digest, chap->digest_size);
- pr_debug("[server] %s Server Digest: %s\n", hash_name, response);
+ pr_debug("[server] %s Server Digest: %s\n",
+ chap->digest_name, response);
if (memcmp(server_digest, client_digest, chap->digest_size) != 0) {
- pr_debug("[server] %s Digests do not match!\n\n", hash_name);
+ pr_debug("[server] %s Digests do not match!\n\n",
+ chap->digest_name);
goto out;
} else
pr_debug("[server] %s Digests match, CHAP connection"
- " successful.\n\n", hash_name);
+ " successful.\n\n", chap->digest_name);
/*
* One way authentication has succeeded, return now if mutual
* authentication is not enabled.
@@ -406,7 +419,9 @@ static int chap_server_compute_hash(
* initiator must not match the original CHAP_C generated by
* the target.
*/
- if (!memcmp(challenge_binhex, chap->challenge, CHAP_CHALLENGE_LENGTH)) {
+ if (challenge_len == chap->challenge_len &&
+ !memcmp(challenge_binhex, chap->challenge,
+ challenge_len)) {
pr_err("initiator CHAP_C matches target CHAP_C, failing"
" login attempt\n");
goto out;
diff --git a/drivers/target/iscsi/iscsi_target_auth.h
b/drivers/target/iscsi/iscsi_target_auth.h
index 8b10f935675a..e22840fea903 100644
--- a/drivers/target/iscsi/iscsi_target_auth.h
+++ b/drivers/target/iscsi/iscsi_target_auth.h
@@ -9,7 +9,7 @@
#define CHAP_DIGEST_SHA6
#define CHAP_DIGEST_SHA3_256 7
-#define CHAP_CHALLENGE_LENGTH 16
+#define MAX_CHAP_CHALLENGE_LEN 32
#define CHAP_CHALLENGE_STR_LEN 4096
#define MAX_RESPONSE_LENGTH128 /* sufficient for SHA3 256 */
#defineMAX_CHAP_N_SIZE 512
@@ -32,7 +32,7 @@ extern u32 chap_main_loop(struct iscsi_conn *, struct
iscsi_node_auth *, char *,
struct iscsi_chap {
unsigned char id;
- unsigned char challenge[CHAP_CHALLENGE_LENGTH];
+ unsigned char challenge[MAX_CHAP_CHALLENGE_LEN];
unsigned intchallenge_len;
unsigned char *digest_name;
unsigned intdigest_size;
--
Maurizio Lombardi
