netdev - Re: [PATCH 2/7] mark root hnode explicitly

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_iQpV7H8v3Oebftd0_aHzwwU_Phr3-7a=Un2-TRmuuZr7VOw@mail.gmail.com>
Date:   Thu, 6 Sep 2018 20:23:36 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH 2/7] mark root hnode explicitly

On Thu, Sep 6, 2018 at 8:04 PM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> On Thu, Sep 06, 2018 at 07:57:25PM -0700, Cong Wang wrote:
>
> > > -       if (root_ht == ht) {
> > > +       if (ht->is_root) {
> >
> >
> > What's wrong with comparing pointers with root ht?
>
> The fact that there may be more than one tcf_proto sharing tp->data.

Hmm? root ht is from tp->root, not tp->data.

Also, this very important information is missing in your one-line changelog...



>
> > >                 NL_SET_ERR_MSG_MOD(extack, "Not allowed to delete root node");
> > >                 return -EINVAL;
> > >         }
> > > @@ -795,6 +797,10 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
> > >                                 NL_SET_ERR_MSG_MOD(extack, "Link hash table not found");
> > >                                 return -EINVAL;
> > >                         }
> > > +                       if (ht_down->is_root) {
> >
> > root ht is saved in tp->root, so you can compare ht_down with it too,
> > if you want.
> >
> > If this check is all what you need, you don't need an extra flag.
>
> Again, *which* tp?  We can trivially check that we are not linking to/deleting


Pretty sure there is a 'tp' in u32_set_parms() parameter list.

Are you saying it is not what you want? If so, why?

More importantly, why this information is again missing in your
changelog? This patch is definitely not trivial, it deserves a detailed
changelog.


> our own root, sure.  But there's nothing to stop doing the same via another
> tcf_proto...

To my best knowledge, the place where you set ->is_root=true
is precisely same with where we set tp->root=root_ht, and it doesn't
change after set. What am I missing here?