| 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
| ||
|
Message-ID: <5ab59f2d-1c22-4602-95ab-a247b5bf048e@nvidia.com>
Date: Tue, 25 Feb 2025 09:38:33 +0800
From: Jianbo Liu <jianbol@...dia.com>
To: Xin Long <lucien.xin@...il.com>
CC: network dev <netdev@...r.kernel.org>, <dev@...nvswitch.org>,
<ovs-dev@...nvswitch.org>, <davem@...emloft.net>, <kuba@...nel.org>, "Eric
Dumazet" <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, "Pravin B
Shelar" <pshelar@....org>, Ilya Maximets <i.maximets@....org>, Aaron Conole
<aconole@...hat.com>, Florian Westphal <fw@...len.de>
Subject: Re: [PATCHv2 net-next] openvswitch: switch to per-action label
counting in conntrack
On 2/25/2025 3:55 AM, Xin Long wrote:
> On Mon, Feb 24, 2025 at 4:01 AM Jianbo Liu <jianbol@...dia.com> wrote:
>>
>>
>>
>> On 8/13/2024 1:17 AM, Xin Long wrote:
>>> Similar to commit 70f06c115bcc ("sched: act_ct: switch to per-action
>>> label counting"), we should also switch to per-action label counting
>>> in openvswitch conntrack, as Florian suggested.
>>>
>>> The difference is that nf_connlabels_get() is called unconditionally
>>> when creating an ct action in ovs_ct_copy_action(). As with these
>>> flows:
>>>
>>> table=0,ip,actions=ct(commit,table=1)
>>> table=1,ip,actions=ct(commit,exec(set_field:0xac->ct_label),table=2)
>>>
>>> it needs to make sure the label ext is created in the 1st flow before
>>> the ct is committed in ovs_ct_commit(). Otherwise, the warning in
>>> nf_ct_ext_add() when creating the label ext in the 2nd flow will
>>> be triggered:
>>>
>>> WARN_ON(nf_ct_is_confirmed(ct));
>>>
>>
>> Hi Xin Long,
>>
>> The ct can be committed before openvswitch handles packets with CT
>> actions. And we can trigger the warning by creating VF and running ping
>> over it with the following configurations:
>>
>> ovs-vsctl add-br br
>> ovs-vsctl add-port br eth2
>> ovs-vsctl add-port br eth4
>> ovs-ofctl add-flow br "table=0, in_port=eth4,ip,ct_state=-trk
>> actions=ct(table=1)"
>> ovs-ofctl add-flow br "table=1, in_port=eth4,ip,ct_state=+trk+new
>> actions=ct(exec(set_field:0xef7d->ct_label), commit), output:eth2"
>> ovs-ofctl add-flow br "table=1, in_port=eth4,ip,ct_label=0xef7d,
>> ct_state=+trk+est actions=output:eth2"
>>
>> The eth2 is PF, and eth4 is VF's representor.
>> Would you like to fix it?
> Hi, Jianbo,
>
> Sure, we have to attach a new ct to the skb in __ovs_ct_lookup() for
> this case, and even delete the one created before ovs_ct.
>
> Can you check if this works on your env?
Yes, it works.
Could you please submit the formal patch? Thanks!
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 3bb4810234aa..c599ee013dfe 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -595,6 +595,11 @@ static bool skb_nfct_cached(struct net *net,
> rcu_dereference(timeout_ext->timeout))
> return false;
> }
> + if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) && !nf_ct_labels_find(ct)) {
> + if (nf_ct_is_confirmed(ct))
> + nf_ct_delete(ct, 0, 0);
> + return false;
> + }
>
> Thanks.
>
>>
>> Thanks!
>> Jianbo
>>
>>> Signed-off-by: Xin Long <lucien.xin@...il.com>
>>> ---
>>> v2: move ovs_net into #if in ovs_ct_exit() as Jakub noticed.
>>> ---
>>> net/openvswitch/conntrack.c | 30 ++++++++++++------------------
>>> net/openvswitch/datapath.h | 3 ---
>>> 2 files changed, 12 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>>> index 8eb1d644b741..a3da5ee34f92 100644
>>> --- a/net/openvswitch/conntrack.c
>>> +++ b/net/openvswitch/conntrack.c
>>> @@ -1368,11 +1368,8 @@ bool ovs_ct_verify(struct net *net, enum ovs_key_attr attr)
>>> attr == OVS_KEY_ATTR_CT_MARK)
>>> return true;
>>> if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
>>> - attr == OVS_KEY_ATTR_CT_LABELS) {
>>> - struct ovs_net *ovs_net = net_generic(net, ovs_net_id);
>>> -
>>> - return ovs_net->xt_label;
>>> - }
>>> + attr == OVS_KEY_ATTR_CT_LABELS)
>>> + return true;
>>>
>>> return false;
>>> }
>>> @@ -1381,6 +1378,7 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
>>> const struct sw_flow_key *key,
>>> struct sw_flow_actions **sfa, bool log)
>>> {
>>> + unsigned int n_bits = sizeof(struct ovs_key_ct_labels) * BITS_PER_BYTE;
>>> struct ovs_conntrack_info ct_info;
>>> const char *helper = NULL;
>>> u16 family;
>>> @@ -1409,6 +1407,12 @@ int ovs_ct_copy_action(struct net *net, const struct nlattr *attr,
>>> return -ENOMEM;
>>> }
>>>
>>> + if (nf_connlabels_get(net, n_bits - 1)) {
>>> + nf_ct_tmpl_free(ct_info.ct);
>>> + OVS_NLERR(log, "Failed to set connlabel length");
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> if (ct_info.timeout[0]) {
>>> if (nf_ct_set_timeout(net, ct_info.ct, family, key->ip.proto,
>>> ct_info.timeout))
>>> @@ -1577,6 +1581,7 @@ static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info)
>>> if (ct_info->ct) {
>>> if (ct_info->timeout[0])
>>> nf_ct_destroy_timeout(ct_info->ct);
>>> + nf_connlabels_put(nf_ct_net(ct_info->ct));
>>> nf_ct_tmpl_free(ct_info->ct);
>>> }
>>> }
>>> @@ -2002,17 +2007,9 @@ struct genl_family dp_ct_limit_genl_family __ro_after_init = {
>>>
>>> int ovs_ct_init(struct net *net)
>>> {
>>> - unsigned int n_bits = sizeof(struct ovs_key_ct_labels) * BITS_PER_BYTE;
>>> +#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>>> struct ovs_net *ovs_net = net_generic(net, ovs_net_id);
>>>
>>> - if (nf_connlabels_get(net, n_bits - 1)) {
>>> - ovs_net->xt_label = false;
>>> - OVS_NLERR(true, "Failed to set connlabel length");
>>> - } else {
>>> - ovs_net->xt_label = true;
>>> - }
>>> -
>>> -#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>>> return ovs_ct_limit_init(net, ovs_net);
>>> #else
>>> return 0;
>>> @@ -2021,12 +2018,9 @@ int ovs_ct_init(struct net *net)
>>>
>>> void ovs_ct_exit(struct net *net)
>>> {
>>> +#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>>> struct ovs_net *ovs_net = net_generic(net, ovs_net_id);
>>>
>>> -#if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>>> ovs_ct_limit_exit(net, ovs_net);
>>> #endif
>>> -
>>> - if (ovs_net->xt_label)
>>> - nf_connlabels_put(net);
>>> }
>>> diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
>>> index 9ca6231ea647..365b9bb7f546 100644
>>> --- a/net/openvswitch/datapath.h
>>> +++ b/net/openvswitch/datapath.h
>>> @@ -160,9 +160,6 @@ struct ovs_net {
>>> #if IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>>> struct ovs_ct_limit_info *ct_limit_info;
>>> #endif
>>> -
>>> - /* Module reference for configuring conntrack. */
>>> - bool xt_label;
>>> };
>>>
>>> /**
>>
Powered by blists - more mailing lists