netdev - Re: [PATCH net-next] net: sched: ingress: set 'unlocked' flag for Qdisc ops

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]

Message-ID: <vbf4l4u7yme.fsf@mellanox.com>
Date:   Thu, 13 Jun 2019 07:06:38 +0000
From:   Vlad Buslov <vladbu@...lanox.com>
To:     Daniel Borkmann <daniel@...earbox.net>
CC:     Vlad Buslov <vladbu@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "alexei.starovoitov@...il.com" <alexei.starovoitov@...il.com>
Subject: Re: [PATCH net-next] net: sched: ingress: set 'unlocked' flag for
 Qdisc ops


On Wed 12 Jun 2019 at 19:33, Daniel Borkmann <daniel@...earbox.net> wrote:
> On 06/12/2019 09:14 AM, Vlad Buslov wrote:
>> To remove rtnl lock dependency in tc filter update API when using ingress
>> Qdisc, set QDISC_CLASS_OPS_DOIT_UNLOCKED flag in ingress Qdisc_class_ops.
>>
>> Ingress Qdisc ops don't require any modifications to be used without rtnl
>> lock on tc filter update path. Ingress implementation never changes its
>> q->block and only releases it when Qdisc is being destroyed. This means it
>> is enough for RTM_{NEWTFILTER|DELTFILTER|GETTFILTER} message handlers to
>> hold ingress Qdisc reference while using it without relying on rtnl lock
>> protection. Unlocked Qdisc ops support is already implemented in filter
>> update path by unlocked cls API patch set.
>>
>> Signed-off-by: Vlad Buslov <vladbu@...lanox.com>
>> ---
>>  net/sched/sch_ingress.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
>> index 0f65f617756b..d5382554e281 100644
>> --- a/net/sched/sch_ingress.c
>> +++ b/net/sched/sch_ingress.c
>> @@ -114,6 +114,7 @@ static int ingress_dump(struct Qdisc *sch, struct sk_buff *skb)
>>  }
>>
>>  static const struct Qdisc_class_ops ingress_class_ops = {
>> +	.flags		=	QDISC_CLASS_OPS_DOIT_UNLOCKED,
>>  	.leaf		=	ingress_leaf,
>>  	.find		=	ingress_find,
>>  	.walk		=	ingress_walk,
>>
>
> Vlad, why is clsact_class_ops not updated here? Please elaborate!

Daniel, no particular reason to not enable unlocked execution for
clsact. I set the unlocked flag for ingress because that was the Qdisc
that I tested all my parallel tc changes on. However, ingress and clsact
implementations are quite similar, so I can send a followup patch that
updates clsact_class_ops, if you want.