module: refactor load_module part 2

Here's a second one. It's slightly less trivial - since we now have error cases - and equally untested so it may well be totally broken. But it also cleans up a bit more, and avoids one of the goto targets, because the "move_module()" helper now does both allocations or none. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

module: refactor load_module part 2
Here's a second one. It's slightly less trivial - since we now have error cases - and equally untested so it may well be totally broken. But it also cleans up a bit more, and avoids one of the goto targets, because the "move_module()" helper now does both allocations or none. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Linus Torvalds · Rusty Russell
1 parent f91a13bb99
Showing 1 changed file with 72 additions and 57 deletions Side-by-side Diff
kernel/module.c
@@ -2082,7 +2082,8 @@
  
 #ifdef CONFIG_DEBUG_KMEMLEAK
 static void kmemleak_load_module(struct module *mod, Elf_Ehdr *hdr,
-				 Elf_Shdr *sechdrs, char *secstrings)
+				 const Elf_Shdr *sechdrs,
+				 const char *secstrings)
 {
 	unsigned int i;
  
@@ -2102,7 +2103,8 @@
 }
 #else
 static inline void kmemleak_load_module(struct module *mod, Elf_Ehdr *hdr,
-					Elf_Shdr *sechdrs, char *secstrings)
+					Elf_Shdr *sechdrs,
+					const char *secstrings)
 {
 }
 #endif
@@ -2172,6 +2174,70 @@
 #endif
 }
  
