On Wed, Jan 16, 2013 at 05:07:14PM -0200, Lucas De Marchi wrote:
> 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:
Sure, and I was only explaining why it happened to work for me. There's
clearly more cases to consider, as you point out.
> # 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.
This sounds like EINVAL or ENOEXEC would be a good fit. I can take a
look at refactoring index_mm_open to do a better job of error reporting
at some point this week. It sounds like it might be useful regardless of
any decision here.
> 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?
My previous patchwork which marked files missing as "acceptable" seems
like a better solution based on this, but note that it's also
potentially open to interpretation as far as what's okay to be missing.
For example, you can load modules just fine without a builtin index, but
you may get false errors because the index is missing. Other indicies
like .dep or .alias are probably more clear cut as being critical for
modprobe to work properly.
Reverting 73298175 is fine by me, but the embedded folk may feel
differently about this. I suppose since kmod is optional in recent udev,
it's less of a deal but that still leaves people hanging if they have
multiple kernels -- a mix of static and modular (I believe you brought
this up).
d
--
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
