On Mon, Oct 8, 2012 at 7:39 PM, Selim T. Erdogan
<se...@alumni.cs.utexas.edu> wrote:
> 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
>> <lucas.demar...@profusion.mobi> wrote:
>> > On Sun, Oct 7, 2012 at 2:35 PM, Marco d'Itri <m...@linux.it> wrote:
>> >> Any objections?
>> >>
>> >> ----- Forwarded message from "Selim T. Erdogan"
>> >> <se...@alumni.cs.utexas.edu> -----
>> >>
>> >> From: "Selim T. Erdogan" <se...@alumni.cs.utexas.edu>
>> >> To: Debian Bug Tracking System <sub...@bugs.debian.org>
>> >> 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 <se...@alumni.cs.utexas.edu>
>> >>
>> >> --- 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.)
Thanks a lot for debugging this. Really appreciated. At the time I
wrote this function I thought a "." was not valid inside the value
param (partially because not using fp in kernel). Now I realize how
naive this assumption was and that it's even written in
Documentation/kernel-parameters.txt that we can have values with dots.
Lucas De Marchi
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