+static struct module *move_module(struct module *mod,
+				  Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
+				  const char *secstrings, unsigned modindex)
+{
+	int i;
+	void *ptr;
+
+	/* Do the allocs. */
+	ptr = module_alloc_update_bounds(mod->core_size);
+	/*
+	 * The pointer to this block is stored in the module structure
+	 * which is inside the block. Just mark it as not being a
+	 * leak.
+	 */
+	kmemleak_not_leak(ptr);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	memset(ptr, 0, mod->core_size);
+	mod->module_core = ptr;
+
+	ptr = module_alloc_update_bounds(mod->init_size);
+	/*
+	 * The pointer to this block is stored in the module structure
+	 * which is inside the block. This block doesn't need to be
+	 * scanned as it contains data and code that will be freed
+	 * after the module is initialized.
+	 */
+	kmemleak_ignore(ptr);
+	if (!ptr && mod->init_size) {
+		module_free(mod, mod->module_core);
+		return ERR_PTR(-ENOMEM);
+	}
+	memset(ptr, 0, mod->init_size);
+	mod->module_init = ptr;
+
+	/* Transfer each section which specifies SHF_ALLOC */
+	DEBUGP("final section addresses:\n");
+	for (i = 0; i < hdr->e_shnum; i++) {
+		void *dest;
+
+		if (!(sechdrs[i].sh_flags & SHF_ALLOC))
+			continue;
+
+		if (sechdrs[i].sh_entsize & INIT_OFFSET_MASK)
+			dest = mod->module_init
+				+ (sechdrs[i].sh_entsize & ~INIT_OFFSET_MASK);
+		else
+			dest = mod->module_core + sechdrs[i].sh_entsize;
+
+		if (sechdrs[i].sh_type != SHT_NOBITS)
+			memcpy(dest, (void *)sechdrs[i].sh_addr,
+			       sechdrs[i].sh_size);
+		/* Update sh_addr to point to copy in image. */
+		sechdrs[i].sh_addr = (unsigned long)dest;
+		DEBUGP("\t0x%lx %s\n",
+		       sechdrs[i].sh_addr, secstrings + sechdrs[i].sh_name);
+	}
+	/* Module has been moved. */
+	mod = (void *)sechdrs[modindex].sh_addr;
+	kmemleak_load_module(mod, hdr, sechdrs, secstrings);
+	return mod;
+}
+
 /* Allocate and load the module: note that size of section 0 is always
    zero, and we rely on this for optional sections. */
 static noinline struct module *load_module(void __user *umod,
@@ -2188,7 +2254,6 @@
 	unsigned int modindex, versindex, infoindex, pcpuindex;
 	struct module *mod;
 	long err = 0;
-	void *ptr = NULL; /* Stops spurious gcc warning */
 	unsigned long symoffs, stroffs, *strmap;
 	void __percpu *percpu;
 	struct _ddebug *debug = NULL;
  
  
@@ -2342,62 +2407,13 @@
 	symoffs = layout_symtab(mod, sechdrs, symindex, strindex, hdr,
 				secstrings, &stroffs, strmap);
  
-	/* Do the allocs. */
-	ptr = module_alloc_update_bounds(mod->core_size);
-	/*
-	 * The pointer to this block is stored in the module structure
-	 * which is inside the block. Just mark it as not being a
-	 * leak.
-	 */
-	kmemleak_not_leak(ptr);
-	if (!ptr) {
-		err = -ENOMEM;
+	/* Allocate and move to the final place */
+	mod = move_module(mod, hdr, sechdrs, secstrings, modindex);
+	if (IS_ERR(mod)) {
+		err = PTR_ERR(mod);
 		goto free_percpu;
 	}
-	memset(ptr, 0, mod->core_size);
-	mod->module_core = ptr;
  
-	ptr = module_alloc_update_bounds(mod->init_size);
-	/*
-	 * The pointer to this block is stored in the module structure
-	 * which is inside the block. This block doesn't need to be
-	 * scanned as it contains data and code that will be freed
-	 * after the module is initialized.
-	 */
-	kmemleak_ignore(ptr);
-	if (!ptr && mod->init_size) {
-		err = -ENOMEM;
-		goto free_core;
-	}
-	memset(ptr, 0, mod->init_size);
-	mod->module_init = ptr;
-
-	/* Transfer each section which specifies SHF_ALLOC */
-	DEBUGP("final section addresses:\n");
-	for (i = 0; i < hdr->e_shnum; i++) {
-		void *dest;
-
-		if (!(sechdrs[i].sh_flags & SHF_ALLOC))
-			continue;
-
-		if (sechdrs[i].sh_entsize & INIT_OFFSET_MASK)
-			dest = mod->module_init
-				+ (sechdrs[i].sh_entsize & ~INIT_OFFSET_MASK);
-		else
-			dest = mod->module_core + sechdrs[i].sh_entsize;
-
-		if (sechdrs[i].sh_type != SHT_NOBITS)
-			memcpy(dest, (void *)sechdrs[i].sh_addr,
-			       sechdrs[i].sh_size);
-		/* Update sh_addr to point to copy in image. */
-		sechdrs[i].sh_addr = (unsigned long)dest;
-		DEBUGP("\t0x%lx %s\n",
-		       sechdrs[i].sh_addr, secstrings + sechdrs[i].sh_name);
-	}
-	/* Module has been moved. */
-	mod = (void *)sechdrs[modindex].sh_addr;
-	kmemleak_load_module(mod, hdr, sechdrs, secstrings);
-
 #if defined(CONFIG_MODULE_UNLOAD)
 	mod->refptr = alloc_percpu(struct module_ref);
 	if (!mod->refptr) {
@@ -2580,7 +2596,6 @@
  free_init:
 #endif
 	module_free(mod, mod->module_init);
- free_core:
 	module_free(mod, mod->module_core);
 	/* mod will be freed with core. Don't access it beyond this line! */
  free_percpu:
...	...	@@ -2082,7 +2082,8 @@
2082	2082
2083	2083	#ifdef CONFIG_DEBUG_KMEMLEAK
2084	2084	static void kmemleak_load_module(struct module mod, Elf_Ehdr hdr,
2085		- Elf_Shdr sechdrs, char secstrings)
	2085	+ const Elf_Shdr *sechdrs,
	2086	+ const char *secstrings)
2086	2087	{
2087	2088	unsigned int i;
2088	2089
...	...	@@ -2102,7 +2103,8 @@
2102	2103	}
2103	2104	#else
2104	2105	static inline void kmemleak_load_module(struct module mod, Elf_Ehdr hdr,
2105		- Elf_Shdr sechdrs, char secstrings)
	2106	+ Elf_Shdr *sechdrs,
	2107	+ const char *secstrings)
2106	2108	{
2107	2109	}
2108	2110	#endif
...	...	@@ -2172,6 +2174,70 @@
2172	2174	#endif
2173	2175	}
2174	2176
	2177	+static struct module move_module(struct module mod,
	2178	+ Elf_Ehdr hdr, Elf_Shdr sechdrs,
	2179	+ const char *secstrings, unsigned modindex)
	2180	+{
	2181	+ int i;
	2182	+ void *ptr;
	2183	+
	2184	+ /* Do the allocs. */
	2185	+ ptr = module_alloc_update_bounds(mod->core_size);
	2186	+ /*
	2187	+ * The pointer to this block is stored in the module structure
	2188	+ * which is inside the block. Just mark it as not being a
	2189	+ * leak.
	2190	+ */
	2191	+ kmemleak_not_leak(ptr);
	2192	+ if (!ptr)
	2193	+ return ERR_PTR(-ENOMEM);
	2194	+
	2195	+ memset(ptr, 0, mod->core_size);
	2196	+ mod->module_core = ptr;
	2197	+
	2198	+ ptr = module_alloc_update_bounds(mod->init_size);
	2199	+ /*
	2200	+ * The pointer to this block is stored in the module structure
	2201	+ * which is inside the block. This block doesn't need to be
	2202	+ * scanned as it contains data and code that will be freed
	2203	+ * after the module is initialized.
	2204	+ */
	2205	+ kmemleak_ignore(ptr);
	2206	+ if (!ptr && mod->init_size) {
	2207	+ module_free(mod, mod->module_core);
	2208	+ return ERR_PTR(-ENOMEM);
	2209	+ }
	2210	+ memset(ptr, 0, mod->init_size);
	2211	+ mod->module_init = ptr;
	2212	+
	2213	+ /* Transfer each section which specifies SHF_ALLOC */
	2214	+ DEBUGP("final section addresses:\n");
	2215	+ for (i = 0; i < hdr->e_shnum; i++) {
	2216	+ void *dest;
	2217	+
	2218	+ if (!(sechdrs[i].sh_flags & SHF_ALLOC))
	2219	+ continue;
	2220	+
	2221	+ if (sechdrs[i].sh_entsize & INIT_OFFSET_MASK)
	2222	+ dest = mod->module_init
	2223	+ + (sechdrs[i].sh_entsize & ~INIT_OFFSET_MASK);
	2224	+ else
	2225	+ dest = mod->module_core + sechdrs[i].sh_entsize;
	2226	+
	2227	+ if (sechdrs[i].sh_type != SHT_NOBITS)
	2228	+ memcpy(dest, (void *)sechdrs[i].sh_addr,
	2229	+ sechdrs[i].sh_size);
	2230	+ /* Update sh_addr to point to copy in image. */
	2231	+ sechdrs[i].sh_addr = (unsigned long)dest;
	2232	+ DEBUGP("\t0x%lx %s\n",
	2233	+ sechdrs[i].sh_addr, secstrings + sechdrs[i].sh_name);
	2234	+ }
	2235	+ /* Module has been moved. */
	2236	+ mod = (void *)sechdrs[modindex].sh_addr;
	2237	+ kmemleak_load_module(mod, hdr, sechdrs, secstrings);
	2238	+ return mod;
	2239	+}
	2240	+
