tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Concerns in kern/subr_kobj.c



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);


Home | Main Index | Thread Index | Old Index