| 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: <Y8bBs4668C9r5oTT@t14s.localdomain>
Date: Tue, 17 Jan 2023 12:41:39 -0300
From: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To: Vlad Buslov <vladbu@...dia.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
pablo@...filter.org, netdev@...r.kernel.org,
netfilter-devel@...r.kernel.org, jhs@...atatu.com,
xiyou.wangcong@...il.com, jiri@...nulli.us, ozsh@...dia.com,
simon.horman@...igine.com
Subject: Re: [PATCH net-next v2 6/7] net/sched: act_ct: offload UDP NEW
connections
On Fri, Jan 13, 2023 at 05:55:47PM +0100, Vlad Buslov wrote:
> When processing connections allow offloading of UDP connections that don't
> have IPS_ASSURED_BIT set as unidirectional. When performing table lookup
Hmm. Considering that this is now offloading one direction only
already, what about skipping this grace period:
In nf_conntrack_udp_packet(), it does:
/* If we've seen traffic both ways, this is some kind of UDP
* stream. Set Assured.
*/
if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) {
...
/* Still active after two seconds? Extend timeout. */
if (time_after(jiffies, ct->proto.udp.stream_ts)) {
extra = timeouts[UDP_CT_REPLIED];
stream = true;
}
...
/* Also, more likely to be important, and not a probe */
if (stream && !test_and_set_bit(IPS_ASSURED_BIT, &ct->status))
nf_conntrack_event_cache(IPCT_ASSURED, ct);
Maybe the patch should be relying on IPS_SEEN_REPLY_BIT instead of
ASSURED for UDP? Just a thought here, but I'm not seeing why not.
> for reply packets check the current connection status: If UDP
> unidirectional connection became assured also promote the corresponding
> flow table entry to bidirectional and set the 'update' bit, else just set
> the 'update' bit since reply directional traffic will most likely cause
> connection status to become 'established' which requires updating the
> offload state.
>
> Signed-off-by: Vlad Buslov <vladbu@...dia.com>
> ---
> net/sched/act_ct.c | 48 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index bfddb462d2bc..563cbdd8341c 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -369,7 +369,7 @@ static void tcf_ct_flow_tc_ifidx(struct flow_offload *entry,
>
> static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft,
> struct nf_conn *ct,
> - bool tcp)
> + bool tcp, bool bidirectional)
> {
> struct nf_conn_act_ct_ext *act_ct_ext;
> struct flow_offload *entry;
> @@ -388,6 +388,8 @@ static void tcf_ct_flow_table_add(struct tcf_ct_flow_table *ct_ft,
> ct->proto.tcp.seen[0].flags |= IP_CT_TCP_FLAG_BE_LIBERAL;
> ct->proto.tcp.seen[1].flags |= IP_CT_TCP_FLAG_BE_LIBERAL;
> }
> + if (bidirectional)
> + __set_bit(NF_FLOW_HW_BIDIRECTIONAL, &entry->flags);
>
> act_ct_ext = nf_conn_act_ct_ext_find(ct);
> if (act_ct_ext) {
> @@ -411,26 +413,34 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft,
> struct nf_conn *ct,
> enum ip_conntrack_info ctinfo)
> {
> - bool tcp = false;
> -
> - if ((ctinfo != IP_CT_ESTABLISHED && ctinfo != IP_CT_ESTABLISHED_REPLY) ||
> - !test_bit(IPS_ASSURED_BIT, &ct->status))
> - return;
> + bool tcp = false, bidirectional = true;
>
> switch (nf_ct_protonum(ct)) {
> case IPPROTO_TCP:
> - tcp = true;
> - if (ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED)
> + if ((ctinfo != IP_CT_ESTABLISHED &&
> + ctinfo != IP_CT_ESTABLISHED_REPLY) ||
> + !test_bit(IPS_ASSURED_BIT, &ct->status) ||
> + ct->proto.tcp.state != TCP_CONNTRACK_ESTABLISHED)
> return;
> +
> + tcp = true;
> break;
> case IPPROTO_UDP:
> + if (!nf_ct_is_confirmed(ct))
> + return;
> + if (!test_bit(IPS_ASSURED_BIT, &ct->status))
> + bidirectional = false;
> break;
> #ifdef CONFIG_NF_CT_PROTO_GRE
> case IPPROTO_GRE: {
> struct nf_conntrack_tuple *tuple;
>
> - if (ct->status & IPS_NAT_MASK)
> + if ((ctinfo != IP_CT_ESTABLISHED &&
> + ctinfo != IP_CT_ESTABLISHED_REPLY) ||
> + !test_bit(IPS_ASSURED_BIT, &ct->status) ||
> + ct->status & IPS_NAT_MASK)
> return;
> +
> tuple = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
> /* No support for GRE v1 */
> if (tuple->src.u.gre.key || tuple->dst.u.gre.key)
> @@ -446,7 +456,7 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft,
> ct->status & IPS_SEQ_ADJUST)
> return;
>
> - tcf_ct_flow_table_add(ct_ft, ct, tcp);
> + tcf_ct_flow_table_add(ct_ft, ct, tcp, bidirectional);
> }
>
> static bool
> @@ -625,13 +635,27 @@ static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p,
> flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
> ct = flow->ct;
>
> + if (dir == FLOW_OFFLOAD_DIR_REPLY &&
> + !test_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags)) {
> + /* Only offload reply direction after connection became
> + * assured.
> + */
> + if (test_bit(IPS_ASSURED_BIT, &ct->status))
> + set_bit(NF_FLOW_HW_BIDIRECTIONAL, &flow->flags);
> + set_bit(NF_FLOW_HW_UPDATE, &flow->flags);
> + return false;
> + }
> +
> if (tcph && (unlikely(tcph->fin || tcph->rst))) {
> flow_offload_teardown(flow);
> return false;
> }
>
> - ctinfo = dir == FLOW_OFFLOAD_DIR_ORIGINAL ? IP_CT_ESTABLISHED :
> - IP_CT_ESTABLISHED_REPLY;
> + if (dir == FLOW_OFFLOAD_DIR_ORIGINAL)
> + ctinfo = test_bit(IPS_SEEN_REPLY_BIT, &ct->status) ?
> + IP_CT_ESTABLISHED : IP_CT_NEW;
> + else
> + ctinfo = IP_CT_ESTABLISHED_REPLY;
>
> flow_offload_refresh(nf_ft, flow);
> nf_conntrack_get(&ct->ct_general);
> --
> 2.38.1
>
Powered by blists - more mailing lists