2175	2241	/* Allocate and load the module: note that size of section 0 is always
2176	2242	zero, and we rely on this for optional sections. */
2177	2243	static noinline struct module load_module(void __user umod,
...	...	@@ -2188,7 +2254,6 @@
2188	2254	unsigned int modindex, versindex, infoindex, pcpuindex;
2189	2255	struct module *mod;
2190	2256	long err = 0;
2191		- void ptr = NULL; / Stops spurious gcc warning */
2192	2257	unsigned long symoffs, stroffs, *strmap;
2193	2258	void __percpu *percpu;
2194	2259	struct _ddebug *debug = NULL;
2195	2260
2196	2261
...	...	@@ -2342,62 +2407,13 @@
2342	2407	symoffs = layout_symtab(mod, sechdrs, symindex, strindex, hdr,
2343	2408	secstrings, &stroffs, strmap);
2344	2409
2345		- /* Do the allocs. */
2346		- ptr = module_alloc_update_bounds(mod->core_size);
2347		- /*
2348		- * The pointer to this block is stored in the module structure
2349		- * which is inside the block. Just mark it as not being a
2350		- * leak.
2351		- */
2352		- kmemleak_not_leak(ptr);
2353		- if (!ptr) {
2354		- err = -ENOMEM;
	2410	+ /* Allocate and move to the final place */
	2411	+ mod = move_module(mod, hdr, sechdrs, secstrings, modindex);
	2412	+ if (IS_ERR(mod)) {
	2413	+ err = PTR_ERR(mod);
2355	2414	goto free_percpu;
2356	2415	}
2357		- memset(ptr, 0, mod->core_size);
2358		- mod->module_core = ptr;
2359	2416
2360		- ptr = module_alloc_update_bounds(mod->init_size);
2361		- /*
2362		- * The pointer to this block is stored in the module structure
2363		- * which is inside the block. This block doesn't need to be
2364		- * scanned as it contains data and code that will be freed
2365		- * after the module is initialized.
2366		- */
2367		- kmemleak_ignore(ptr);
2368		- if (!ptr && mod->init_size) {
2369		- err = -ENOMEM;
2370		- goto free_core;
2371		- }
2372		- memset(ptr, 0, mod->init_size);
2373		- mod->module_init = ptr;
2374		-
2375		- /* Transfer each section which specifies SHF_ALLOC */
2376		- DEBUGP("final section addresses:\n");
2377		- for (i = 0; i < hdr->e_shnum; i++) {
2378		- void *dest;
2379		-
2380		- if (!(sechdrs[i].sh_flags & SHF_ALLOC))
2381		- continue;
2382		-
2383		- if (sechdrs[i].sh_entsize & INIT_OFFSET_MASK)
2384		- dest = mod->module_init
2385		- + (sechdrs[i].sh_entsize & ~INIT_OFFSET_MASK);
2386		- else
2387		- dest = mod->module_core + sechdrs[i].sh_entsize;
2388		-
2389		- if (sechdrs[i].sh_type != SHT_NOBITS)
2390		- memcpy(dest, (void *)sechdrs[i].sh_addr,
2391		- sechdrs[i].sh_size);
2392		- /* Update sh_addr to point to copy in image. */
2393		- sechdrs[i].sh_addr = (unsigned long)dest;
2394		- DEBUGP("\t0x%lx %s\n",
2395		- sechdrs[i].sh_addr, secstrings + sechdrs[i].sh_name);
2396		- }
2397		- /* Module has been moved. */
2398		- mod = (void *)sechdrs[modindex].sh_addr;
2399		- kmemleak_load_module(mod, hdr, sechdrs, secstrings);
2400		-
2401	2417	#if defined(CONFIG_MODULE_UNLOAD)
2402	2418	mod->refptr = alloc_percpu(struct module_ref);
2403	2419	if (!mod->refptr) {
...	...	@@ -2580,7 +2596,6 @@
2580	2596	free_init:
2581	2597	#endif
2582	2598	module_free(mod, mod->module_init);
2583		- free_core:
2584	2599	module_free(mod, mod->module_core);
2585	2600	/* mod will be freed with core. Don't access it beyond this line! */
2586	2601	free_percpu: