又发现一个Linux Kernel的Bug
本帖最后由 gaojl0728 于 2014-08-15 18:03 编辑以前发布过一个linux kernel 内存泄露的bug没什么反响：一个Linux Kernel Memory Leak的定位过程
http://bbs.chinaunix.net/forum.php?mod=viewthread&tid=4136887&fromuid=29434317
今天再来一个内核IP协议栈的Bug, 不索罗直接给了:
dst_release 通过引用计数来管理dst_entry 内存， 当引用计数递减到0， 就释放dst_entry,
atomic_dec_return虽然能保证dst->__refcnt递减操作是原子的，但是不能保证newrefcnt赋值并判断是否为0也是原子的，
这就造成了race condition， 在newrefcnt赋值之前， atomic_dec_return(&dst->__refcnt)可能已经在不同的调用路径被调用了多次，
也就是说newrefcnt有可能没有看到0值， 直接从1跳到-1， 这就造成dst_entry的内存永远不会被释放掉。
在我们的服务器环境上，能观察到看到WARN_ON(newrefcnt < 0)被触发了很多次，说明dst->__refcnt已经递减到负数。
这个bug影响所有的内核版本，包括最新的mainline
这个bug因为只有race condition发生的时候才会产生，因此极难重现，所以影响不会太大。
void dst_release(struct dst_entry *dst)
{
if (dst) {
int newrefcnt;
newrefcnt = atomic_dec_return(&dst->__refcnt);
WARN_ON(newrefcnt < 0);
if (unlikely(dst->flags & DST_NOCACHE) && !newrefcnt) {
dst = dst_destroy(dst);
if (dst)
__dst_free(dst);
}
}
}
EXPORT_SYMBOL(dst_release); 提交给 kernel team 看看呀. 因为newrefcnt是local变量，所以虽然有你说的WARN_ON()会被触发，但是dst还是会被释放的，应该没有memory leak.
假设thread 1把__refcnt减为0，在thread1的newrefcnt被赋值前，thread2进来又做了一次dst_release，因为是-1，所以不会free dst。thread 1重新运行的时候，他的newrefcnt还0，所以它还是会free dst。
我觉得这个WARN_ON()的本义是认为__refcnt不应该会减为负数，肯定是有的地方没有get dst，导致__refcnt不匹配了。 回复 1# gaojl0728
感谢分享。可以 report 给 kernel 啊。
没看出有什么问题，应该是get和put之类没配对好。 回复 3# eexplorer
因为newrefcnt是local变量，所以虽然有你说的WARN_ON()会被触发，但是dst还是会被释放的，应该没有memory leak.
我同意你的观点.既然有-1之类出现,必然有0的线程出现. 0的线程负责释放内存. -1的线程不需要释放.内存泄露不会有.
问题是为什么release的次数能多到产生-1,肯定别的地方有bug 楼主能否再描述下硬件配置和网络情况? 为了保证并行化竞争的最小粒度锁,这样设计可能是合理的.
多个路径竞争调用release时,如果一个路径上有-1出现,那么必定有一个路径上有0出现,最终还是被释放了.
倒是5楼说的那种配错对才算是bug. 本帖最后由 amarant 于 2014-08-17 16:44 编辑
ls们说的很对呀。感觉没什么问题。但这样的话题应该鼓励！希望能多看到这一类的话题 回复 3# eexplorer
的确如你所言， 不存在竞争条件，每个调用路径下，newrefcnt看到的都是自己的值，总结起来其实是我错误的理解了atomic_dec_return
下面是我的总结分析:
dst_release 存不存在竞争条件，关键在于 atomic_dec_return 能不能保证 "递减并且返回递减之后的值" 这两步是不是原子的。
看了下atomic_dec_return反汇编代码， 看起来atomic_dec_return的确能保证"递减并且返回递减之后的值" 是个原子操作。
而我之前理解atomic_dec_return 只能保证”递减“这一步是原子的，”返回递减之后的值“之前有可能被打断(也就有可能返回之前递减被执行了多次)
我之前认为是这样的:
newrefcnt = atomic_dec_return(&dst->__refcnt);
会被分成两步执行，这两步之间可能被打断，于是产生了竞争条件。
1. dst->__refcnt原子递减1
2. dst->__refcnt赋值给newrefcnt
在执行2之前，步骤1可能会被多个并发路径调用，这样newrefcnt看到的值就有可能是被递减了多次之后的dst->__refcnt， 这存在了竞争条件。
但是实际上是这样执行的：
1. dst->__refcnt赋值给newrefcnt， 然后dst->__refcnt递减1， 注意这两个动作被合并到一起，整体是一个原子操作
2. newrefcnt递减1
这样就保证了newrefcnt看到的值是dst->__refcnt减1之后的值，不存在前面的竞争条件
在gdb下反汇编dst_release，用"disassemble dst_release"命令显示如下:
/* %r12d 就是 newrefcnt， 先置为-1 */
0xffffffff814fecae <+30>: mov $0xffffffff,%r12d
/* %r12d 是 newrefcnt， 0x80(%rdi) 是 dst->__refcnt， xadd交换目的和源操作数，然后载入两个值的和到目的操作数
这样xadd执行完之后newrefcnt存储的是 dst->__refcnt的旧值(减1之前的值)， dst->__refcnt存放减1之后的值
lock 前缀保证了xadd是原子操作。
*/
0xffffffff814fecb4 <+36>: lock xadd %r12d,0x80(%rdi)
/*上一句是dst->__refcnt递减1，这句是newrefcnt递减1，这两句保证了atomic_dec_return不存在竞争条件*/
0xffffffff814fecbd <+45>: sub $0x1,%r12d
//如果递减为负数，就触发WARN_ON
0xffffffff814fecc1 <+49>: js 0xffffffff814fecfe <dst_release+110>
//测试r12d如果为0， 也就是newrefcnt，如果为0， 就进一步判断dst->flags & DST_NOCACHE
0xffffffff814fecc3 <+51>: test %r12d,%r12d
0xffffffff814fecc6 <+54>: je 0xffffffff814fecd8 <dst_release+72>
0xffffffff814fecc8 <+56>: mov (%rsp),%rbx
0xffffffff814feccc <+60>: mov 0x8(%rsp),%r12
页:
[1]
2