netdev - Re: [PATCH net-next RFC 19/20] p4tc: add dynamic action commands

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAM0EoMmHpd+MXr3OmfHks6WVkVo1o9SP_r9PhJiQgM_4rLHN_w@mail.gmail.com>
Date:   Thu, 26 Jan 2023 14:01:03 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Vlad Buslov <vladbu@...dia.com>
Cc:     netdev@...r.kernel.org, kernel@...atatu.com,
        deb.chatterjee@...el.com, anjali.singhai@...el.com,
        namrata.limaye@...el.com, khalidm@...dia.com, tom@...anda.io,
        pratyush@...anda.io, jiri@...nulli.us, xiyou.wangcong@...il.com,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, simon.horman@...igine.com
Subject: Re: [PATCH net-next RFC 19/20] p4tc: add dynamic action commands

On Thu, Jan 26, 2023 at 12:10 PM Vlad Buslov <vladbu@...dia.com> wrote:
>
>
> On Thu 26 Jan 2023 at 07:52, Jamal Hadi Salim <jhs@...atatu.com> wrote:
> > On Wed, Jan 25, 2023 at 4:31 PM Vlad Buslov <vladbu@...dia.com> wrote:
> >>
> >>

> >> > cmd set metadata.myprog.mymd metadata.kernel.skbmark

> >> The comment explains why the lock is required, but doesn't address the
> >> lockdep off/on. Could you elaborate on why it is needed?
> >>
> >
> > Note: we can invoke actions from other actions.
> > Reason it is needed is there is a deadlock false positive splat in the
> > following scenario:
> > A dynamic action will lock(dynact->tcf_lock) for its data and then
> > invoke a totally
> > different dynamic action (with totally independent data) which will also protect
> > its data by invoking its tcf_lock.
>
> Is the ordering of recursive action execution while holding the lock
> well defined? Is there anything preventing action A from calling B
> (maybe indirectly via some chain of actions) and vice versa resulting
> ABBA deadlock?

Yes, it is well defined - we dont allow recursion (action A calling action A)
even indirectly(ABA).  We have explicit code that ensures action
topological ordering;
see patch 15 (ex function determine_act_topological_order()).

> > We will add more description as such.
> > Does this also address what you said on "doesnt address the lockdep off/on"?
>
> Yep.
>
> > Unfortunately this is in the datapath - not sure how much cost it adds.
>
> I would assume lockdep_off/on doesn't result any performance impact on
> production kernels that are compiled without lockdep, just prefer to
> "cooperate" with lockdep by assigning classes where possible instead of
> disabling it outright. Not sure whether it is an option this case
> though.

We are going to look at getting rid of some of those locks altogether.
One other small tidbit:
Note: in P4, actions dont have counters embedded - so we are also leaning to
not even hold a lock for the counters by getting rid of stats
altogether. Essentially
we will create another P4 object called "counter" which can be
optionally bound when requested.

cheers,
jamal