On Wed, 20 Jun 2012 16:00:40 -0300
Lucas De Marchi <lucas.demarchi@xxxxxxxxxxxxxx> wrote:
> Hi Robert,
>
> On Wed, Jun 20, 2012 at 9:47 AM, Robert Milasan <rmilasan@xxxxxxx>
> wrote:
> > On Wed, 20 Jun 2012 09:15:33 -0300
> > Lucas De Marchi <lucas.demarchi@xxxxxxxxxxxxxx> wrote:
> >
> >> On Wed, Jun 20, 2012 at 8:56 AM, Robert Milasan <rmilasan@xxxxxxx>
> >> wrote:
> >> > On Wed, 20 Jun 2012 08:34:59 -0300
> >> > Lucas De Marchi <lucas.demarchi@xxxxxxxxxxxxxx> wrote:
> >> >
> >> >> Hi Robert,
> >> >>
> >> >> please CC linux-modules@xxxxxxxxxxxxxxx so others can benefit
> >> >> from the responses. I'm doing so now.
> >> >>
> >> >> On Wed, Jun 20, 2012 at 8:21 AM, Robert Milasan
> >> >> <rmilasan@xxxxxxx> wrote:
> >> >> > Hello, I'm trying now to build kmod 9 on 64 and 32 bits
> >> >> > opensuse. On 64bits everything is fine, but on 32bits it fails
> >> >> > in init_modules.c: 150:assert(mkdir_p(buf, 0755) >= 0);
> >> >> >
> >> >> > If I drop assert, all works. Do you have an idea why it fails?
> >> >>
> >> >> Does "make check" work for you after removing this assert? Some
> >> >> of the tests that rely on being able to create dirs on
> >> >> testsuite's rootfs should fail if you didn't create the dirs.
> >> >>
> >> >> On your 32 bits machine, is your testsuite/rootfs dir (or any of
> >> >> the dirs below) read-only?
> >> >>
> >> >> Lucas De Marchi
> >> >
> >> > If I drop assert(mkdir_p...) then all works. rootfs and the below
> >> > dirs all have the rights "0755" so all good, but this issue
> >> > doesn't happen on 64bits which is a bit strange for me. Maybe
> >> > there is something wrong in mkdir.c -> mkdir_p function.
> >>
> >> Maybe... gotta check this on a 32 bits machine.
> >>
> >> >
> >> > Anyway I've asked around and some people said that using assert
> >> > on "mkdir_p" is kind of a bad idea, but I'm not a developer so I
> >> > wouldn't know.
> >>
> >> Yeah, except that this code path runs only on *testsuite*, i.e.
> >> it's part of the tests we make to check it's right, but it's not
> >> part of libkmod, modprobe or depmod, etc that runs on production.
> >>
> >> I put the assert because there are tests that depend on being able
> >> to create dirs: among others, the "install command loop" test. If
> >> dir is not created, it will fail nonetheless, but instead of
> >> saying it's because of a failed dir creation it will create a fork
> >> bomb (yeah, it's a loop of fork + exec). I preferred failing the
> >> test earlier than having to killall modprobe later or wait for
> >> kernel's oom killer.
> >>
> >>
> >> Lucas De Marchi
> >
> > OK, now I know why it fails errno -17 which means the dir already
> > exists.
> >
> > test-init: testsuite/init_module.c:155: create_sysfs_files:
> > Assertion `err >= 0' failed. err: -17
> >
> > This is a small patch for that:
>
> Thanks for this.
>
>
> The bug is a bit deeper: neither on mkdir_p or the filesystem
> permissions. The testsuite includes a 64 bits module that we need to
> fake its insertion (both in 64 and 32 bits machine) - and that means
> picking the module name at the correct memory offset. This is why it
> fails only for 32 bits machine - it was using the "32-bits offset" to
> parse a module for 64 bits and the end result was: modname = "".
>
> The patch below (the same as attached) fixed the issue for me on 32
> bits. Let me know if it works for you
>
> Lucas De Marchi
>
> ----------8<-----------
> diff --git a/testsuite/init_module.c b/testsuite/init_module.c
> index 814998a..8089636 100644
> --- a/testsuite/init_module.c
> +++ b/testsuite/init_module.c
> @@ -16,6 +16,7 @@
> */
>
> #include <assert.h>
> +#include <elf.h>
> #include <errno.h>
> #include <dirent.h>
> #include <fcntl.h>
> @@ -206,6 +207,12 @@ static inline bool module_is_inkernel(const char
> *modname) return ret;
> }
>
> +static uint8_t elf_identify(void *mem)
> +{
> + uint8_t *p = mem;
> + return p[EI_CLASS];
> +}
> +
> TS_EXPORT long init_module(void *mem, unsigned long len, const char
> *args);
>
> /*
> @@ -225,6 +232,8 @@ long init_module(void *mem, unsigned long len,
> const char *args)
> const void *buf;
> uint64_t bufsize;
> int err;
> + uint8_t class;
> + off_t offset;
>
> init_retcodes();
>
> @@ -236,14 +245,18 @@ long init_module(void *mem, unsigned long len,
> const char *args)
> &bufsize);
> kmod_elf_unref(elf);
>
> - /*
> - * We couldn't find the module's name inside the ELF file.
> Just exit
> - * as if it was successful
> - */
> + /* We couldn't parse the ELF file. Just exit as if it was
> successful */ if (err < 0)
> return 0;
>
> - modname = (char *)buf + offsetof(struct module, name);
> + /* We need to open both 32 and 64 bits module - hack! */
> + class = elf_identify(mem);
> + if (class == ELFCLASS64)
> + offset = MODULE_NAME_OFFSET_64;
> + else
> + offset = MODULE_NAME_OFFSET_32;
> +
> + modname = (char *)buf + offset;
> mod = find_module(modules, modname);
> if (mod != NULL) {
> errno = mod->errcode;
> diff --git a/testsuite/stripped-module.h b/testsuite/stripped-module.h
> index 9f97dae..19862f3 100644
> --- a/testsuite/stripped-module.h
> +++ b/testsuite/stripped-module.h
> @@ -13,6 +13,7 @@ struct list_head {
> };
>
> #define MODULE_NAME_LEN (64 - sizeof(unsigned long))
> +
> struct module
> {
> enum module_state state;
> @@ -24,4 +25,8 @@ struct module
> char name[MODULE_NAME_LEN];
> };
>
> +/* padding */
> +#define MODULE_NAME_OFFSET_64 4 + 4 + 2 * 8
> +#define MODULE_NAME_OFFSET_32 4 + 2 * 4
> +
> #endif
Thank you, this worked, at least it builds :)
--
Robert Milasan
L3 Support Engineer
SUSE
rmilasan@xxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
