Re: [PATCH] lib/bitmap.c: return -EINVAL for grouping errors in __bitmap_parselist

[Yury Norov]

2015-07-01 4:37 GMT+03:00 Pan Xinhui <xinhuix.pan@xxxxxxxxx>:
> hi, Yury
>         thanks for your nice reply.
>
> On 2015å06æ29æ 21:39, Yury Norov wrote:
>>>
>>> Sometimes the input from user may cause an unexpected result.
>>
>>
>> Could you please provide specific example?
>>
> I wrote some scripts to do some tests about irqs.
> echo "1-3," > /proc/irq/<xxx>/smp_affinity_list
> this command ends with ',' by mistake.
> actually __bitmap_parselist() will report "0-3" for the final result which
> is wrong.
>

Hmm...
I don't think this is wrong passing echo "1-3,".
With or without a comma, the final result must be the same.
More flexible format is useful for hard scripts (for your one).
It's not too difficult to imagine a script producing a line:
         "1-24,  ,   ,,, ,  12-64, 92,92,92,,,"
And I don't think we should reject user with this once the range is valid.
Even more, to spend a time writing some additional code for it, and make
user spend his time as well.

I just tried
          cd /home/yury///./././/work
and it works perfectly well for me, and it's fine.

The true problem is that a and b variables
goes zero after comma, and EOL after comma just takes it:
 514     do {
 ...
 517         a = b = 0;                                           //
<--- comma makes it 0 here
 ...
 520         while (buflen) {
 ...
 539             /* A '\0' or a ',' signal the end of a cpu# or range */
 540             if (c == '\0' || c == ',')                     //
<---here we just break after '\0'
 541                 break;
 559         }
 ...
 565             while (a <= b) {
 566                 set_bit(a, maskp);                   // <--- and
here we set unneeded 0 bit.
 567                 a++;
 568             }

So currently, "1-3,\0" is the same as "1-3,0,\0". And this is definitely wrong.

>
>>>
>>> just like __bitmap_parse, we return -EINVAL if there is no avaiable digit
>>> in each
>>> parsing procedures.
>>>
>>> Signed-off-by: Pan Xinhui <xinhuix.pan@xxxxxxxxx>
>>
>>
>> Hello, Pan.
>>
>> (Adding Alexey Klimov, Rasmus Villemoes)
>>
>>> ---
>>>    lib/bitmap.c | 7 +++++--
>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/bitmap.c b/lib/bitmap.c
>>> index 64c0926..995fca2 100644
>>> --- a/lib/bitmap.c
>>> +++ b/lib/bitmap.c
>>> @@ -504,7 +504,7 @@ static int __bitmap_parselist(const char *buf,
>>> unsigned int buflen,
>>>            int nmaskbits)
>>>    {
>>>        unsigned a, b;
>>> -    int c, old_c, totaldigits;
>>> +    int c, old_c, totaldigits, ndigits;
>>>        const char __user __force *ubuf = (const char __user __force
>>> *)buf;
>>>        int exp_digit, in_range;
>>>
>>> @@ -514,6 +514,7 @@ static int __bitmap_parselist(const char *buf,
>>> unsigned int buflen,
>>>            exp_digit = 1;
>>>            in_range = 0;
>>>            a = b = 0;
>>> +        ndigits = 0;
>>>
>>>            /* Get the next cpu# or a range of cpu#'s */
>>>            while (buflen) {
>>> @@ -555,8 +556,10 @@ static int __bitmap_parselist(const char *buf,
>>> unsigned int buflen,
>>>                if (!in_range)
>>>                    a = b;
>>>                exp_digit = 0;
>>> -            totaldigits++;
>>> +            ndigits++; totaldigits++;
>>
>>
>> I'm not happy with joining two statements to a single line.
>> Maybe sometimes it's OK for loop iterators like
>>
>>      while (a[i][j]) {
>>          i++; j++;
>>      }
>>
>> But here it looks nasty. Anyway, it's minor.
>>
>
> thanks for pointing out my mistake about the code style :)
>
>>>            }
>>> +        if (ndigits == 0)
>>> +            return -EINVAL;
>>
>>
>> You can avoid in-loop incrementation of ndigits if you'll
>> save current totaldigits to ndigits before loop, and check
>> ndigits against totaldigits after the loop:
>>
>>      ndigits = totaldigits;
>>      while (...) {
>>           ...
>>          totaldigits++;
>>      }
>>
>>      if (ndigits == totaldigits)
>>          return -EINVAL;
>>
>> Maybe it's a good point to rework initial __bitmap_parse() similar way...
>>
>
> your advice is a good idea, thanks.
> I am also thinking if we can rewrite them into one function for common
> codes.
>
> thanks for your reply again :)
>
> thanks
> xinhui
>
>
>>>            if (!(a <= b))
>>>                return -EINVAL;
>>>            if (b >= nmaskbits)
>>> --
>>> 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/