netdev - Re: [PATCH net] Fix an intermittent pr_emerg warning about lo becoming free.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 8 Jun 2017 14:41:04 -0700
From:   Wei Wang <weiwan@...gle.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     Krister Johansen <kjlx@...pleofstupid.com>,
        "David S . Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net] Fix an intermittent pr_emerg warning about lo
 becoming free.

> Wei Wan is actually working on a patch series removing all this
> dst_garbage list stuff.

Yes. I am working on removing the dst garbage collector completely.
dst_dev_event() will be removed from the list of callbacks that the
netdevice notifiers invoke in my patch series.

On Thu, Jun 8, 2017 at 2:06 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> On Thu, 2017-06-08 at 13:12 -0700, Krister Johansen wrote:
>
> ...
>
>> Looking at the gc_task intervals, they started at 663ms when we invoked
>> __dst_free().  After that, they increased to 1663, 3136, 5567, 8191,
>> 10751, and 14848.  The release that set the refcnt to 0 on our dst entry
>> occurred after the gc_task was enqueued for 14 second interval so we had
>> to wait longer than the warning time in wait_allrefs in order for the
>> dst entry to get free'd and the hold on 'lo' to be released.
>>
>> A simple solution to this problem is to have dst_dev_event() reset the
>> gc timer, which causes us to process this list shortly after the
>> gc_mutex is relased when dst_dev_event() completes.
>>
>> Signed-off-by: Krister Johansen <kjlx@...pleofstupid.com>
>> ---
>>  net/core/dst.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/net/core/dst.c b/net/core/dst.c
>> index 6192f11..13ba4a0 100644
>> --- a/net/core/dst.c
>> +++ b/net/core/dst.c
>> @@ -469,6 +469,20 @@ static int dst_dev_event(struct notifier_block *this, unsigned long event,
>>               spin_lock_bh(&dst_garbage.lock);
>>               dst = dst_garbage.list;
>>               dst_garbage.list = NULL;
>> +             /* The code in dst_ifdown places a hold on the loopback device.
>> +              * If the gc entry processing is set to expire after a lengthy
>> +              * interval, this hold can cause netdev_wait_allrefs() to hang
>> +              * out and wait for a long time -- until the the loopback
>> +              * interface is released.  If we're really unlucky, it'll emit
>> +              * pr_emerg messages to console too.  Reset the interval here,
>> +              * so dst cleanups occur in a more timely fashion.
>> +              */
>> +             if (dst_garbage.timer_inc > DST_GC_INC) {
>> +                     dst_garbage.timer_inc = DST_GC_INC;
>> +                     dst_garbage.timer_expires = DST_GC_MIN;
>> +                     mod_delayed_work(system_wq, &dst_gc_work,
>> +                                      dst_garbage.timer_expires);
>> +             }
>>               spin_unlock_bh(&dst_garbage.lock);
>>
>>               if (last)
>
> Looks very nice to me !
>
> Acked-by: Eric Dumazet <edumazet@...gle.com>
>
> Wei Wan is actually working on a patch series removing all this
> dst_garbage list stuff.
>
>
>