Re: Concerns in kern/subr_kobj.c

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

On Mon, 17 Apr 2017, Christos Zoulas wrote:

> In article <Pine.NEB.4.64.1704171932080.13026%speedy.whooppee.com@localhost>,
> Paul Goyette  <paul%whooppee.com@localhost> wrote:
> > -=-=-=-=-=-
> >
> > 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?
>
> How about if (error == 0 && kobj->ko_foo_address) to avoid the extra
> level of indentation?

Yeah, looks better that way.  Revised diffs attached.


+------------------+--------------------------+----------------------------+
| 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 22:36:41 -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,
@@ -762,36 +764,37 @@ kobj_affix(kobj_t ko, const char *name)
 	 *
 	 * Most architectures use this opportunity to flush their caches.
 	 */
-	if (error == 0) {
+	if (error == 0 && 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 (ko->ko_data_address != 0) {
-			error = kobj_machdep(ko, (void *)ko->ko_data_address,
-			    ko->ko_data_size, true);
-			if (error != 0)
-				kobj_error(ko, "machine dependent init failed"
-				    "(data) %d", error);
-		}
+			kobj_error(ko, "machine dependent init failed (text)"
+			    " %d", error);
+	}
 
-		if (ko->ko_rodata_address != 0) {
-			error = kobj_machdep(ko, (void *)ko->ko_rodata_address,
-			    ko->ko_rodata_size, true);
-			if (error != 0)
-				kobj_error(ko, "machine dependent init failed"
-				    "(rodata) %d", error);
-		}
+	if (error == 0 && ko->ko_data_address != 0) {
+		error = kobj_machdep(ko, (void *)ko->ko_data_address,
+		    ko->ko_data_size, true);
+		if (error != 0)
+			kobj_error(ko, "machine dependent init failed (data)"
+			    " %d", error);
+	}
 
-		ko->ko_loaded = true;
+	if (error == 0 ko->ko_rodata_address != 0) {
+		error = kobj_machdep(ko, (void *)ko->ko_rodata_address,
+		    ko->ko_rodata_size, true);
+		if (error != 0)
+			kobj_error(ko, "machine dependent init failed (rodata)"
+			    " %d", error);
 	}
 
 	if (error == 0) {
+		ko->ko_loaded = true;
+
 		/* Change the memory protections, when needed. */
-		uvm_km_protect(module_map, ko->ko_text_address,
-		     ko->ko_text_size, VM_PROT_READ|VM_PROT_EXECUTE);
+		if (ko->ko_text_address != 0) {
+			uvm_km_protect(module_map, ko->ko_text_address,
+			     ko->ko_text_size, VM_PROT_READ|VM_PROT_EXECUTE);
 		if (ko->ko_rodata_address != 0) {
 			uvm_km_protect(module_map, ko->ko_rodata_address,
 			    ko->ko_rodata_size, VM_PROT_READ);