| 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: <CADvbK_d6zmbBQH_+1hK2EX9L1MW=RFUbT8j8NQkK10cXMDJ2bg@mail.gmail.com>
Date: Wed, 2 Nov 2022 16:01:09 -0400
From: Xin Long <lucien.xin@...il.com>
To: Aaron Conole <aconole@...hat.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
On Wed, Nov 2, 2022 at 3:21 PM Aaron Conole <aconole@...hat.com> wrote:
>
> 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.
I just tried to keep nf_ct_helper() as similar as possible
to the original ovs_ct_helper().
But sure, we can also pass ct and ctinfo as the arguments,
like some other functions where it passes all of skb, ct and ctinfo.
Thanks.
>
> > +{
> > + 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