On Fri, 2012-02-24 at 21:19 -0800, Roland Dreier wrote:
> On Fri, Feb 24, 2012 at 5:17 PM, Nicholas A. Bellinger
> <nab@xxxxxxxxxxxxxxx> wrote:
> > This looks like a double free here on the second failure of
> > iscsit_allocate_iovecs(). Fixing this up now to drop the extra bogus
> > kfree(cmd->t_mem_sg) here..
>
> I don't see how the patch you committed:
>
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -830,7 +830,7 @@ page_alloc_failed:
> __free_page(sg_page(&sgl[i]));
> i--;
> }
> - kfree(cmd->t_mem_sg);
> + kfree(sgl);
> cmd->t_mem_sg = NULL;
> return -ENOMEM;
> }
>
> could possibly be right... you drop the kfree() of cmd->t_mem_sg but leave
> in the "cmd->t_mem_sg = NULL". Isn't that a guaranteed leak of cmd->t_mem_sg?
>
It's the same memory that's being allocated in iscsit_alloc_buffs(), but
you're right that this is bogus for the iscsit_allocate_iovecs() failure
case.
> I think the old code was probably OK (although I get lost a bit in the
> twisty maze of what exactly happens to the cmd on this failure path), and
> maybe the new code would be OK if you got rid of the "cmd->t_mem_sg = NULL".
>
After testing with simulated alloc_page() + iscsit_allocate_iovecs()
failures in iscsit_alloc_buffs(), the old code was in fact doing the
wrong thing.
Here is the incremental patch that I'm pushing for lio-core, and will
get this fixed up in for-next shortly.
Thanks,
--nab
commit 742c0d2e4436c7bc3c62c07dd838c7f52b3ccdcf
Author: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
Date: Sat Feb 25 12:18:48 2012 -0800
iscsi-target: Fix iscsit_alloc_buffs() failure cases
Make iscsit_alloc_buffs() failure case for page_alloc_failed use correct
__free_page() SGL pointer, and return -ENOMEM for iscsit_allocate_iovecs
failure to push se_cmd->t_mem_sg release into iscsit_release_cmd()
callback during iscsit_add_reject_from_cmd() connection reset.
Also drop cmd->t_mem_sg = NULL assignment from page_alloc_failed
failure case.
Cc: Roland Dreier <roland@xxxxxxxxxxxxxxx>
Cc: Andy Grover <agrover@xxxxxxxxxx>
Signed-off-by: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 3e18efd..eb25737 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -780,7 +780,7 @@ static int iscsit_alloc_buffs(struct iscsi_cmd *cmd)
struct scatterlist *sgl;
u32 length = cmd->se_cmd.data_length;
int nents = DIV_ROUND_UP(length, PAGE_SIZE);
- int i = 0, ret;
+ int i = 0, j = 0, ret;
/*
* If no SCSI payload is present, allocate the default iovecs used for
* iSCSI PDU Header
@@ -821,17 +821,15 @@ static int iscsit_alloc_buffs(struct iscsi_cmd *cmd)
*/
ret = iscsit_allocate_iovecs(cmd);
if (ret < 0)
- goto page_alloc_failed;
+ return -ENOMEM;
return 0;
page_alloc_failed:
- while (i >= 0) {
- __free_page(sg_page(&sgl[i]));
- i--;
- }
+ while (j < i)
+ __free_page(sg_page(&sgl[j++]));
+
kfree(sgl);
- cmd->t_mem_sg = NULL;
return -ENOMEM;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
