又发现一个Linux Kernel的Bug

gaojl0728

以前发布过一个linux kernel 内存泄露的bug没什么反响：一个Linux Kernel Memory Leak的定位过程
http://bbs.chinaunix.net/forum.p ... mp;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);

q1208c

提交给 kernel team 看看呀.

eexplorer

因为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不匹配了。

Godbach

回复 1# gaojl0728

感谢分享。可以 report 给 kernel 啊。


   

mnipxh

没看出有什么问题，应该是get和put之类没配对好。

antriver

回复 3# eexplorer


    因为newrefcnt是local变量，所以虽然有你说的WARN_ON()会被触发，但是dst还是会被释放的，应该没有memory leak.

我同意你的观点.既然有-1之类出现,必然有0的线程出现. 0的线程负责释放内存. -1的线程不需要释放.内存泄露不会有.
问题是为什么release的次数能多到产生-1,肯定别的地方有bug

smalloc

楼主能否再描述下硬件配置和网络情况?

smalloc

为了保证并行化竞争的最小粒度锁,这样设计可能是合理的.
多个路径竞争调用release时,如果一个路径上有-1出现,那么必定有一个路径上有0出现,最终还是被释放了.

倒是5楼说的那种配错对才算是bug.

amarant

ls们说的很对呀。感觉没什么问题。但这样的话题应该鼓励！希望能多看到这一类的话题

gaojl0728

回复 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