Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FS#544 - kmodloader segfault on ixp4xx (armeb) #5574

Closed
openwrt-bot opened this issue Feb 21, 2017 · 10 comments
Closed

FS#544 - kmodloader segfault on ixp4xx (armeb) #5574

openwrt-bot opened this issue Feb 21, 2017 · 10 comments
Labels
flyspray

Comments

@openwrt-bot
Copy link

thess:

I believe this issue to be a really obscure endian problem which may be more toolchain related than a raw software bug in the source code. I've tried to work-around this issue with code changes, but the problem persists. I give up...

Inside the routine 'alloc_module' the following code produces the wrong result -- specifically byte-swapped pointers in the '_aliases' array.

char **_aliases;
...
char *ptr = (char *)_aliases + naliases * sizeof(_aliases[0]);
int len;

i = 0;
do {
len = strlen(aliases[i]) + 1;
memcpy(ptr, aliases[i], len);
_aliases[i] = ptr; <<-- this is the failing assignment
ptr += len;
i++;
} while (i < naliases);
@openwrt-bot
Copy link
Author

yousong:

_aliases[i] = ptr; <<-- this is the failing assignment

You mean "ptr" had correct value, but after the assignment "_aliases[i]" was endian-swapped?

If that is the case, I think inspecting the assembly output with compiler -S flag will help. Can you also share the buggy binary here so that we can objdump it and check the final content.

@openwrt-bot
Copy link
Author

thess:

Yes, that is exactly what I've narrowed it down to. Believe it or not, the statement:

if (_aliases[i] != ptr)

inserted after the assignment is TRUE!

Attached is the offending binary, the assembler output (-S to gcc) and objdump -D of the kmodloader.c.o output.

@openwrt-bot
Copy link
Author

kofec:

Do you think that my issue can be related to this?
https://forum.lede-project.org/t/nsa310-kernel-modules-not-loaded/1760

part of log:
[ 3.278806] sd 2:0:0:0: [sdb] Attached SCSI removable disk
Press the [f] key and hit [enter] to enter failsafe mode
Press the [1], [2], [3] or [4] key and hit [enter] to select the debug level
[ 5.423149] mount_root: loading kmods from internal overlay
Segmentation fau[ 5.435046] mount_root: failed to launch kmodloader from internal overlay
lt
[ 6.349724] UBIFS (ubi0:2): background thread "ubifs_bgt0_2" started, PID 994

@openwrt-bot
Copy link
Author

yousong:

This is very likely an address alignment issue. I will see if I can provide a patch.

@openwrt-bot
Copy link
Author

thess:

Somewhat less than elegant solution - calloc_a() argument alignment is the most likely culprit. This patch does alleviate the issue.

--- a/kmodloader.c +++ b/kmodloader.c @@ -250,7 +250,6 @@ alloc_module(const char *name, const cha { struct module *m; char *_name, *_dep; - char **_aliases; int i, len_aliases; 
len_aliases = naliases * sizeof(aliases[0]);

@@ -258,11 +257,9 @@ alloc_module(const char *name, const cha
len_aliases += strlen(aliases[i]) + 1;
m = calloc_a(sizeof(*m),
&_name, strlen(name) + 1,

  •   &_dep, depends ? strlen(depends) + 2 : 0,

  •   &_aliases, len_aliases);

  •   &_dep, depends ? strlen(depends) + 2 : 0);
    if (!m)
    return NULL;
  • m->name = strcpy(_name, name);
    m->opts = 0;

@@ -279,18 +276,22 @@ alloc_module(const char *name, const cha
if (naliases == 0)
m->aliases = NULL;
else {

  •   char *ptr = (char *)_aliases + naliases * sizeof(_aliases[0]);

  •   m->aliases = (char **)calloc(1, len_aliases);

  •   if (!m->aliases) {

  •   	m->naliases = 0;

  •   	return NULL;

  •   }

  •   char *ptr = (char *)m->aliases + naliases * sizeof(char *);
  int len;

  i = 0;
  do {
  	len = strlen(aliases[i]) + 1;
  	memcpy(ptr, aliases[i], len);

  •   	_aliases[i] = ptr;

  •   	m->aliases[i] = ptr;
  	ptr += len;
  	i++;
  } while (i < naliases);

  •   m->aliases = _aliases;

    }

    m->refcnt = 0;
    @@ -305,6 +306,8 @@ static void free_module(struct module *m
    {
    if (m->opts)
    free(m->opts);

  • if (m->aliases)
  •   free(m->aliases);
    free(m);
    }

@openwrt-bot
Copy link
Author

yousong:

A patch has been posted to the mailing list, please test: http://lists.infradead.org/pipermail/lede-dev/2017-February/006235.html

@openwrt-bot
Copy link
Author

kofec:

Ted, Yousong,
You propose two paches :-) maybe you can merge it in one. Then I can test on my NSA310.

@openwrt-bot
Copy link
Author

yousong:

Hi, kofec

The two patches cannot be merged. I am fairly sure ted's patch should work and it has been run-tested by himself. But it's a temporary workaround not intended for final inclusion I guess.

To try it

  1. Download the patch at http://patchwork.ozlabs.org/patch/731386/ and copy it under dir package/system/ubox/patches
  2. make package/ubox/{clean,compile}
  3. copy the result kmodloader to your board
  4. test it.

@openwrt-bot
Copy link
Author

thess:

I vote to go with @yousong patch - removes the extra copying of the aliases. Additionally, I will be submitting a patch to libubox to update calloc_a() to conform to malloc() alignment of pointers.

@openwrt-bot
Copy link
Author

kofec:

I see your commit in LEDE project and may confirm that it is fine/working on my nsa310

[ 9.619284] NET: Registered protocol family 24 [ 9.630624] kmodloader: done loading kernel modules from /etc/modules.d/* [ 9.873041] EXT4-fs (sda1): mounted filesystem with ordered data mode. Opts:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
flyspray
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@openwrt-bot