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
Attachment:
fix-32bits.diff
Description: Binary data
