In this code, 0 is returned on memory allocation failure, even though
other failures return -ENOMEM or other similar values.
A simplified version of the Coccinelle semantic match that finds this
problem is as follows:
// <smpl>
@@
expression ret;
expression x,e1,e2,e3;
identifier alloc;
@@
ret = 0
... when != ret = e1
*x = alloc(...)
... when != ret = e2
if (x == NULL) { ... when != ret = e3
return ret;
}
// </smpl>
Signed-off-by: Himangi Saraogi <[email protected]>
Acked-by: Julia Lawall <[email protected]>
---
drivers/scsi/qla4xxx/ql4_os.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index c5d9564..72ba671 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host *shost, char *buf, int len)
if (!ql_iscsi_stats) {
ql4_printk(KERN_ERR, ha,
"Unable to allocate memory for iscsi stats\n");
+ ret = -ENOMEM;
goto exit_host_stats;
}
--
1.9.1
> -----Original Message-----
> From: [email protected] [mailto:linux-scsi-
> [email protected]] On Behalf Of Himangi Saraogi
> Sent: Friday, 04 July, 2014 1:28 PM
> To: Vikas Chaudhary; [email protected]; James E.J. Bottomley; linux-
> [email protected]; [email protected]
> Cc: [email protected]
> Subject: [PATCH] qla4xxx: Return -ENOMEM on memory allocation failure
>
> In this code, 0 is returned on memory allocation failure, even though
> other failures return -ENOMEM or other similar values.
>
> A simplified version of the Coccinelle semantic match that finds this
> problem is as follows:
>
> // <smpl>
> @@
> expression ret;
> expression x,e1,e2,e3;
> identifier alloc;
> @@
>
> ret = 0
> ... when != ret = e1
> *x = alloc(...)
> ... when != ret = e2
> if (x == NULL) { ... when != ret = e3
> return ret;
> }
> // </smpl>
>
> Signed-off-by: Himangi Saraogi <[email protected]>
> Acked-by: Julia Lawall <[email protected]>
> ---
> drivers/scsi/qla4xxx/ql4_os.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> index c5d9564..72ba671 100644
> --- a/drivers/scsi/qla4xxx/ql4_os.c
> +++ b/drivers/scsi/qla4xxx/ql4_os.c
> @@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host
> *shost, char *buf, int len)
> if (!ql_iscsi_stats) {
> ql4_printk(KERN_ERR, ha,
> "Unable to allocate memory for iscsi stats\n");
> + ret = -ENOMEM;
> goto exit_host_stats;
> }
>
Also, the only caller of this function doesn't use the return
value - it's overwritten by another function call:
drivers/scsi/scsi_transport_iscsi.c:
err = transport->get_host_stats(shost, buf, host_stats_size);
actual_size = nlmsg_total_size(sizeof(*ev) + host_stats_size);
skb_trim(skbhost_stats, NLMSG_ALIGN(actual_size));
nlhhost_stats->nlmsg_len = actual_size;
err = iscsi_multicast_skb(skbhost_stats, ISCSI_NL_GRP_ISCSID,
GFP_KERNEL);
---
Rob Elliott HP Server Storage
On Fri, 4 Jul 2014, Elliott, Robert (Server Storage) wrote:
>
>
> > -----Original Message-----
> > From: [email protected] [mailto:linux-scsi-
> > [email protected]] On Behalf Of Himangi Saraogi
> > Sent: Friday, 04 July, 2014 1:28 PM
> > To: Vikas Chaudhary; [email protected]; James E.J. Bottomley; linux-
> > [email protected]; [email protected]
> > Cc: [email protected]
> > Subject: [PATCH] qla4xxx: Return -ENOMEM on memory allocation failure
> >
> > In this code, 0 is returned on memory allocation failure, even though
> > other failures return -ENOMEM or other similar values.
> >
> > A simplified version of the Coccinelle semantic match that finds this
> > problem is as follows:
> >
> > // <smpl>
> > @@
> > expression ret;
> > expression x,e1,e2,e3;
> > identifier alloc;
> > @@
> >
> > ret = 0
> > ... when != ret = e1
> > *x = alloc(...)
> > ... when != ret = e2
> > if (x == NULL) { ... when != ret = e3
> > return ret;
> > }
> > // </smpl>
> >
> > Signed-off-by: Himangi Saraogi <[email protected]>
> > Acked-by: Julia Lawall <[email protected]>
> > ---
> > drivers/scsi/qla4xxx/ql4_os.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> > index c5d9564..72ba671 100644
> > --- a/drivers/scsi/qla4xxx/ql4_os.c
> > +++ b/drivers/scsi/qla4xxx/ql4_os.c
> > @@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host
> > *shost, char *buf, int len)
> > if (!ql_iscsi_stats) {
> > ql4_printk(KERN_ERR, ha,
> > "Unable to allocate memory for iscsi stats\n");
> > + ret = -ENOMEM;
> > goto exit_host_stats;
> > }
> >
>
> Also, the only caller of this function doesn't use the return
> value - it's overwritten by another function call:
>
> drivers/scsi/scsi_transport_iscsi.c:
> err = transport->get_host_stats(shost, buf, host_stats_size);
>
> actual_size = nlmsg_total_size(sizeof(*ev) + host_stats_size);
> skb_trim(skbhost_stats, NLMSG_ALIGN(actual_size));
> nlhhost_stats->nlmsg_len = actual_size;
>
> err = iscsi_multicast_skb(skbhost_stats, ISCSI_NL_GRP_ISCSID,
> GFP_KERNEL);
Maybe that is intentional? If get_host_stats fails, eg because of a lack
of memory, the worst that will happen is that a region of the buffer will
be 0. If the loop is done again, there seems to be a risk of an infinite
loop.
julia
On Fri, 4 Jul 2014, Julia Lawall wrote:
>
>
> On Fri, 4 Jul 2014, Elliott, Robert (Server Storage) wrote:
>
> >
> >
> > > -----Original Message-----
> > > From: [email protected] [mailto:linux-scsi-
> > > [email protected]] On Behalf Of Himangi Saraogi
> > > Sent: Friday, 04 July, 2014 1:28 PM
> > > To: Vikas Chaudhary; [email protected]; James E.J. Bottomley; linux-
> > > [email protected]; [email protected]
> > > Cc: [email protected]
> > > Subject: [PATCH] qla4xxx: Return -ENOMEM on memory allocation failure
> > >
> > > In this code, 0 is returned on memory allocation failure, even though
> > > other failures return -ENOMEM or other similar values.
> > >
> > > A simplified version of the Coccinelle semantic match that finds this
> > > problem is as follows:
> > >
> > > // <smpl>
> > > @@
> > > expression ret;
> > > expression x,e1,e2,e3;
> > > identifier alloc;
> > > @@
> > >
> > > ret = 0
> > > ... when != ret = e1
> > > *x = alloc(...)
> > > ... when != ret = e2
> > > if (x == NULL) { ... when != ret = e3
> > > return ret;
> > > }
> > > // </smpl>
> > >
> > > Signed-off-by: Himangi Saraogi <[email protected]>
> > > Acked-by: Julia Lawall <[email protected]>
> > > ---
> > > drivers/scsi/qla4xxx/ql4_os.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> > > index c5d9564..72ba671 100644
> > > --- a/drivers/scsi/qla4xxx/ql4_os.c
> > > +++ b/drivers/scsi/qla4xxx/ql4_os.c
> > > @@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host
> > > *shost, char *buf, int len)
> > > if (!ql_iscsi_stats) {
> > > ql4_printk(KERN_ERR, ha,
> > > "Unable to allocate memory for iscsi stats\n");
> > > + ret = -ENOMEM;
> > > goto exit_host_stats;
> > > }
> > >
> >
> > Also, the only caller of this function doesn't use the return
> > value - it's overwritten by another function call:
> >
> > drivers/scsi/scsi_transport_iscsi.c:
> > err = transport->get_host_stats(shost, buf, host_stats_size);
> >
> > actual_size = nlmsg_total_size(sizeof(*ev) + host_stats_size);
> > skb_trim(skbhost_stats, NLMSG_ALIGN(actual_size));
> > nlhhost_stats->nlmsg_len = actual_size;
> >
> > err = iscsi_multicast_skb(skbhost_stats, ISCSI_NL_GRP_ISCSID,
> > GFP_KERNEL);
>
> Maybe that is intentional? If get_host_stats fails, eg because of a lack
> of memory, the worst that will happen is that a region of the buffer will
> be 0. If the loop is done again, there seems to be a risk of an infinite
> loop.
Or one could jump out of the function completely, as on failure of
alloc_skb.
julia
On 07/04/2014 03:37 PM, Julia Lawall wrote:
>
>
> On Fri, 4 Jul 2014, Elliott, Robert (Server Storage) wrote:
>
>>
>>
>>> -----Original Message-----
>>> From: [email protected] [mailto:linux-scsi-
>>> [email protected]] On Behalf Of Himangi Saraogi
>>> Sent: Friday, 04 July, 2014 1:28 PM
>>> To: Vikas Chaudhary; [email protected]; James E.J. Bottomley; linux-
>>> [email protected]; [email protected]
>>> Cc: [email protected]
>>> Subject: [PATCH] qla4xxx: Return -ENOMEM on memory allocation failure
>>>
>>> In this code, 0 is returned on memory allocation failure, even though
>>> other failures return -ENOMEM or other similar values.
>>>
>>> A simplified version of the Coccinelle semantic match that finds this
>>> problem is as follows:
>>>
>>> // <smpl>
>>> @@
>>> expression ret;
>>> expression x,e1,e2,e3;
>>> identifier alloc;
>>> @@
>>>
>>> ret = 0
>>> ... when != ret = e1
>>> *x = alloc(...)
>>> ... when != ret = e2
>>> if (x == NULL) { ... when != ret = e3
>>> return ret;
>>> }
>>> // </smpl>
>>>
>>> Signed-off-by: Himangi Saraogi <[email protected]>
>>> Acked-by: Julia Lawall <[email protected]>
>>> ---
>>> drivers/scsi/qla4xxx/ql4_os.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
>>> index c5d9564..72ba671 100644
>>> --- a/drivers/scsi/qla4xxx/ql4_os.c
>>> +++ b/drivers/scsi/qla4xxx/ql4_os.c
>>> @@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host
>>> *shost, char *buf, int len)
>>> if (!ql_iscsi_stats) {
>>> ql4_printk(KERN_ERR, ha,
>>> "Unable to allocate memory for iscsi stats\n");
>>> + ret = -ENOMEM;
>>> goto exit_host_stats;
>>> }
>>>
>>
>> Also, the only caller of this function doesn't use the return
>> value - it's overwritten by another function call:
>>
>> drivers/scsi/scsi_transport_iscsi.c:
>> err = transport->get_host_stats(shost, buf, host_stats_size);
>>
>> actual_size = nlmsg_total_size(sizeof(*ev) + host_stats_size);
>> skb_trim(skbhost_stats, NLMSG_ALIGN(actual_size));
>> nlhhost_stats->nlmsg_len = actual_size;
>>
>> err = iscsi_multicast_skb(skbhost_stats, ISCSI_NL_GRP_ISCSID,
>> GFP_KERNEL);
>
> Maybe that is intentional?
I think it was a mistake. There is also one more issue in
qla4xxx_get_host_stats where it is returning an internal qlogic
specific error value. This patch should fix all issues.
[PATCH] qla4xxx/iscsi: fix get_host_stats error propagation
This patch fixes 2 bugs.
1. qla4xxx was not always returning -EXYZ error codes when
qla4xxx_get_host_stats failed.
2. iscsi_get_host_stats was dropping the error code returned
by drivers like qla4xxx.
Signed-off-by: Mike Christie <[email protected]>
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index 459b9f7..e4dc3e0 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host *shost, char *buf, int len)
if (!ql_iscsi_stats) {
ql4_printk(KERN_ERR, ha,
"Unable to allocate memory for iscsi stats\n");
+ ret = -ENOMEM;
goto exit_host_stats;
}
@@ -1058,6 +1059,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host *shost, char *buf, int len)
if (ret != QLA_SUCCESS) {
ql4_printk(KERN_ERR, ha,
"Unable to retrieve iscsi stats\n");
+ ret = -EIO;
goto exit_host_stats;
}
host_stats->mactx_frames = le64_to_cpu(ql_iscsi_stats->mac_tx_frames);
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 0102a2d..ea2996a 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -3467,6 +3467,10 @@ iscsi_get_host_stats(struct iscsi_transport *transport, struct nlmsghdr *nlh)
memset(buf, 0, host_stats_size);
err = transport->get_host_stats(shost, buf, host_stats_size);
+ if (err) {
+ kfree(skbhost_stats);
+ goto exit_host_stats;
+ }
actual_size = nlmsg_total_size(sizeof(*ev) + host_stats_size);
skb_trim(skbhost_stats, NLMSG_ALIGN(actual_size));
On 07/07/14 10:01 pm, "Mike Christie" <[email protected]> wrote:
>On 07/04/2014 03:37 PM, Julia Lawall wrote:
>>
>>
>> On Fri, 4 Jul 2014, Elliott, Robert (Server Storage) wrote:
>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: [email protected] [mailto:linux-scsi-
>>>> [email protected]] On Behalf Of Himangi Saraogi
>>>> Sent: Friday, 04 July, 2014 1:28 PM
>>>> To: Vikas Chaudhary; [email protected]; James E.J. Bottomley;
>>>>linux-
>>>> [email protected]; [email protected]
>>>> Cc: [email protected]
>>>> Subject: [PATCH] qla4xxx: Return -ENOMEM on memory allocation failure
>>>>
>>>> In this code, 0 is returned on memory allocation failure, even though
>>>> other failures return -ENOMEM or other similar values.
>>>>
>>>> A simplified version of the Coccinelle semantic match that finds this
>>>> problem is as follows:
>>>>
>>>> // <smpl>
>>>> @@
>>>> expression ret;
>>>> expression x,e1,e2,e3;
>>>> identifier alloc;
>>>> @@
>>>>
>>>> ret = 0
>>>> ... when != ret = e1
>>>> *x = alloc(...)
>>>> ... when != ret = e2
>>>> if (x == NULL) { ... when != ret = e3
>>>> return ret;
>>>> }
>>>> // </smpl>
>>>>
>>>> Signed-off-by: Himangi Saraogi <[email protected]>
>>>> Acked-by: Julia Lawall <[email protected]>
>>>> ---
>>>> drivers/scsi/qla4xxx/ql4_os.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/scsi/qla4xxx/ql4_os.c
>>>>b/drivers/scsi/qla4xxx/ql4_os.c
>>>> index c5d9564..72ba671 100644
>>>> --- a/drivers/scsi/qla4xxx/ql4_os.c
>>>> +++ b/drivers/scsi/qla4xxx/ql4_os.c
>>>> @@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct
>>>>Scsi_Host
>>>> *shost, char *buf, int len)
>>>> if (!ql_iscsi_stats) {
>>>> ql4_printk(KERN_ERR, ha,
>>>> "Unable to allocate memory for iscsi stats\n");
>>>> + ret = -ENOMEM;
>>>> goto exit_host_stats;
>>>> }
>>>>
>>>
>>> Also, the only caller of this function doesn't use the return
>>> value - it's overwritten by another function call:
>>>
>>> drivers/scsi/scsi_transport_iscsi.c:
>>> err = transport->get_host_stats(shost, buf,
>>>host_stats_size);
>>>
>>> actual_size = nlmsg_total_size(sizeof(*ev) +
>>>host_stats_size);
>>> skb_trim(skbhost_stats, NLMSG_ALIGN(actual_size));
>>> nlhhost_stats->nlmsg_len = actual_size;
>>>
>>> err = iscsi_multicast_skb(skbhost_stats,
>>>ISCSI_NL_GRP_ISCSID,
>>> GFP_KERNEL);
>>
>> Maybe that is intentional?
>
>I think it was a mistake. There is also one more issue in
>qla4xxx_get_host_stats where it is returning an internal qlogic
>specific error value. This patch should fix all issues.
>
>
>[PATCH] qla4xxx/iscsi: fix get_host_stats error propagation
>
>This patch fixes 2 bugs.
>
>1. qla4xxx was not always returning -EXYZ error codes when
>qla4xxx_get_host_stats failed.
>
>2. iscsi_get_host_stats was dropping the error code returned
>by drivers like qla4xxx.
>
>Signed-off-by: Mike Christie <[email protected]>
>
>diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
>index 459b9f7..e4dc3e0 100644
>--- a/drivers/scsi/qla4xxx/ql4_os.c
>+++ b/drivers/scsi/qla4xxx/ql4_os.c
>@@ -1050,6 +1050,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host
>*shost, char *buf, int len)
> if (!ql_iscsi_stats) {
> ql4_printk(KERN_ERR, ha,
> "Unable to allocate memory for iscsi stats\n");
>+ ret = -ENOMEM;
> goto exit_host_stats;
> }
>
>@@ -1058,6 +1059,7 @@ static int qla4xxx_get_host_stats(struct Scsi_Host
>*shost, char *buf, int len)
> if (ret != QLA_SUCCESS) {
> ql4_printk(KERN_ERR, ha,
> "Unable to retrieve iscsi stats\n");
>+ ret = -EIO;
> goto exit_host_stats;
> }
> host_stats->mactx_frames = le64_to_cpu(ql_iscsi_stats->mac_tx_frames);
>diff --git a/drivers/scsi/scsi_transport_iscsi.c
>b/drivers/scsi/scsi_transport_iscsi.c
>index 0102a2d..ea2996a 100644
>--- a/drivers/scsi/scsi_transport_iscsi.c
>+++ b/drivers/scsi/scsi_transport_iscsi.c
>@@ -3467,6 +3467,10 @@ iscsi_get_host_stats(struct iscsi_transport
>*transport, struct nlmsghdr *nlh)
> memset(buf, 0, host_stats_size);
>
> err = transport->get_host_stats(shost, buf, host_stats_size);
>+ if (err) {
>+ kfree(skbhost_stats);
>+ goto exit_host_stats;
>+ }
>
> actual_size = nlmsg_total_size(sizeof(*ev) + host_stats_size);
> skb_trim(skbhost_stats, NLMSG_ALIGN(actual_size));
Acked-by: Vikas Chaudhary <[email protected]>