netdev - Re: [Patch net 8/9] act_ife: move tcfa

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: <CAM_iQpXR56EUwF77QETJvKAvA6mHR3kNOEdA0CuF+uyhr+_Frg@mail.gmail.com>
Date:   Mon, 20 Aug 2018 16:57:46 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Vlad Buslov <vladbu@...lanox.com>
Subject: Re: [Patch net 8/9] act_ife: move tcfa_lock down to where necessary

On Mon, Aug 20, 2018 at 11:29 AM David Miller <davem@...emloft.net> wrote:
>
> From: Cong Wang <xiyou.wangcong@...il.com>
> Date: Sun, 19 Aug 2018 12:22:12 -0700
>
> > The only time we need to take tcfa_lock is when adding
> > a new metainfo to an existing ife->metalist. We don't need
> > to take tcfa_lock so early and so broadly in tcf_ife_init().
> >
> > This means we can always take ife_mod_lock first, avoid the
> > reverse locking ordering warning as reported by Vlad.
> >
> > Reported-by: Vlad Buslov <vladbu@...lanox.com>
> > Tested-by: Vlad Buslov <vladbu@...lanox.com>
> > Cc: Vlad Buslov <vladbu@...lanox.com>
> > Cc: Jamal Hadi Salim <jhs@...atatu.com>
> > Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
>
> After this change we no longer call populate_metalist() in an atomic
> context via tcf_ife_init(), and populate_metalist passes 'exists'
> down to add_metainfo() as an 'atomic' indicator.  It doesn't have this
> meaning if you aren't holding the tcfa_lock in the callers with BH
> disabled.

Passing 'exists' as 'atomic' is prior to my change. With my change,
they are separated as two parameters:

 static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
-                       int len, bool atomic)
+                       int len, bool atomic, bool exists)


Or I misunderstand your point here?


>
> Therefore, add_metainfo()'s 'atomic' indication is inaccurate in this
> call chain and will use GFP_ATOMIC unnecessarily.
>
> Probably the thing to just is just pass 'false' down to add_metainfo()
> in populate_metalist().

This is exactly what this patch does?


 static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
                             bool exists, bool rtnl_held)
 {
@@ -436,12 +431,11 @@ static int populate_metalist(struct tcf_ife_info
*ife, struct nlattr **tb,
...
-                       rc = add_metainfo(ife, i, val, len, exists);
+                       rc = add_metainfo(ife, i, val, len, false, exists);