diff mbox

Linux 2.6.38-rc4 (hysdn: BUG)

Message ID AANLkTimUg8Dm9mZotubcgPHz8_at=_hnbeWUo-LfSALp@mail.gmail.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Linus Torvalds Feb. 9, 2011, 7:44 p.m. UTC
On Wed, Feb 9, 2011 at 9:24 AM, Randy Dunlap <randy.dunlap@oracle.com> wrote:
>
> on x86_64.  no HYSDN hardware found (correct).
> Nearly allmodconfig.
>
>
> [   65.397577] HYSDN: module Rev: 1.6.6.6 loaded
> [   65.397584] HYSDN: network interface Rev: 1.8.6.4
> [   65.398057] HYSDN: 0 card(s) found.
> [   65.398121] BUG: unable to handle kernel paging request at ffffffffa06c99f0
> [   65.398269] IP: [<ffffffffa06c68ba>] hysdn_getrev+0x2e/0x50 [hysdn]
> [   65.398379] PGD 1a14067 PUD 1a18063 PMD 6f6c1067 PTE 800000006ce8c161
> [   65.398613] Oops: 0003 [#1] SMP DEBUG_PAGEALLOC
> [   65.400030]
> [   65.400030] Pid: 2497, comm: modprobe Not tainted 2.6.38-rc4 #1 0TY565/OptiPlex 745
> [   65.400030] RIP: 0010:[<ffffffffa06c68ba>]  [<ffffffffa06c68ba>] hysdn_getrev+0x2e/0x50 [hysdn]
> [   65.400030] RSP: 0018:ffff88006eec1e68  EFLAGS: 00010206
> [   65.400030] RAX: ffffffffa06c99f1 RBX: ffffffffa06c99e9 RCX: ffff88007c4159a0

The instruction sequence decodes to

  1e:	be 24 00 00 00       	mov    $0x24,%esi
  23:	48 89 df             	mov    %rbx,%rdi
  26:	e8 5b 39 c0 e0       	callq  0xffffffffe0c03986
  2b:*	c6 40 ff 00          	movb   $0x0,-0x1(%rax)     <-- trapping instruction

which seems to be this

                p = strchr(rev, '$');
                *--p = 0;

code. And yes, it's total crap, because while "p" and "rev" are "char
*", the string that is passed in is actually of type "const char *",
so that function is seriously broken. It's also seriously broken to
not test that "p" is non-NULL - the function would just break if there
is a colon in the string but not a '$'.

And hysdn_procconf_init() passes in a constant string to the thing:

    static char *hysdn_procconf_revision = "$Revision: 1.8.6.4 $";

What happens is that it breaks when we mark the constant section as
read-only, because you have CONFIG_DEBUG_RODATA enabled.

So the fix seems to be to
 - fix the prototype for hysdn_getrev() to not have "const".
 - fix hysdn_procconf_init() to not pass in a constant string to it

The minimal patch would appear to be something like the appended. UNTESTED!

Btw, all of this code seems to go back to before the git history even
started, so it doesn't seem to be new. I assume you haven't tried
booting these all-module kernels before? Or is it just the
DEBUG_RODATA thing that is new for you?

                    Linus

Comments

Randy Dunlap Feb. 9, 2011, 9:25 p.m. UTC | #1
On Wed, 9 Feb 2011 11:44:00 -0800 Linus Torvalds wrote:

> On Wed, Feb 9, 2011 at 9:24 AM, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> >
> > on x86_64.  no HYSDN hardware found (correct).
> > Nearly allmodconfig.
> >
> >
> > [   65.397577] HYSDN: module Rev: 1.6.6.6 loaded
> > [   65.397584] HYSDN: network interface Rev: 1.8.6.4
> > [   65.398057] HYSDN: 0 card(s) found.
> > [   65.398121] BUG: unable to handle kernel paging request at ffffffffa06c99f0
> > [   65.398269] IP: [<ffffffffa06c68ba>] hysdn_getrev+0x2e/0x50 [hysdn]
> > [   65.398379] PGD 1a14067 PUD 1a18063 PMD 6f6c1067 PTE 800000006ce8c161
> > [   65.398613] Oops: 0003 [#1] SMP DEBUG_PAGEALLOC
> > [   65.400030]
> > [   65.400030] Pid: 2497, comm: modprobe Not tainted 2.6.38-rc4 #1 0TY565/OptiPlex 745
> > [   65.400030] RIP: 0010:[<ffffffffa06c68ba>]  [<ffffffffa06c68ba>] hysdn_getrev+0x2e/0x50 [hysdn]
> > [   65.400030] RSP: 0018:ffff88006eec1e68  EFLAGS: 00010206
> > [   65.400030] RAX: ffffffffa06c99f1 RBX: ffffffffa06c99e9 RCX: ffff88007c4159a0
> 
> The instruction sequence decodes to
> 
>   1e:	be 24 00 00 00       	mov    $0x24,%esi
>   23:	48 89 df             	mov    %rbx,%rdi
>   26:	e8 5b 39 c0 e0       	callq  0xffffffffe0c03986
>   2b:*	c6 40 ff 00          	movb   $0x0,-0x1(%rax)     <-- trapping instruction
> 
> which seems to be this
> 
>                 p = strchr(rev, '$');
>                 *--p = 0;
> 
> code. And yes, it's total crap, because while "p" and "rev" are "char
> *", the string that is passed in is actually of type "const char *",
> so that function is seriously broken. It's also seriously broken to
> not test that "p" is non-NULL - the function would just break if there
> is a colon in the string but not a '$'.
> 
> And hysdn_procconf_init() passes in a constant string to the thing:
> 
>     static char *hysdn_procconf_revision = "$Revision: 1.8.6.4 $";
> 
> What happens is that it breaks when we mark the constant section as
> read-only, because you have CONFIG_DEBUG_RODATA enabled.
> 
> So the fix seems to be to
>  - fix the prototype for hysdn_getrev() to not have "const".
>  - fix hysdn_procconf_init() to not pass in a constant string to it
> 
> The minimal patch would appear to be something like the appended. UNTESTED!

for your patch:

Tested-and-acked-by: Randy Dunlap <randy.dunlap@oracle.com>

> Btw, all of this code seems to go back to before the git history even
> started, so it doesn't seem to be new. I assume you haven't tried
> booting these all-module kernels before? Or is it just the
> DEBUG_RODATA thing that is new for you?

Neither is new.  I tested and reported many-modules on 2.6.37-rc1 and
reported these 2 bugs:

https://bugzilla.kernel.org/show_bug.cgi?id=22912
https://bugzilla.kernel.org/show_bug.cgi?id=22882

and that was with CONFIG_DEBUG_RODATA=y.
I don't know how hysdn was missed at that time.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

 drivers/isdn/hysdn/hysdn_defs.h     |    2 +-
 drivers/isdn/hysdn/hysdn_init.c     |    2 +-
 drivers/isdn/hysdn/hysdn_procconf.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/isdn/hysdn/hysdn_defs.h b/drivers/isdn/hysdn/hysdn_defs.h
index 729df40..d21b4a0 100644
--- a/drivers/isdn/hysdn/hysdn_defs.h
+++ b/drivers/isdn/hysdn/hysdn_defs.h
@@ -227,7 +227,7 @@  extern hysdn_card *card_root;	/* pointer to first card */
 /*************************/
 /* im/exported functions */
 /*************************/
-extern char *hysdn_getrev(const char *);
+extern char *hysdn_getrev(char *);
 
 /* hysdn_procconf.c */
 extern int hysdn_procconf_init(void);	/* init proc config filesys */
diff --git a/drivers/isdn/hysdn/hysdn_init.c b/drivers/isdn/hysdn/hysdn_init.c
index b7cc5c2..4ba2123 100644
--- a/drivers/isdn/hysdn/hysdn_init.c
+++ b/drivers/isdn/hysdn/hysdn_init.c
@@ -53,7 +53,7 @@  static hysdn_card *card_last = NULL;	/* pointer to first card */
 /* extract revision number from string for log output */
 /******************************************************/
 char *
-hysdn_getrev(const char *revision)
+hysdn_getrev(char *revision)
 {
 	char *rev;
 	char *p;
diff --git a/drivers/isdn/hysdn/hysdn_procconf.c b/drivers/isdn/hysdn/hysdn_procconf.c
index 96b3e39..1c396e1 100644
--- a/drivers/isdn/hysdn/hysdn_procconf.c
+++ b/drivers/isdn/hysdn/hysdn_procconf.c
@@ -23,7 +23,7 @@ 
 #include "hysdn_defs.h"
 
 static DEFINE_MUTEX(hysdn_conf_mutex);
-static char *hysdn_procconf_revision = "$Revision: 1.8.6.4 $";
+static char hysdn_procconf_revision[] = "$Revision: 1.8.6.4 $";
 
 #define INFO_OUT_LEN 80		/* length of info line including lf */