On Mon, Oct 08, 2012 at 06:56:05PM -0300, Lucas De Marchi wrote:
> On Mon, Oct 8, 2012 at 6:52 PM, Lucas De Marchi
> <[email protected]> wrote:
> > On Sun, Oct 7, 2012 at 2:35 PM, Marco d'Itri <[email protected]> wrote:
> >> Any objections?
> >>
> >> ----- Forwarded message from "Selim T. Erdogan"
> >> <[email protected]> -----
> >>
> >> From: "Selim T. Erdogan" <[email protected]>
> >> To: Debian Bug Tracking System <[email protected]>
> >> Subject: Bug#689872: libkmod2: module parameters on kernel command line
> >> parsed
> >> wrong
> >>
> >> Package: libkmod2
> >> Version: 9-2
> >> Severity: important
> >> Tags: d-i patch
> >>
> >> This problem only affects module parameters given on the kernel command
> >> line at boot time which include a '.' as part of their value. For
> >> example "libata.force=1.5Gbps". The bug causes such values to
> >> overwrite the parameter, so this would get interpreted as parameter
> >> "5Gbps" with no value. ("force" gets lost.)
> >>
> >> To reproduce this safely, add "testmodule.testparam=1.5G" to the end of
> >> the kernel command line at boot time. Then:
> >>
> >> $ /sbin/modprobe -c |grep 5G
> >> options testmodule 5G
> >>
> >> This problem prevented me from using the wheezy beta 2 debian-installer
> >> on a machine that needed libata.force=1.5Gbps to work properly with my
> >> new SSD. See https://lists.debian.org/debian-boot/2012/10/msg00049.html
> >>
> >> I'm including a very simple at the end of the report that fixes the
> >> problem. Feel free to modify/edit as necessary. With this patch:
> >>
> >> $ /sbin/modprobe -c |grep 5G
> >> options testmodule testparam=1.5G
> >>
> >>
> >> -- System Information:
> >> Debian Release: wheezy/sid
> >> APT prefers unstable
> >> APT policy: (500, 'unstable'), (1, 'experimental')
> >> Architecture: i386 (i686)
> >>
> >> Kernel: Linux 3.2.0-3-686-pae (SMP w/1 CPU core)
> >> Locale: LANG=tr_TR.UTF-8, LC_CTYPE=tr_TR.UTF-8 (charmap=UTF-8)
> >> Shell: /bin/sh linked to /bin/dash
> >>
> >> Versions of packages libkmod2 depends on:
> >> ii libc6 2.13-35
> >> ii multiarch-support 2.13-35
> >>
> >> libkmod2 recommends no packages.
> >>
> >> libkmod2 suggests no packages.
> >>
> >> -- no debconf information
> >>
> >> *** kcmdline-option-parsing
> >> Description: Fixes parsing of module parameters on the kernel command
> >> line. Before this patch, if the parameter value had a '.' in it, that
> >> would break things.
> >> Author: Selim T. Erdoğan <[email protected]>
> >>
> >> --- kmod-9.orig/libkmod/libkmod-config.c
> >> +++ kmod-9/libkmod/libkmod-config.c
> >> @@ -542,6 +542,7 @@ static int kmod_config_parse_kcmdline(st
> >> char buf[KCMD_LINE_SIZE];
> >> int fd, err;
> >> char *p, *modname, *param = NULL, *value = NULL;
> >> + int in_value_text = 0;
> >>
> >> fd = open("/proc/cmdline", O_RDONLY|O_CLOEXEC);
> >> if (fd < 0) {
> >> @@ -564,15 +565,20 @@ static int kmod_config_parse_kcmdline(st
> >> *p = '\0';
> >> kcmdline_parse_result(config, modname, param,
> >> value);
> >> param = value = NULL;
> >> + in_value_text = 0;
> >> modname = p + 1;
> >> break;
> >> case '.':
> >> - *p = '\0';
> >> - param = p + 1;
> >> - break;
> >> + if (in_value_text == 0) {
> >> + *p = '\0';
> >> + param = p + 1;
> >> + break;
> >> + }
> >
> > this fall-through here doesn't seem intentional
You're correct.
> >
> >
> >> case '=':
> >> - if (param != NULL)
> >> + if (param != NULL) {
> >> value = p + 1;
> >> + in_value_text=1;
> >> + }
> >> break;
> >> }
> >> }
> >>
> >
> > Why adding another var? Isn't something like below (whitespace
> > damaged) sufficient?
The version below looks good to me. (Not being familiar with how
module loading works, it took me quite some time to figure out where the
problem was: looked in the kernel, then in the debian-installer and
finally in kmod. At that point I just wanted to get a quick fix to
distinguish the two situations in which we may get a '.' and didn't
notice that param being non-NULL would be sufficient for signaling.)
> >
> >
> > Lucas De Marchi
> >
> > diff --git a/libkmod/libkmod-config.c b/libkmod/libkmod-config.c
> > index 70044f0..398468e 100644
> > --- a/libkmod/libkmod-config.c
> > +++ b/libkmod/libkmod-config.c
> > @@ -567,8 +567,10 @@ static int kmod_config_parse_kcmdline(struct
> > kmod_config *config)
> > modname = p + 1;
> > break;
> > case '.':
> > - *p = '\0';
> > - param = p + 1;
> > + if (param == NULL) {
> > + *p = '\0';
> > + param = p + 1;
> > + }
> > break;
> > case '=':
> > if (param != NULL)
>
> Just made a quick test here and it works:
>
> └ kmod ➤ ./tools/modprobe -c | grep testmodule
> options testmodule testparam=1.5G
>
> Lucas De Marchi
Thanks!
Selim
--
To UNSUBSCRIBE, email to [email protected]
with a subject of "unsubscribe". Trouble? Contact [email protected]
