netdev - Re: [RFC net-next 0/1] net: sched: Introduce conntrack action

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]

Date:   Wed, 3 Apr 2019 07:47:10 +0000
From:   Paul Blakey <paulb@...lanox.com>
To:     Kevin 'ldir' Darbyshire-Bryant <ldir@...byshire-bryant.me.uk>,
        Cong Wang <xiyou.wangcong@...il.com>
CC:     Paul Blakey <paulb@...lanox.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Oz Shlomo <ozsh@...lanox.com>
Subject: Re: [RFC net-next 0/1] net: sched: Introduce conntrack action



Kevin 'ldir' Darbyshire-Bryant wrote on 02/04/2019 12:24:
> Hi Cong & all,
> 
>> On 1 Apr 2019, at 22:06, Cong Wang <xiyou.wangcong@...il.com> wrote:
>>
>> On Mon, Apr 1, 2019 at 7:22 AM Paul Blakey <paulb@...lanox.com> wrote:
>>>
>>
> <snip some context>
>>
>>>
>>> This would probably be better off with the previous name act_conndscp.
>>
>>
>> If naming is the only concern here, then it is not hard to find
>> a name we are all satisfied with, like act_ctinfo (if we still want more
>> than just DSCP).
> 
> Personally I don’t *need* anything more than restoring the DSCP from conntrack mark, however my own narrow viewpoint doesn’t preclude some future desire to restore something else.  For that reason I changed the name from act_conndscp to act_conntrack and hope that the latest suggested name change ‘act_ctinfo’ be the last.  Incidentally I like the name ‘act_ctinfo’ especially if it is intended there be a ’send to conntrack’ type action of act_ct/act_conntrack.
> 
> Moving on from naming the darn thing and assuming ‘act_ctinfo’ sticks, is there anything fundamentally wrong with the code?  Nitpicks (or bigpicks) in approach or style?  I’m naively hoping the first real submission results in “That’s wonderful code, we’d be mad not to accept it” instead of “you’ve done that wrong, and that, and that, and that….” :-)
> 
> 
>>
>> Thanks.
> 
> 
> Cheers,
> 
> Kevin D-B
> 
> gpg: 012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A
> 


Hi Kevin,
If you'd like, You can rebase it on our upcoming act_ct (our
act_conntrack) once we're done (hopefully in a couple of weeks). If you
going with the a standalone action, it's fine with me as well.


> +       /* let's not trust userspace entirely */
> +       /* need at least contiguous 6 bit mask */
> +       if ((0x3f & (ci->mask >> ci->maskshift)) != 0x3f)
> +               ci->mode &= ~CONNTRACK_FLAG_SETDSCP;
> +       /* mask & statemask must not overlap */
> +       if (ci->mask & ci->statemask)
> +               ci->mode &= ~CONNTRACK_FLAG_SETDSCP;
> +

Some nitpicks, you check if the user provides sane input in
conntrack_parmset, but instead of returning an error, you just disable
the only action the user provided and the module supports, so it won't
do nothing, yet the command succeeds.

As marcelo said, this module isn't RCUified... see the design pattern in
act_vlan, act_tunnel_key, act_csum, or what this commits changed:

commit 4c5b9d9642c859f7369338fc42c0f62f4151bef3
Author: Manish Kurup <kurup.manish@...il.com>
act_vlan: VLAN action rewrite to use RCU lock/unlock and update

commit 9c5f69bbd75a7db80578782b037629c5f1e59dce
Author: Davide Caratti <dcaratti@...hat.com>
net/sched: act_csum: don't use spinlock in the fast path


And regarding the
> +       ct = nf_ct_get(skb, &ctinfo);
> +       if (!ct) { /* look harder */


The packet didn't pass connection tracking yet right (!ct) but you're
setting the DSCP early?


> +               if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb),
> +                                      proto, ca->net, &tuple))
> +                       goto out;
> +               zone.id = ca->zone;
> +               zone.dir = NF_CT_DEFAULT_ZONE_DIR;
> +
> +               thash = nf_conntrack_find_get(ca->net, &zone, &tuple);
> +               if (!thash)
> +                       goto out;
> +
> +               if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb),
> +                                      proto, ca->net, &tuple))
> +                       goto out;
> +

Aren't searching for the same tuple twice?

Thanks,
Paul.