On Wed, Jan 16, 2013 at 4:23 PM, Dave Reisner <d@xxxxxxxxxxxxxx> wrote:
> On Wed, Jan 16, 2013 at 03:22:52PM -0200, Lucas De Marchi wrote:
>> On Wed, Jan 16, 2013 at 12:02 PM, Dave Reisner <dreisner@xxxxxxxxxxxxx> wrote:
>> > Trust this value, instead of injecting our own, as this could be
>> > something as simple as ENOENT, rather than an alarming ENOMEM.
>> > ---
>> > libkmod/libkmod.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
>> > index b3e1d6b..0d9cd80 100644
>> > --- a/libkmod/libkmod.c
>> > +++ b/libkmod/libkmod.c
>> > @@ -816,7 +816,7 @@ KMOD_EXPORT int kmod_load_resources(struct kmod_ctx *ctx)
>> >
>> > fail:
>> > kmod_unload_resources(ctx);
>> > - return -ENOMEM;
>> > + return -errno;
>> > }
>>
>> Does this actually work? if index_mm_open() fails it may had passed
>> through munmap(), close() and free() before returning to
>> kmod_load_resources(). The first 2 can touch errno. Worse than that,
>> we also "goto fail" in index_mm_open() if the checks for magic or
>> version failed, in which case errno will not be set and we will
>> incorrectly return that this function succeeded.
>
> Sure, I tested this after all. With patch 2/2, but without this one:
>
> # rm /lib/modules/3.7.2-1-ARCH/modules.builtin*
> # modprobe --show-depends -S 3.7.2-1-ARCH
> # ./tools/modprobe --show-depends -S 3.7.2-1-ARCH ahci
> modprobe: ERROR: Failed to load module indicies: Cannot allocate memory
>
> And with the patch...
>
> # ./tools/modprobe --show-depends -S 3.7.2-1-ARCH ahci
> modprobe: ERROR: Failed to load module indicies: No such file or directory
>
> I hadn't delved too much into index_mm_open, but it sounds like the
> plumbing needs to be tightened up to keep a more reliable errno.
That's because you are considering the return of this function
anything with a meaning, which it's not. < 0 is error (to be fair
most of our docs says this but try to give a meaning to the return
code).
It *happens* to work in your tests. With my malicious mind I can
imagine how to trigger a bug this patch (actually the second one which
makes use of the return value) introduces:
# dd if=/dev/urandom bs=32 count=1 >
# ./tools/modprobe --show-depends achi
modprobe: ERROR: libkmod/libkmod-index.c:818 index_mm_open() magic
check fail: a34bdda1 instead of b007f457
modprobe: ERROR: Failed to load module indicies: No such file or directory
yet strace shows:
open("/lib/modules/3.6.11-1-ARCH/modules.dep.bin", O_RDONLY|O_CLOEXEC) = 3
open("/lib/modules/3.6.11-1-ARCH/modules.alias.bin", O_RDONLY|O_CLOEXEC) = 3
open("/lib/modules/3.6.11-1-ARCH/modules.symbols.bin", O_RDONLY|O_CLOEXEC) = 3
open("/lib/modules/3.6.11-1-ARCH/modules.builtin.bin", O_RDONLY|O_CLOEXEC) = 3
If you really want to give a meaning to the return code you will need
to refactor the code a bit more passing the err code from
index_mm_open up to kmod_load_resources(). Then you also need to be
creative to choose an error number for the problem above.
That said I'm thinking about just reverting
73298175ea05793b48bd7ddfc6b1acdd88dd88fc and going back to your
previous solution marking which indexes are ok to miss. Or just
revert. Thoughts?
Lucas De Marchi
--
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
