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 12:35:44 +0000
From:   Kevin 'ldir' Darbyshire-Bryant <ldir@...byshire-bryant.me.uk>
To:     Paul Blakey <paulb@...lanox.com>
CC:     Cong Wang <xiyou.wangcong@...il.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



> On 3 Apr 2019, at 12:56, Paul Blakey <paulb@...lanox.com> wrote:
> 
> 
> 
> Kevin 'ldir' Darbyshire-Bryant wrote on 03/04/2019 11:23:
>> 
>> 
>>> On 3 Apr 2019, at 08:47, Paul Blakey <paulb@...lanox.com> wrote:
>>> 
>>> 
>>> 
>>> 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>
>>> 
>>> 
>>> 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.
>> 
>> Ok, I’ll return an -E something.  I guess I’m still stuck between this
>> generic ‘act_ctinfo’ potentially does more than one thing vs ‘act_conndscp’
>> doing a single thing.
>> 
>>> 
>>> 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
>> 
>> Ok, will take a look.  Note current act_connmark on which this is a
>> shameless copy has the same problem.  Is that an oversight and also
>> needs fixing?
> 
> I think it's a performance consideration (and not an error) for now, and
> there were yet to be updated.
> 
>> 
>>> 
>>> 
>>> 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?
>> 
>> Again, I’m not doing anything that act_connmark (conntrack mark to
>> skb mark setting) isn’t doing already, so is act_connmark also wrong?
>> As you can almost certainly tell I’m working in areas that I don’t
>> understand, I only (think I) know the result I wish to achieve and so far
>> it is working.  More luck than judgement?! :-)
>> 
> 
> if I recall correctly, act_connmark doesn't call nf_ct_get_tuplepr twice.
> 

Whoops!  Hangs head in shame - copy/paste merge error at some point - will fix…
along with all the other stuff.


> 
>>> 
>>> Thanks,
>>> Paul.


Cheers,

Kevin D-B