Kevin 'ldir' Darbyshire-Bryant wrote on 03/04/2019 11:23:
>
>
>> On 3 Apr 2019, at 08:47, Paul Blakey <pa...@mellanox.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.wangc...@gmail.com> wrote:
>>>>
>>>> On Mon, Apr 1, 2019 at 7:22 AM Paul Blakey <pa...@mellanox.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.man...@gmail.com>
>> act_vlan: VLAN action rewrite to use RCU lock/unlock and update
>>
>> commit 9c5f69bbd75a7db80578782b037629c5f1e59dce
>> Author: Davide Caratti <dcara...@redhat.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.
>>
>> Thanks,
>> Paul.
>
