Bug 531932 - SIGSEGV booting installer with "radeon.nomodeset=1"
Summary: SIGSEGV booting installer with "radeon.nomodeset=1"
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: anaconda
Version: rawhide
Hardware: All
OS: Linux
low
high
Target Milestone: ---
Assignee: David Cantrell
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 532460 (view as bug list)
Depends On:
Blocks: F12AnacondaBlocker
TreeView+ depends on / blocked
 
Reported: 2009-10-29 21:45 UTC by John Reiser
Modified: 2009-11-07 03:37 UTC (History)
4 users (show)

Fixed In Version: anaconda-12.45-1
Clone Of:
Environment:
Last Closed: 2009-11-07 03:37:33 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description John Reiser 2009-10-29 21:45:22 UTC
Description of problem: /sbin/loader aborts installation with SIGSEGV if " radeon.nomodeset=1" is added to the boot command line for "Install or upgrade an existing system"


Version-Release number of selected component (if applicable):
anaconda-12.41

How reproducible: Every time


Steps to Reproduce:
1. boot DVD for install (today's rawhide 2009-10-29)
2. hit <Tab> on "Install or upgrade an existing system"
3. append " radeon.nomodeset=1", and boot
  
Actual results:  Installation aborts with:
Greetings.
anaconda installer init version 12.41 starting
mounting /proc filesystem... done
creating /dev filesystem... done
starting udev...done
mount /dev/pts (unix98 pty) filesystem... done
mounting /sys filesystem... done
trying to remount root filesystem read write... done
mounting /tmp as tmpfs... done
running install...
running /sbin/loader
loader received SIGSEGV!  Backtrace:
/sbin/loader[0x40783e]
/lib64/libc.so.6(+0x335f0))[0x7f7a046715f0]
/sbin/loader[0x40c2f9]
/sbin/loader[0x408819]
/lib64/libc.so.6(__libc_start_main+0xfd)[0x7f7a0465cb4d]
/sbin/loader[0x4058f9]
install extited abnormally [1/1]
you may safely reboot your system


Expected results: No SIGSEGV; successful installation using radeon driver with no kernel mode setting


Additional info: The same problem occurs on i686 and x86_64.  Install via the "basic video driver" works.

Comment 1 Chris Lumens 2009-11-02 14:12:16 UTC
*** Bug 532460 has been marked as a duplicate of this bug. ***

Comment 2 James Laska 2009-11-02 16:23:32 UTC
Also affects booting with "drm.debug=1"

Comment 3 Adam Williamson 2009-11-02 17:20:59 UTC
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

Comment 4 James Laska 2009-11-02 20:26:38 UTC
(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.

Comment 5 Ales Kozumplik 2009-11-04 18:28:41 UTC
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.

Comment 6 Adam Williamson 2009-11-04 18:40:28 UTC
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

Comment 7 David Cantrell 2009-11-04 22:29:09 UTC
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;

Comment 8 Adam Williamson 2009-11-05 00:58:15 UTC
Tag request:

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

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

Comment 9 Adam Williamson 2009-11-05 18:35:23 UTC
this was tagged, jlaska will test to confirm the fix and close it off.

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

Comment 10 James Laska 2009-11-05 18:44:38 UTC
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.

Comment 11 David Cantrell 2009-11-06 23:57:28 UTC
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;

Comment 12 James Laska 2009-11-07 03:26:15 UTC
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

Comment 13 Adam Williamson 2009-11-07 03:37:33 UTC
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


Note You need to log in before you can comment on or make changes to this bug.