- Fixes memory leaks in scst_user spotted by new SGV cache backend

- Version changed on -pre3 - Minor fixes and cosmetics git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@199 d57e44dd-8a1f-0410-8b47-8ef2f437770f
2026-05-14 09:11:27 +00:00 · 2007-09-25 09:52:53 +00:00
parent a61f896dd3
commit 455778bf10
5 changed files with 43 additions and 39 deletions
--- a/scst/include/scsi_tgt.h
+++ b/scst/include/scsi_tgt.h
@@ -39,7 +39,7 @@
 /* Version numbers, the same as for the kernel */
 #define SCST_VERSION_CODE 0x000906
 #define SCST_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))
-#define SCST_VERSION_STRING "0.9.6-pre2"
+#define SCST_VERSION_STRING "0.9.6-pre3"

 /*************************************************************
 ** States of command processing state machine
--- a/scst/src/dev_handlers/scst_user.c
+++ b/scst/src/dev_handlers/scst_user.c
@@ -361,7 +361,7 @@ static void dev_user_on_cached_mem_free(struct dev_user_cmd *ucmd)
 {
 	TRACE_ENTRY();

-	TRACE_DBG("Preparing ON_CACHED_MEM_FREE (ucmd %p, h %d, ubuff %lx)",
+	TRACE_MEM("Preparing ON_CACHED_MEM_FREE (ucmd %p, h %d, ubuff %lx)",
 		ucmd, ucmd->h, ucmd->ubuff);

 	ucmd->user_cmd.cmd_h = ucmd->h;
@@ -400,18 +400,14 @@ static void dev_user_unmap_buf(struct dev_user_cmd *ucmd)
 	return;
 }

-static void dev_user_free_sg_entries(struct scatterlist *sg, int sg_count,
-	void *priv)
+static void __dev_user_free_sg_entries(struct dev_user_cmd *ucmd)
 {
-	struct dev_user_cmd *ucmd = (struct dev_user_cmd*)priv;
-
 	TRACE_ENTRY();

 	sBUG_ON(ucmd->data_pages == NULL);

-	TRACE_MEM("Freeing data pages (ucmd=%p, ubuff=%lx, sg=%p, sg_count=%d, "
-		"buff_cached=%d)", ucmd, ucmd->ubuff, sg, sg_count,
-		ucmd->buff_cached);
+	TRACE_MEM("Freeing data pages (ucmd=%p, ubuff=%lx, buff_cached=%d)",
+		ucmd, ucmd->ubuff, ucmd->buff_cached);

 	dev_user_unmap_buf(ucmd);

@@ -424,6 +420,19 @@ static void dev_user_free_sg_entries(struct scatterlist *sg, int sg_count,
 	return;
 }

+static void dev_user_free_sg_entries(struct scatterlist *sg, int sg_count,
+	void *priv)
+{
+	struct dev_user_cmd *ucmd = (struct dev_user_cmd*)priv;
+
+	TRACE_MEM("Freeing data pages (sg=%p, sg_count=%d, priv %p)", sg,
+		sg_count, ucmd);
+
+	__dev_user_free_sg_entries(ucmd);
+
+	return;
+}
+
 static inline int is_buff_cached(struct dev_user_cmd *ucmd)
 {
 	int mem_reuse_type = ucmd->dev->memory_reuse_type;
@@ -510,9 +519,8 @@ static int dev_user_alloc_sg(struct dev_user_cmd *ucmd, int cached_buff)
 					cmd->tgt->sg_tablesize);
 				ll++;
 			}
-			sgv_pool_free(ucmd->sgv);
-			ucmd->sgv = NULL;
 			cmd->sg = NULL;
+			/* sgv will be freed in dev_user_free_sgv() */
 			res = -1;
 		}
 	} else {
@@ -520,9 +528,9 @@ static int dev_user_alloc_sg(struct dev_user_cmd *ucmd, int cached_buff)
 			"sg_cnt %d, ubuff %lx, sgv %p", ucmd, ucmd->h,
 			ucmd->buff_cached, cmd->sg_cnt,	ucmd->ubuff, ucmd->sgv);
 		if (unlikely(cmd->sg_cnt == 0)) {
+			TRACE_MEM("Refused allocation (ucmd %p)", ucmd);
+			sBUG_ON(ucmd->sgv != NULL);
 			res = -1;
-			if (ucmd->data_pages != NULL)
-				dev_user_unmap_buf(ucmd);
 		} else {
 			switch(ucmd->state & ~UCMD_STATE_MASK) {
 			case UCMD_STATE_BUF_ALLOCING:
@@ -530,8 +538,6 @@ static int dev_user_alloc_sg(struct dev_user_cmd *ucmd, int cached_buff)
 				break;
 			case UCMD_STATE_EXECING:
 				res = -1;
-				if (ucmd->data_pages != NULL)
-					dev_user_unmap_buf(ucmd);
 				break;
 			default:
 				sBUG();
@@ -810,7 +816,12 @@ static void dev_user_free_sgv(struct dev_user_cmd *ucmd)
 	if (ucmd->sgv != NULL) {
 		sgv_pool_free(ucmd->sgv);
 		ucmd->sgv = NULL;
+	} else if (ucmd->data_pages != NULL) {
+		/* We mapped pages, but for some reason didn't allocate them */
+		ucmd_get(ucmd, 0);
+		__dev_user_free_sg_entries(ucmd);
 	}
+	return;
 }

 static void dev_user_on_free_cmd(struct scst_cmd *cmd)
@@ -822,7 +833,7 @@ static void dev_user_on_free_cmd(struct scst_cmd *cmd)
 	if (unlikely(ucmd == NULL))
 		goto out;

-	TRACE_DBG("ucmd %p, cmd %p, buff_cached %d, ubuff %lx", ucmd, ucmd->cmd,
+	TRACE_MEM("ucmd %p, cmd %p, buff_cached %d, ubuff %lx", ucmd, ucmd->cmd,
 		ucmd->buff_cached, ucmd->ubuff);

 	ucmd->cmd = NULL;
@@ -998,6 +1009,8 @@ static int dev_user_map_buf(struct dev_user_cmd *ucmd, unsigned long ubuff,
 	if (unlikely(ubuff == 0))
 		goto out_nomem;

+	sBUG_ON(ucmd->data_pages != NULL);
+
 	ucmd->num_data_pages = num_pg;

 	ucmd->data_pages = kzalloc(sizeof(*ucmd->data_pages)*ucmd->num_data_pages,
--- a/scst/src/scst_mem.c
+++ b/scst/src/scst_mem.c
@@ -503,6 +503,8 @@ struct scatterlist *sgv_pool_alloc(struct sgv_pool *pool, unsigned int size,
 	int no_cached = flags & SCST_POOL_ALLOC_NO_CACHED;
 	int no_fail = ((gfp_mask & __GFP_NOFAIL) == __GFP_NOFAIL);

+	TRACE_ENTRY();
+
 	sBUG_ON(size == 0);

 	pages = ((size + PAGE_SIZE - 1) >> PAGE_SHIFT);
@@ -519,13 +521,8 @@ struct scatterlist *sgv_pool_alloc(struct sgv_pool *pool, unsigned int size,
 		EXTRACHECKS_BUG_ON(obj->sg_count != 0);
 		pages_to_alloc = (1 << order);
 		cache = pool->caches[obj->order];
-		if (sgv_pool_hiwmk_check(pages_to_alloc, no_fail) != 0) {
-			obj->sg_count = 0;
-			if ((flags & SCST_POOL_RETURN_OBJ_ON_ALLOC_FAIL))
-				goto out_return1;
-			else
-				goto out_fail_free_sg_entries;
-		}
+		if (sgv_pool_hiwmk_check(pages_to_alloc, no_fail) != 0)
+			goto out_fail_free_sg_entries;
 	} else if ((order < SGV_POOL_ELEMENTS) && !no_cached) {
 		cache = pool->caches[order];
 		obj = sgv_pool_cached_get(pool, order, gfp_mask);
@@ -579,13 +576,8 @@ struct scatterlist *sgv_pool_alloc(struct sgv_pool *pool, unsigned int size,
 			goto out_return;
 			
 		obj->allocator_priv = priv;
-		if (sgv_pool_hiwmk_check(pages_to_alloc, no_fail) != 0) {
-			obj->sg_count = 0;
-			if ((flags & SCST_POOL_RETURN_OBJ_ON_ALLOC_FAIL))
-				goto out_return1;
-			else
-				goto out_fail_free_sg_entries;
-		}
+		if (sgv_pool_hiwmk_check(pages_to_alloc, no_fail) != 0)
+			goto out_fail_free_sg_entries;
 	} else {
 		int sz;
 		pages_to_alloc = pages;
@@ -604,10 +596,8 @@ struct scatterlist *sgv_pool_alloc(struct sgv_pool *pool, unsigned int size,
 		obj->sg_entries = obj->sg_entries_data;
 		obj->allocator_priv = priv;
 		
-		if (sgv_pool_hiwmk_check(pages_to_alloc, no_fail) != 0) {
-			obj->sg_count = 0;
+		if (sgv_pool_hiwmk_check(pages_to_alloc, no_fail) != 0)
 			goto out_fail_free_sg_entries;
-		}
 		TRACE_MEM("Big or no_cached sgv_obj %p (size %d)", obj,	sz);		
 	}

@@ -662,6 +652,7 @@ success:
 		obj->sg_count, *count, obj->sg_entries[obj->orig_sg].length);

 out:
+	TRACE_EXIT_HRES(res);
 	return res;

 out_return:
--- a/usr/fileio/common.c
+++ b/usr/fileio/common.c
@@ -663,9 +663,9 @@ void *main_loop(void *arg)

 	vcmd.fd = open_dev_fd(dev);
 	if (vcmd.fd < 0) {
-		res = vcmd.fd;
+		res = -errno;
 		PRINT_ERROR_PR("Unable to open file %s (%s)", dev->file_name,
-			strerror(res));
+			strerror(-res));
 		goto out;
 	}

--- a/usr/fileio/fileio.c
+++ b/usr/fileio/fileio.c
@@ -277,9 +277,9 @@ int main(int argc, char **argv)
 	TRACE_DBG("Opening file %s", dev.file_name);
 	fd = open(dev.file_name, O_RDONLY|O_LARGEFILE);
 	if (fd < 0) {
-		res = errno;
+		res = -errno;
 		PRINT_ERROR_PR("Unable to open file %s (%s)", dev.file_name,
-			strerror(res));
+			strerror(-res));
 		goto out_done;
 	}

@@ -368,9 +368,9 @@ int main(int argc, char **argv)
 	dev.scst_usr_fd = open(DEV_USER_PATH DEV_USER_NAME, O_RDWR | 
 		(dev.non_blocking ? O_NONBLOCK : 0));
 	if (dev.scst_usr_fd < 0) {
-		res = dev.scst_usr_fd;
+		res = -errno;
 		PRINT_ERROR_PR("Unable to open SCST device %s (%s)",
-			DEV_USER_PATH DEV_USER_NAME, strerror(res));
+			DEV_USER_PATH DEV_USER_NAME, strerror(-res));
 		goto out_done;
 	}