On Wed, Mar 19, 2014 at 6:50 PM, Anssi Hannula <anssi@xxxxxxxxxx> wrote:
> 19.03.2014 23:22, Lucas De Marchi kirjoitti:
>> On Wed, Mar 19, 2014 at 6:12 PM, Anssi Hannula <anssi@xxxxxxxxxx> wrote:
>>> 19.03.2014 14:24, Lucas De Marchi kirjoitti:
>>>> Hi Anssi,
>>>
>>> Hi,
>>>
>>>> On Tue, Mar 18, 2014 at 8:26 PM, Anssi Hannula <anssi@xxxxxxxxxx> wrote:
>>>>> Currently e.g. "search foo foobar built-in" will cause unpredictable
>>>>> results if baz.ko is in both foo/ and foobar/, since "foo" in search may
>>>>> match both of those directories and the preferred module therefore
>>>>> depends on processing order.
>>>>>
>>>>> Fix the code to ensure that the match is performed on full pathname
>>>>> components only.
>>>>> ---
>>>>> tools/depmod.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/tools/depmod.c b/tools/depmod.c
>>>>> index 9f83ee8..328e578 100644
>>>>> --- a/tools/depmod.c
>>>>> +++ b/tools/depmod.c
>>>>> @@ -1153,10 +1153,10 @@ static int depmod_module_is_higher_priority(const struct depmod *depmod, const s
>>>>> DBG("search %s\n", se->builtin ? "built-in" : se->path);
>>>>> if (se->builtin)
>>>>> bprio = i;
>>>>> - else if (newlen >= se->len &&
>>>>> + else if (newlen > se->len && newpath[se->len] == '/' &&
>>>>> memcmp(se->path, newpath, se->len) == 0)
>>>>> newprio = i;
>>>>> - else if (oldlen >= se->len &&
>>>>> + else if (oldlen > se->len && oldpath[se->len] == '/' &&
>>>>> memcmp(se->path, oldpath, se->len) == 0)
>>>>> oldprio = i;
>>>>> }
>>>>> --
>>>>
>>>>
>>>> Both patches have been applied. Thanks a lot.
>>>>
>>>> I added some test cases for depmod. Could you take a look if they are ok?
>>>
>>> Well, just reversing the "search" directive does not ensure the bug is
>>> caught by the test (and indeed it is not on my testing, i.e. tests still
>>> pass after reverting my fix locally), since it does not alter the
>>> initial order the modules are found.
>>
>> hum... assuming the traverse order is stable, one of them should hit
>> it. In fact. Reverting your commit does reproduce the bug for me 100%
>> of the time.
>>
>>>
>>> To hit the bug, the code needs to hit the short-directory module first,
>>> so that the short path is in "oldpath" and the long path is in "newpath".
>>
>> But it really depends if the short is the right one as opposed to the
>> long one. Otherwise it would be correct to replace the module, even
>> if by accident.
>
> I don't think so. Let me try to show what I mean with an example with
> all the 4 combinations with unfixed code:
>
> Let's have foo/module.ko and foobar/module.ko.
>
> Traverse order foo,foobar:
> oldpath = foo/module.ko
> newpath = foobar/module.ko
>
> "search foo(2) foobar(1) built-in(0)"
> iteration 0: bprio = 0
> iteration 1: newprio = 1
> iteration 2: newprio = 2
> oldprio fallback: oldprio = 0
> => oldprio 0, newprio 2
> => WRONG
>
> "search foobar(2) foo(1) built-in(0)"
> iteration 0: bprio = 0
> iteration 1: newprio = 1
> iteration 2: newprio = 2
> oldprio fallback: oldprio = 0
> => oldprio 0, newprio 2
> => OK
>
>
> Traverse order foobar,foo:
> oldpath = foobar/module.ko
> newpath = foo/module.ko
>
> "search foo(2) foobar(1) built-in(0)"
> iteration 0: bprio = 0
> iteration 1: oldprio = 1
> iteration 2: newprio = 2
> => oldprio 1, newprio 2
> => OK
>
> "search foobar(2) foo(1) built-in(0)"
> iteration 0: bprio = 0
> iteration 1: newprio = 1
> iteration 2: oldprio = 2
> => oldprio 2, newprio 1
> => OK
>
>
> So the bug is triggered only if the shorter name is higher-prio _and_
> shorter name is traversed first. If the long name is traversed first,
> the bug don't trigger with either "search" directive order (and on my
> "make check" runs this is the case).
Yep, you are right. And I'm not sure there's a way to trigger the bug
to always reproduce :(
If I don't figure out a way to do this I think I'll just revert the
test, or at least remove the double test.
thanks
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
