Skip to contentSkip to conversation

Unsafe MODULE_ usage in crc32.c

PATCH In review Matt Reppert 4 participants 4 messages Dec 1, 2002 – Dec 2, 2002 Library code

lore
Share & tools
lore

Matt Reppert

author
#0 lore raw

PATCH 1/1 Unsafe MODULE_ usage in crc32.c

Hi,

Okay, I know, it's just a library module, doesn't need to ever be unloaded anyway. But error noise in dmesg annoys me, hence this patch.

Matt

Convert CRC32 to try_module_get; fixes an unsafe usage that prevents unloading.

 lib/crc32.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletion(-)
lib/crc32.c renamed from lib/crc32.c~crc32-unsafe +4 -1
@@ -551,7 +551,10 @@ static int __init init_crc32(void)
 	rc1 = crc32init_le();
 	rc2 = crc32init_be();
 	rc = rc1 || rc2;
-	if (!rc) MOD_INC_USE_COUNT;
+	if (!rc) {
+		if (!try_module_get(THIS_MODULE))
+			rc = -1;
+	}
 	return rc;
 }

[patch ends]

#1 lore raw
Quoted 4 lines · Matt Reppert

Hi,

Okay, I know, it's just a library module, doesn't need to ever be unloaded anyway. But error noise in dmesg annoys me, hence this patch.

Better asking Rusty, why module cannot call MOD_INC_USE_COUNT on itself during its own init... I'm pretty sure that crc32 module knows that nobody can unload it at this point: it is executing its own code, isn't it?
Petr Vandrovec [email protected]

Quoted 30 lines

Matt

Convert CRC32 to try_module_get; fixes an unsafe usage that prevents unloading.

 lib/crc32.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletion(-)
lib/crc32.c renamed from lib/crc32.c~crc32-unsafe +4 -1
@@ -551,7 +551,10 @@ static int __init init_crc32(void)
 	rc1 = crc32init_le();
 	rc2 = crc32init_be();
 	rc = rc1 || rc2;
-	if (!rc) MOD_INC_USE_COUNT;
+	if (!rc) {
+		if (!try_module_get(THIS_MODULE))
+			rc = -1;
+	}
 	return rc;
 }

[patch ends]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#2 lore raw
In reply to Matt Reppert
Quoted 2 lines · Matt Reppert

Okay, I know, it's just a library module, doesn't need to ever be unloaded anyway. But error noise in dmesg annoys me, hence this patch.

I don't think you even need to set the use count at all for crc32:
As long as another module is using it, you can't unload it because the exported symbols are used. When those symbols are not known to other modules, it is also safe to unload crc32.

I noticed another small problem with init_crc: if crc32init_be() fails, the memory allocated by crc32init_le() is never freed, see below.

	Arnd <><
lib/crc32.c +7 -5
@@ -547,11 +547,13 @@
  */
 static int __init init_crc32(void)
 {
-	int rc1, rc2, rc;
-	rc1 = crc32init_le();
-	rc2 = crc32init_be();
-	rc = rc1 || rc2;
-	if (!rc) MOD_INC_USE_COUNT;
+	int rc;
+	rc = crc32init_le();
+	if (rc)
+		return rc;
+	rc = crc32init_be();
+	if (rc)
+		crc32cleanup_le();
 	return rc;
 }
#3 lore raw
Quoted 3 lines · Arnd Bergmann

I noticed another small problem with init_crc: if crc32init_be() fails, the memory allocated by crc32init_le() is never freed, see below.

The -ac tree solves that by compiling in the table. Saves on boot up time too 8)