netdev - Re: [PATCH net-next 17/17] net: sched: unlock rules update API

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: <vbfftw5jdit.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
Date:   Tue, 13 Nov 2018 13:46:54 +0000
From:   Vlad Buslov <vladbu@...lanox.com>
To:     David Miller <davem@...emloft.net>
CC:     "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>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>
Subject: Re: [PATCH net-next 17/17] net: sched: unlock rules update API

On Mon 12 Nov 2018 at 17:30, David Miller <davem@...emloft.net> wrote:
> From: Vlad Buslov <vladbu@...lanox.com>
> Date: Mon, 12 Nov 2018 09:55:46 +0200
>
>> Register netlink protocol handlers for message types RTM_NEWTFILTER,
>> RTM_DELTFILTER, RTM_GETTFILTER as unlocked. Set rtnl_held variable that
>> tracks rtnl mutex state to be false by default.
>
> This whole conditional locking mechanism is really not clean and makes
> this code so much harder to understand and audit.
>
> Please improve the code so that this kind of construct is not needed.
>
> Thank you.

Hi David,

I considered several approaches to this problem and decided that this
one is most straightforward to implement. I understand your concern and
agree that this code is not easiest to understand and can suggest
several possible solutions that do not require this kind of elaborate
locking mechanism in cls API, but have their own drawbacks:

1. Convert all qdiscs and classifiers to support unlocked execution,
like we did for actions. However, according to my experience with
converting flower classifier, these require much more code than actions.
I would estimate it to be more work than whole current unlocking effort
(hundred+ patches). Also, authors of some of them might be unhappy with
such intrusive changes. I don't think this approach is realistic.

2. Somehow determine if rtnl is needed at the beginning of cls API rule
update functions. Currently, this is not possible because locking
requirements are determined by qdisc_class_ops and tcf_proto_ops 'flags'
field, which requires code to first do whole ops lookup sequence.
However, instead of class field I can put 'flags' in some kind of hash
table or array that will map qdisc/classifier type string to flags, so
it will be possible to determine locking requirements by just parsing
netlink message and obtaining flags by qdisc/classifier type. I do not
consider it pretty solution either, but maybe you have different
opinion.

3. Anything you can suggest? I might be missing something simple that
you would consider more elegant solution to this problem.

Thanks,
Vlad