Bug 531932

*** Bug 532460 has been marked as a duplicate of this bug. ***

Also affects booting with "drm.debug=1"

note that 'radeon.nomodeset=1' is an invalid parameter, you want either 'nomodeset' or 'radeon.modeset=0'. 'drm.debug=1' is valid, however, so the validity of the parameter doesn't seem to be the problem. I wonder if this happens with _all_ kernel parameters, or just ones of the module.parameter=value format which is used for passing parameters to modules...

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

(In reply to comment #3)
> I wonder if this
> happens with _all_ kernel parameters, or just ones of the
> module.parameter=value format which is used for passing parameters to
> modules...

Odd, I tried booting with "foo.bar=1" and that works.  However, "drm.debug=1" and "radeon.modeset=0" both result in a SIGSEGV.

I think the cause of this problem is in the addOption() function in loader/modules.

It was changed about a month ago (29e18c35d264f93acb0bd552c287acfb753c4afb) and the current revision does not initialize modopts[i].options to anything in case the module was not found in modopts table.

When I revert the change the loader does not crash. There of course must be a reason for the previous patch, someone should contact the submitter. Unfortunately it's getting late in Brno, the bug is a blocker and I was instructed to it this over.

For the record, it was a dcantrell change, the commit log reads:

"Do not assume we found a module in addOption() in loader/modules.c



Avoid SIGSEGV in addOption() in loader/modules.c.  Just because found

does not equal 0 does not mean we found the module requested."

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

modopts was not initialized under certain conditions, such as passing "radeon.nomodeset=1".  Fix:

diff --git a/loader/modules.c b/loader/modules.c
index d5129b7..f61f680 100644
--- a/loader/modules.c
+++ b/loader/modules.c
@@ -117,16 +117,18 @@ static void addOption(const char *module, const char *opti
     }
 
     if (found) {
-        modopts[i].options = realloc(modopts[i].options,
-                                     sizeof(modopts[i].options) *
-                                     (modopts[i].numopts + 1));
-        modopts[i].options[modopts[i].numopts - 1] = strdup(option);
-        modopts[i].options[modopts[i].numopts] = NULL;
-    } else {
-        modopts = realloc(modopts, sizeof(*modopts) * (nummodopts + 1));
-        modopts[nummodopts].name = strdup(module);
-        modopts[nummodopts].numopts = 1;
-        modopts[nummodopts++].options = NULL;
+        if (modopts == NULL) {
+            modopts = realloc(modopts, sizeof(*modopts) * (nummodopts + 1));
+            modopts[nummodopts].name = strdup(module);
+            modopts[nummodopts].numopts = 1;
+            modopts[nummodopts++].options = NULL;
+        } else {
+            modopts[i].options = realloc(modopts[i].options,
+                                         sizeof(modopts[i].options) *
+                                         (modopts[i].numopts + 1));
+            modopts[i].options[modopts[i].numopts - 1] = strdup(option);
+            modopts[i].options[modopts[i].numopts] = NULL;
+        }
     }
 
     return;

Tag request:

https://fedorahosted.org/rel-eng/ticket/3101

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

this was tagged, jlaska will test to confirm the fix and close it off.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Confirmed the fix using anaconda-12.44-1.fc12.  Tested the following boot args:
 * radeon.modeset=0
 * foo.bar=1
 * nomodeset
 * foo.bar.baz.radeon=1
 * radeon...modeset=0

Moving this issue to CLOSED RAWHIDE.

A point was raised on the anaconda team about my initial fix.  It does prevent SIGSEGV, but it also never collects module options, effectively ignoring them all.  I've redone the patch and would like it retested for F-12:

diff --git a/loader/modules.c b/loader/modules.c
index f61f680..fbdd0c9 100644
--- a/loader/modules.c
+++ b/loader/modules.c
@@ -105,7 +105,7 @@ void mlAddBlacklist(char *module) {
 }
 
 static void addOption(const char *module, const char *option) {
-    int found = 0, i;
+    int found = 0, i, sz;
 
     found = 0;
     for (i = 0; i < nummodopts; i++) {
@@ -117,18 +117,41 @@ static void addOption(const char *module, const char *option) {
     }
 
     if (found) {
+        modopts[i].options = realloc(modopts[i].options,
+                                     sizeof(modopts[i].options) *
+                                     (modopts[i].numopts + 1));
+        if (modopts[i].options == NULL) {
+            logMessage(ERROR, "%s (%d): %m", __func__, __LINE__);
+            abort();
+        }
+
+        modopts[i].options[modopts[i].numopts - 1] = strdup(option);
+        modopts[i].options[modopts[i].numopts] = NULL;
+    } else {
         if (modopts == NULL) {
-            modopts = realloc(modopts, sizeof(*modopts) * (nummodopts + 1));
-            modopts[nummodopts].name = strdup(module);
-            modopts[nummodopts].numopts = 1;
-            modopts[nummodopts++].options = NULL;
+            modopts = malloc(sizeof(struct moduleOptions) * (nummodopts + 1));
         } else {
-            modopts[i].options = realloc(modopts[i].options,
-                                         sizeof(modopts[i].options) *
-                                         (modopts[i].numopts + 1));
-            modopts[i].options[modopts[i].numopts - 1] = strdup(option);
-            modopts[i].options[modopts[i].numopts] = NULL;
+            modopts = realloc(modopts, sizeof(*modopts) * (nummodopts + 1));
         }
+
+        if (modopts == NULL) {
+            logMessage(ERROR, "%s (%d): %m", __func__, __LINE__);
+            abort();
+        }
+
+        modopts[nummodopts].name = strdup(module);
+        modopts[nummodopts].numopts = 1;
+
+        sz = sizeof(modopts[nummodopts].options) * 2;
+        if ((modopts[nummodopts].options = malloc(sz)) == NULL) {
+            logMessage(ERROR, "%s (%d): %m", __func__, __LINE__);
+            abort();
+        }
+
+        modopts[nummodopts].options[0] = strdup(option);
+        modopts[nummodopts].options[1] = NULL;
+
+        nummodopts++;
     }
 
     return;

Retested using http://jkeating.fedorapeople.org/boot.iso.

Tested the same boot arguments as tested before.  However, this time confirmed expected results with respect to modesetting.  The attached screenshot resolutions match what I would expect for modesetting being enabled and disabled.

 * radeon.modeset=0 - http://jlaska.fedorapeople.org/radeon.modeset=0.jpg
 * radeon.modeset=1 - http://jlaska.fedorapeople.org/radeon.modeset=1.jpg
 * nomodeset - http://jlaska.fedorapeople.org/nomodeset.jpg

In addition, the following boot arguments did not result in a sigsegv of loader.
 * foo.bar=1 
 * foo.bar.baz.radeon=1
 * radeon...modeset=0
 * asdf...foo==42

Looks good to me too. Confirmed that nouveau.modeset works as expected, also tried feeding all kinds of garbage in an attempt to break it and couldn't, despite going well into Doctor It Hurts territory. Unless we missed something, looks good to me.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers