LINUX.ORG.RU

Баг в ядре?

 , ,


0

3

Читал тут код ядра линукс от нефиг делать, набрел на такую функцию двоичного поиска. Алгоритм двоичного поиска печально известен тем, что в нем очень часто делают ошибки https://ru.wikipedia.org/wiki/Двоичный_поиск

Учёный Йон Бентли утверждает, что 90 % студентов, разрабатывая двоичный поиск, забывают учесть какое-либо из этих требований. И даже в код, написанный самим Йоном и ходивший из книги в книгу, вкралась ошибка: код не стоек к переполнениям

http://lxr.free-electrons.com/source/kernel/groups.c#L133

133 int groups_search(const struct group_info *group_info, kgid_t grp)
134 {
135         unsigned int left, right;
...
143                 unsigned int mid = (left+right)/2; <-- вот это вот
...
Насколько баг критичен и можно ли через него root получить

★★★★★

Последнее исправление: cetjs2 (всего исправлений: 1)

Предположу, что раз речь о группах, то их 65536 максимум. Переменная 4-х байтовая, значит переполнения не будет.

anonymous
()
Ответ на: комментарий от anonymous

ОК, лезем в драфт стандарта C http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf Страница 44-45:

Their implementation-defined values shall be equal or greater in magnitude

maximum value for an object of type unsigned int UINT_MAX 65535 // 2^16-1

Так что стандарт допускает двухбайтный unsigned int

SZT ★★★★★
() автор топика
Ответ на: комментарий от SZT

maximum value for an object of type unsigned int UINT_MAX 65535 // 2^16-1

В коде речь идёт не о GID, а о kgid_t, а это 32-битный идентификатор, представляющий собой отображение пользовательских 16-битных идентификаторов в ядерные и который, насколько я вижу по коду, может выходить за пределы 16 бит, но вот насколько далеко может выходить - не совсем ясно.

Sorcerer ★★★★★
()
Ответ на: комментарий от redgremlin

Ну например uclinux работает на m68k проце, а он I16LP32 т.е. инты у него 16-битные, а указатели и лонги 32-битные

SZT ★★★★★
() автор топика

Во-первых, про проблему с переполнением там упоминают в связи с знаковым mid и выходом за границу массива в результате, тут mid беззнаковый и выхода не будет, максимум будет бесконечный цикл в случае right > 2^31 и искомого gid'е, близкого к right.
Во-вторых, NGROUPS_MAX в линуксе установлено в 65536 и подобная ситуация встретиться в нормальной ситуации (без патчинга ядра) не может.

redgremlin ★★★★★
()
Ответ на: комментарий от redgremlin

Переполнение может быть и с беззнаковым mid, и это тоже будет баг

SZT ★★★★★
() автор топика
Ответ на: комментарий от f1u77y

Конечно.

Но лучше скажи, почему ты считаешь, что на проце m68k под управлением Linux sizeof(int) == 2. Вики Debian так не считает.

tailgunner ★★★★★
()
Ответ на: комментарий от f1u77y
wget ftp://ftp.eskimo.com/home/scs/src/inttypes/inttypes.h -qO- | grep 68000
 *  Motorola 68000 (used in 68k Macs) with 16-bit ints and 32-bit pointers.
SZT ★★★★★
() автор топика

Если group_info->ngroups может быть более, чем MAX_INT/2+1, то при right>MAX_INT/2 будет переполнение независимо от sizeof(int), имхо.

Если число групп точно ограничено 65536, то проблема всплывёт только при sizeof(int)<3.

backbone ★★★★★
()
Ответ на: комментарий от anonymous

Предположу, что раз речь о группах, то их 65536 максимум.

Нет, нифига подобного

root@user-MS-7519:~# chown 4294967294:4294967294 test1
root@user-MS-7519:~# ls -la test1
-rw-r--r-- 1 4294967294 4294967294 0 Авг 13 10:45 test1

SZT ★★★★★
() автор топика
Ответ на: комментарий от SZT

Нет, нифига подобного

redgremlin выше всё правильно сказал. Указанный код работает не с номером группы, а с индексом массива, размер которого не может превышать 65535.

Sorcerer ★★★★★
()
Ответ на: комментарий от Sorcerer

А например если сделать sysctl kernel.ngroups_max=4294967294 или echo 4294967294 > /proc/sys/kernel/ngroups_max ? Только на моем ядре это не работает. Надо на других дистрах/ядрах проверить. В любом случае это баг т.к. NGROUPS_MAX в будущих ядрах вполне может поменяться, и тогда это все может заглючить

SZT ★★★★★
() автор топика
Ответ на: комментарий от SZT

если сделать sysctl kernel.ngroups_max=4294967294

Это значение read only.

В любом случае это баг т.к. NGROUPS_MAX в будущих ядрах вполне может поменяться

Разве что compilation assert вставить (BUILD_BUG_ON). В сам код в это место вставить дополнительный набор бесполезных инструкций вряд ли позволят: если посмотреть историю изменений, то предыдущее исправление в этом месте - это замена типов индексов с int на unsigned int, но не по причине переполнений, а по причине производительности (были приведены бенчмарки, согласно которым вычисление среднего значения беззнаковых величин быстрее вычисления среднего значения знаковых).

Sorcerer ★★★★★
()
Ответ на: комментарий от SZT

В любом случае это баг т.к. NGROUPS_MAX в будущих ядрах вполне может поменяться, и тогда это все может заглючить

Тогда любая операция с типом time_t является багом, ведь каким бы не был размер этого типа, все равно в стодекальйонный год будет переполнение. «Баг» начнёт проявляться только когда число созданных групп перевалит за 2 миллиарда с хвостиком. Это только /etc/group на десятки гигабайт потянет.

redgremlin ★★★★★
()
Ответ на: комментарий от redgremlin

давно пора уйти от заигрываний с переполнениями и аллоцировать все важные системные величины динамически, работать с ними через gmp

anonymous
()
Ответ на: комментарий от Sorcerer

В сам код в это место вставить дополнительный набор бесполезных инструкций вряд ли позволят

Для 64-битных архитектур вообще никаких дополнительных накладных расходов не будет. Можно тупо сделать unsigned int mid = ((uint64_t)left+(uint64_t)right)/2 и переполнения уже не будет. А если архитектура 32-битная, это делается через mid = left + (right - left) / 2; т.е. всего одна дополнительная операция

SZT ★★★★★
() автор топика

Вам мнение ЛОРовских аналитиков важнее мнения разработчиков ядра? Напишите в баг-трекер.

Klymedy ★★★★★
()
Ответ на: комментарий от SZT

Можно тупо сделать unsigned int mid = ((uint64_t)left+(uint64_t)right)/2 и переполнения уже не будет.

Можно, только это может замедлить работу для некоторых архитектур.

т.е. всего одна дополнительная операция

всего одна дополнительная бесполезная операция. :) Я за BUILD_BUG_ON, если очень хочется.

Sorcerer ★★★★★
()
Ответ на: комментарий от Sorcerer

Ну пусть они уже сами решают как это патчить, если им там важна производительность

SZT ★★★★★
() автор топика
Последнее исправление: SZT (всего исправлений: 1)
Ответ на: комментарий от redgremlin

Тогда любая операция с типом time_t является багом, ведь каким бы не был размер этого типа, все равно в стодекальйонный год будет переполнение.

Ну это не проблема. Можно выбрать настолько большой time_t что тепловая смерть вселенной наступит раньше, чем он переполнися

SZT ★★★★★
() автор топика
Ответ на: комментарий от Sorcerer

Кстати, раз уж говорить об оптимизации кода, то там много мест, которые можно переделать. Вот взять например оттуда же

http://lxr.free-electrons.com/source/kernel/groups.c#L104

109         for (stride = 1; stride < gidsetsize; stride = 3 * stride + 1)
110                 ; /* nothing */
111         stride /= 3;
особенность таких циклов (внутри нифига толком не делается, только меняются переменные) в том, что их можно заоптимизировать через плавучку, воспользовавшись реккурентным соотношением:
stride(1) = 1;
stride(n) = 3 * stride(n-1) + 1

Решаем через maxima:

(%i1) display2d:false$

(%i2) load(solve_rec)$

(%i3) solve_rec(stride[n]=3*stride[n-1]+1, stride[n], stride[1]=1);

(%o3) stride[n] = 3^n/2-1/2
Теперь надо найти такой n для stride(n), чтобы stride(n) = gidsetsize;
(%i4) solve ([3^n/2-1/2=x], [n]);

(%o4) [n = log(2*x+1)/log(3)]

Записываем получившиеся формулы через double. Сначала находим n (оно скорее всего получится нецелым) Получаем целую часть часть через http://www.cplusplus.com/reference/cmath/modf/ и считаем ей по формуле 3^n/2-1/2. Думаю что точности double вполне должно хватить. На всякий случай можно сравнить выдаваемые через вычисления по формулам значения с вычислением через цикл на всем интервале допустимых значений.

Таким образом можно сократить много всяких примитивных циклов, но что-то я сомневаюсь что такой патч будет принят разрабами ядра

SZT ★★★★★
() автор топика
Последнее исправление: SZT (всего исправлений: 1)
Ответ на: комментарий от SZT

в ядре floating point избегают использовать

anonymous
()
Ответ на: комментарий от SZT

Теперь надо найти такой n для stride(n), чтобы stride(n) = gidsetsize;

Вот этого не понял. Зачем? Просто же считаем 3^gitsetsize/2-1/2.

UPD: А, понял. Да, надо, но так ещё медленнее получается.

Таким образом можно сократить много всяких примитивных циклов

Надо ещё посмотреть на количество итераций в цикле. Для текущего NGROUPS_MAX максимальное число итераций равно 11. Вряд ли вычисления с плавающей точкой по этой формуле будут быстрее 11 операций умножения и 11 инкрементов.

А для такого количества итераций можно ещё и заготовить массив всех промежуточных значений, так что даже умножать не потребуется. Далее в алгоритме происходят постоянные деления полученного значения на 3, их тоже можно все заранее предвычислить. Вот такую оптимизацию если сделаете, то вполне могут и принять.

Sorcerer ★★★★★
()
Последнее исправление: Sorcerer (всего исправлений: 2)
Ответ на: комментарий от Sorcerer

Просто же считаем 3^gitsetsize/2-1/2

Нет же. Там ищется максимальное число из ряда (3^k-1)/2, не большее gidsetsize/3. https://en.wikipedia.org/wiki/Shellsort
Ты прав в том, что на флоатах будет дольше (те более, что на практике до NGROUPS_MAX никто не добирается и количество итераций в 99.999% будет не больше 3х (у меня ни на одной машине нет не то что 120, но и 100 групп)) и код станет нечитаемый (потребуется, как минимум, развёрнутый комментарий, откуда все эти логарифмы берутся). Ну и про плавающую точку анонимус прав — math.h в ядре нет.

redgremlin ★★★★★
()
Ответ на: комментарий от SZT

Предположу, что раз речь о группах, то их 65536 максимум.

Нет, нифига подобного
root@user-MS-7519:~# chown 4294967294:4294967294 test1

Ты просто не знаешь, что такое NGROUPS_MAX.

tailgunner ★★★★★
()
Ответ на: комментарий от charly_one

Звиняй, хрустальный шар разбил, так что без вопроса затрудняюсь ответить, смогу ли я по нему подсказать.

redgremlin ★★★★★
()
Вы не можете добавлять комментарии в эту тему. Тема перемещена в архив.