Re: Concerns in kern/subr_kobj.c

[Paul Goyette <paul%whooppee.com@localhost>]

On Mon, 17 Apr 2017, Paul Goyette wrote:

> While perusing the code, I noticed some possible issues:
>
> 1. In kobj_unload() the calls to kobj_machdep() for data and rodata
>   sections are conditional on the appropriate ko->ko_xxx_address being
>   non-zero, yet the corresponding call for the text section is
>   unconditional.
>
>   But, from code just a little bit further down, there is a check
>   for the ko->ko_text_address being 0 before calling uvm_km_free()
>   on the text section.
>
>   It seems to me that both calls should be conditional, or both
>   should be unconditional.
>
> 2. In kobj_affix(), the calls to kobj_machdep() for data and rodata
>   sections discard any possible error value from previous sections.
>   So, if kobj_machdep() fails for the text section, but succeeds for
>   the data or rodata sections, the rest of the code is unaware of
>   any error having occurred.
>
>   In particular, the trailing call to kobj_unload() would not happen;
>   instead the code would simply proceed to update the VM protection
>   bits for the text and rodata sections.

I'm proposing the attached diffs to address these issues.  Does anyone 
see any reason why these should not be committed?



+------------------+--------------------------+----------------------------+
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:          |
| (Retired)        | FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com   |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org |
+------------------+--------------------------+----------------------------+

Index: subr_kobj.c
===================================================================
RCS file: /cvsroot/src/sys/kern/subr_kobj.c,v
retrieving revision 1.59
diff -u -p -r1.59 subr_kobj.c
--- subr_kobj.c	2 Aug 2016 12:23:08 -0000	1.59
+++ subr_kobj.c	17 Apr 2017 11:32:03 -0000
@@ -646,11 +646,13 @@ kobj_unload(kobj_t ko)
 	 * Notify MD code that a module has been unloaded.
 	 */
 	if (ko->ko_loaded) {
-		error = kobj_machdep(ko, (void *)ko->ko_text_address,
-		    ko->ko_text_size, false);
-		if (error != 0)
-			kobj_error(ko, "machine dependent deinit failed (text) %d",
-			    error);
+		if (ko->ko_text_address != 0) {
+			error = kobj_machdep(ko, (void *)ko->ko_text_address,
+			    ko->ko_text_size, false);
+			if (error != 0)
+				kobj_error(ko, "machine dependent deinit failed"
+			    " (text) %d", error);
+		}
 
 		if (ko->ko_data_address != 0) {
 			error = kobj_machdep(ko, (void *)ko->ko_data_address,
@@ -763,12 +765,16 @@ kobj_affix(kobj_t ko, const char *name)
 	 * Most architectures use this opportunity to flush their caches.
 	 */
 	if (error == 0) {
-		error = kobj_machdep(ko, (void *)ko->ko_text_address,
-		    ko->ko_text_size, true);
-		if (error != 0)
-			kobj_error(ko, "machine dependent init failed (text) %d",
-			    error);
+		if (ko->ko_text_address != 0) {
+			error = kobj_machdep(ko, (void *)ko->ko_text_address,
+			    ko->ko_text_size, true);
+			if (error != 0)
+				kobj_error(ko, "machine dependent init failed"
+				    " (text) %d", error);
+		}
+	}
 
+	if (error == 0) {
 		if (ko->ko_data_address != 0) {
 			error = kobj_machdep(ko, (void *)ko->ko_data_address,
 			    ko->ko_data_size, true);
@@ -776,7 +782,9 @@ kobj_affix(kobj_t ko, const char *name)
 				kobj_error(ko, "machine dependent init failed"
 				    "(data) %d", error);
 		}
+	}
 
+	if (error == 0) {
 		if (ko->ko_rodata_address != 0) {
 			error = kobj_machdep(ko, (void *)ko->ko_rodata_address,
 			    ko->ko_rodata_size, true);