netdev - Re: [PATCH net 2/2] net/sched: add delete_empty() to filters and use it in cls

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:   Tue, 24 Dec 2019 15:17:33 +0000
From:   Vlad Buslov <vladbu@...lanox.com>
To:     Davide Caratti <dcaratti@...hat.com>
CC:     Vlad Buslov <vladbu@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>
Subject: Re: [PATCH net 2/2] net/sched: add delete_empty() to filters and use
 it in cls_flower


On Tue 24 Dec 2019 at 16:53, Davide Caratti <dcaratti@...hat.com> wrote:
> hello Jamal and Vlad,
>
> thanks for looking at this.
>
> On Tue, 2019-12-24 at 11:48 +0000, Vlad Buslov wrote:
>> I guess we can reduce this code to just:
>>
>> spin_lock(&tp->lock);
>> tp->deleting = idr_is_empty(&head->handle_idr);
>> spin_unlock(&tp->lock);
>
> on the current version of delete_empty() for cls_flower, we are assuming
> an empty filter also when the IDR is allocated, but its refcount is zero:
>
> 1931         idr_for_each_entry_continue_ul(&head->handle_idr, f, tmp, id) {
> 1932                 /* don't return filters that are being deleted */
> 1933                 if (!refcount_inc_not_zero(&f->refcnt))
> 1934                         continue;
> 1935                 if (arg->fn(tp, f, arg) < 0) {
> 1936                         __fl_put(f);
> 1937                         arg->stop = 1;
> 1938                         break;
> 1939                 }
> 1940                 __fl_put(f);
> 1941                 arg->count++;
> 1942         }
>
> but probably this is relevant to dump(), not delete(). Correct?

I don't think that it is possible to get filter with refcnt==0 from idr
when holding the tp lock (filter is inserted with refcnt==1 and removed
from idr before releasing the last reference in fl_delete()). fl_walk()
doesn't take the lock by itself so it needs to deal with concurrent
removals.

>
>  # ./tdc.py -c flower -d enp2s0
>
> ^^ I'm running several loops of this, just to make sure. If I don't find
> anything relevant in few hours, I will send a v2.
> thanks!