| 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
| ||
|
Date: Wed, 02 Nov 2022 15:21:27 -0400
From: Aaron Conole <aconole@...hat.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>,
Jamal Hadi Salim <jhs@...atatu.com>,
Cong Wang <xiyou.wangcong@...il.com>,
Jiri Pirko <jiri@...nulli.us>,
Pablo Neira Ayuso <pablo@...filter.org>,
Florian Westphal <fw@...len.de>,
Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
Davide Caratti <dcaratti@...hat.com>,
Oz Shlomo <ozsh@...dia.com>, Paul Blakey <paulb@...dia.com>,
Ilya Maximets <i.maximets@....org>,
Eelco Chaudron <echaudro@...hat.com>
Subject: Re: [PATCHv3 net-next 1/4] net: move the ct helper function to
nf_conntrack_helper for ovs and tc
Xin Long <lucien.xin@...il.com> writes:
> Move ovs_ct_helper from openvswitch to nf_conntrack_helper and rename
> as nf_ct_helper so that it can be used in TC act_ct in the next patch.
> Note that it also adds the checks for the family and proto, as in TC
> act_ct, the packets with correct family and proto are not guaranteed.
>
> Signed-off-by: Xin Long <lucien.xin@...il.com>
> ---
Hi Xin,
> include/net/netfilter/nf_conntrack_helper.h | 2 +
> net/netfilter/nf_conntrack_helper.c | 71 +++++++++++++++++++++
> net/openvswitch/conntrack.c | 61 +-----------------
> 3 files changed, 74 insertions(+), 60 deletions(-)
>
> diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
> index 9939c366f720..6c32e59fc16f 100644
> --- a/include/net/netfilter/nf_conntrack_helper.h
> +++ b/include/net/netfilter/nf_conntrack_helper.h
> @@ -115,6 +115,8 @@ struct nf_conn_help *nf_ct_helper_ext_add(struct nf_conn *ct, gfp_t gfp);
> int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
> gfp_t flags);
>
> +int nf_ct_helper(struct sk_buff *skb, u16 proto);
> +
> void nf_ct_helper_destroy(struct nf_conn *ct);
>
> static inline struct nf_conn_help *nfct_help(const struct nf_conn *ct)
> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> index ff737a76052e..83615e479f87 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c
> @@ -26,7 +26,9 @@
> #include <net/netfilter/nf_conntrack_extend.h>
> #include <net/netfilter/nf_conntrack_helper.h>
> #include <net/netfilter/nf_conntrack_l4proto.h>
> +#include <net/netfilter/nf_conntrack_seqadj.h>
> #include <net/netfilter/nf_log.h>
> +#include <net/ip.h>
>
> static DEFINE_MUTEX(nf_ct_helper_mutex);
> struct hlist_head *nf_ct_helper_hash __read_mostly;
> @@ -240,6 +242,75 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
> }
> EXPORT_SYMBOL_GPL(__nf_ct_try_assign_helper);
>
> +/* 'skb' should already be pulled to nh_ofs. */
> +int nf_ct_helper(struct sk_buff *skb, u16 proto)
AFAICT, in all the places we call this we will have the nf_conn and
ip_conntrack_info already. Maybe it makes sense to pass them here
rather than looking up again? Originally, that information wasn't
available in the calling function - over time this has been added so we
might as well reduce the amount of "extra lookups" performed.
> +{
> + const struct nf_conntrack_helper *helper;
> + const struct nf_conn_help *help;
> + enum ip_conntrack_info ctinfo;
> + unsigned int protoff;
> + struct nf_conn *ct;
> + int err;
> +
> + ct = nf_ct_get(skb, &ctinfo);
> + if (!ct || ctinfo == IP_CT_RELATED_REPLY)
> + return NF_ACCEPT;
> +
> + help = nfct_help(ct);
> + if (!help)
> + return NF_ACCEPT;
> +
> + helper = rcu_dereference(help->helper);
> + if (!helper)
> + return NF_ACCEPT;
> +
> + if (helper->tuple.src.l3num != NFPROTO_UNSPEC &&
> + helper->tuple.src.l3num != proto)
> + return NF_ACCEPT;
> +
> + switch (proto) {
> + case NFPROTO_IPV4:
> + protoff = ip_hdrlen(skb);
> + proto = ip_hdr(skb)->protocol;
> + break;
> + case NFPROTO_IPV6: {
> + u8 nexthdr = ipv6_hdr(skb)->nexthdr;
> + __be16 frag_off;
> + int ofs;
> +
> + ofs = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &nexthdr,
> + &frag_off);
> + if (ofs < 0 || (frag_off & htons(~0x7)) != 0) {
> + pr_debug("proto header not found\n");
> + return NF_ACCEPT;
> + }
> + protoff = ofs;
> + proto = nexthdr;
> + break;
> + }
> + default:
> + WARN_ONCE(1, "helper invoked on non-IP family!");
> + return NF_DROP;
> + }
> +
> + if (helper->tuple.dst.protonum != proto)
> + return NF_ACCEPT;
> +
> + err = helper->help(skb, protoff, ct, ctinfo);
> + if (err != NF_ACCEPT)
> + return err;
> +
> + /* Adjust seqs after helper. This is needed due to some helpers (e.g.,
> + * FTP with NAT) adusting the TCP payload size when mangling IP
> + * addresses and/or port numbers in the text-based control connection.
> + */
> + if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
> + !nf_ct_seq_adjust(skb, ct, ctinfo, protoff))
> + return NF_DROP;
> + return NF_ACCEPT;
> +}
> +EXPORT_SYMBOL_GPL(nf_ct_helper);
> +
> /* appropriate ct lock protecting must be taken by caller */
> static int unhelp(struct nf_conn *ct, void *me)
> {
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index c7b10234cf7c..19b5c54615c8 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -434,65 +434,6 @@ static int ovs_ct_set_labels(struct nf_conn *ct, struct sw_flow_key *key,
> return 0;
> }
>
> -/* 'skb' should already be pulled to nh_ofs. */
> -static int ovs_ct_helper(struct sk_buff *skb, u16 proto)
> -{
> - const struct nf_conntrack_helper *helper;
> - const struct nf_conn_help *help;
> - enum ip_conntrack_info ctinfo;
> - unsigned int protoff;
> - struct nf_conn *ct;
> - int err;
> -
> - ct = nf_ct_get(skb, &ctinfo);
> - if (!ct || ctinfo == IP_CT_RELATED_REPLY)
> - return NF_ACCEPT;
> -
> - help = nfct_help(ct);
> - if (!help)
> - return NF_ACCEPT;
> -
> - helper = rcu_dereference(help->helper);
> - if (!helper)
> - return NF_ACCEPT;
> -
> - switch (proto) {
> - case NFPROTO_IPV4:
> - protoff = ip_hdrlen(skb);
> - break;
> - case NFPROTO_IPV6: {
> - u8 nexthdr = ipv6_hdr(skb)->nexthdr;
> - __be16 frag_off;
> - int ofs;
> -
> - ofs = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &nexthdr,
> - &frag_off);
> - if (ofs < 0 || (frag_off & htons(~0x7)) != 0) {
> - pr_debug("proto header not found\n");
> - return NF_ACCEPT;
> - }
> - protoff = ofs;
> - break;
> - }
> - default:
> - WARN_ONCE(1, "helper invoked on non-IP family!");
> - return NF_DROP;
> - }
> -
> - err = helper->help(skb, protoff, ct, ctinfo);
> - if (err != NF_ACCEPT)
> - return err;
> -
> - /* Adjust seqs after helper. This is needed due to some helpers (e.g.,
> - * FTP with NAT) adusting the TCP payload size when mangling IP
> - * addresses and/or port numbers in the text-based control connection.
> - */
> - if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
> - !nf_ct_seq_adjust(skb, ct, ctinfo, protoff))
> - return NF_DROP;
> - return NF_ACCEPT;
> -}
> -
> /* Returns 0 on success, -EINPROGRESS if 'skb' is stolen, or other nonzero
> * value if 'skb' is freed.
> */
> @@ -1038,7 +979,7 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
> */
> if ((nf_ct_is_confirmed(ct) ? !cached || add_helper :
> info->commit) &&
> - ovs_ct_helper(skb, info->family) != NF_ACCEPT) {
> + nf_ct_helper(skb, info->family) != NF_ACCEPT) {
> return -EINVAL;
> }
Powered by blists - more mailing lists