Hi Darrell,
Darrell Ball <dlu...@gmail.com> writes:
> Upcoming requirements for new algs make it necessary to split out
> alg helper more cleanly.
>
> Signed-off-by: Darrell Ball <dlu...@gmail.com>
> ---
Thanks for jumping on this so quickly.
I think this and 3/4 should be an independent series. They don't have
much to do with 1/4 or 4/4, and logically could go in (and be developed)
independently.
> lib/conntrack.c | 73
> ++++++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 44 insertions(+), 29 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index b8d0e77..7fbcfba 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -67,6 +67,12 @@ enum ct_alg_mode {
> CT_TFTP_MODE,
> };
>
> +enum ct_alg_ctl_type {
> + CT_ALG_CTL_NONE,
> + CT_ALG_CTL_FTP,
> + CT_ALG_CTL_TFTP,
> +};
> +
Any reason to hardcode these? I think we could have a struct like:
struct ct_alg_helper {
const char *name;
bool (*process_dp_pkt)(struct conn *ct,
const struct conn *expectation,
const struct dp_packet *pkt,
bool nat);
... /* not sure of what else would be needed, yet */;
};
Then we can register statically with some table. I think it will
simplify the way conntrack helpers are added, as well as simplify the
code in the 'framework'/'core' area.
> static bool conn_key_extract(struct conntrack *, struct dp_packet *,
> ovs_be16 dl_type, struct conn_lookup_ctx *,
> uint16_t zone);
> @@ -432,33 +438,47 @@ get_ip_proto(const struct dp_packet *pkt)
> }
>
> static bool
> -is_ftp_ctl(const struct dp_packet *pkt)
> +is_ftp_ctl(const enum ct_alg_ctl_type ct_alg_ctl)
> +{
> + return ct_alg_ctl == CT_ALG_CTL_FTP;
> +}
> +
> +static enum ct_alg_ctl_type
> +get_alg_ctl_type(const struct dp_packet *pkt)
> {
> uint8_t ip_proto = get_ip_proto(pkt);
> + struct udp_header *uh = dp_packet_l4(pkt);
> struct tcp_header *th = dp_packet_l4(pkt);
>
> - /* CT_IPPORT_FTP is used because IPPORT_FTP in not defined in OSX,
> - * at least in in.h. Since this value will never change, just remove
> + /* CT_IPPORT_FTP/TFTP is used because IPPORT_FTP/TFTP in not defined
> + * in OSX, at least in in.h. Since these values will never change, remove
> * the external dependency. */
> -#define CT_IPPORT_FTP 21
> + enum { CT_IPPORT_FTP = 21 };
> + enum { CT_IPPORT_TFTP = 69 };
>
> - return (ip_proto == IPPROTO_TCP &&
> - (th->tcp_src == htons(CT_IPPORT_FTP) ||
> - th->tcp_dst == htons(CT_IPPORT_FTP)));
> + if (ip_proto == IPPROTO_UDP && uh->udp_dst == htons(CT_IPPORT_TFTP)) {
> + return CT_ALG_CTL_TFTP;
> + } else if (ip_proto == IPPROTO_TCP &&
> + (th->tcp_src == htons(CT_IPPORT_FTP) ||
> + th->tcp_dst == htons(CT_IPPORT_FTP))) {
> + return CT_ALG_CTL_FTP;
> + }
> + return CT_ALG_CTL_NONE;
> }
>
> -static bool
> -is_tftp_ctl(const struct dp_packet *pkt)
> +static void
> +handle_alg_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx,
> + struct dp_packet *pkt, enum ct_alg_ctl_type ct_alg_ctl,
> + const struct conn *conn, long long now, bool nat,
> + const struct conn *conn_for_expectation)
> {
> - uint8_t ip_proto = get_ip_proto(pkt);
> - struct udp_header *uh = dp_packet_l4(pkt);
> -
> - /* CT_IPPORT_TFTP is used because IPPORT_TFTP in not defined in OSX,
> - * at least in in.h. Since this value will never change, remove
> - * the external dependency. */
> -#define CT_IPPORT_TFTP 69
> - return (ip_proto == IPPROTO_UDP &&
> - uh->udp_dst == htons(CT_IPPORT_TFTP));
> + /* ALG control packet handling with expectation creation. */
> + if (OVS_UNLIKELY((ct_alg_ctl == CT_ALG_CTL_FTP) && conn)) {
> + handle_ftp_ctl(ct, ctx, pkt, conn_for_expectation,
> + now, CT_FTP_CTL_INTEREST, nat);
> + } else if (OVS_UNLIKELY((ct_alg_ctl == CT_ALG_CTL_TFTP) && conn)) {
> + handle_tftp_ctl(ct, conn_for_expectation, now);
> + }
> }
>
> static void
> @@ -1110,10 +1130,11 @@ process_one(struct conntrack *ct, struct dp_packet
> *pkt,
> bool create_new_conn = false;
> struct conn conn_for_un_nat_copy;
> conn_for_un_nat_copy.conn_type = CT_CONN_TYPE_DEFAULT;
> - bool ftp_ctl = is_ftp_ctl(pkt);
> +
> + enum ct_alg_ctl_type ct_alg_ctl = get_alg_ctl_type(pkt);
>
> if (OVS_LIKELY(conn)) {
> - if (ftp_ctl) {
> + if (is_ftp_ctl(ct_alg_ctl)) {
> /* Keep sequence tracking in sync with the source of the
> * sequence skew. */
> if (ctx->reply != conn->seq_skew_dir) {
> @@ -1173,9 +1194,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
> set_label(pkt, conn, &setlabel[0], &setlabel[1]);
> }
>
> - bool tftp_ctl = is_tftp_ctl(pkt);
> struct conn conn_for_expectation;
> - if (OVS_UNLIKELY((ftp_ctl || tftp_ctl) && conn)) {
> + if (OVS_UNLIKELY((ct_alg_ctl != CT_ALG_CTL_NONE) && conn)) {
> conn_for_expectation = *conn;
> }
>
> @@ -1185,13 +1205,8 @@ process_one(struct conntrack *ct, struct dp_packet
> *pkt,
> create_un_nat_conn(ct, &conn_for_un_nat_copy, now, !!alg_exp);
> }
>
> - /* FTP control packet handling with expectation creation. */
> - if (OVS_UNLIKELY(ftp_ctl && conn)) {
> - handle_ftp_ctl(ct, ctx, pkt, &conn_for_expectation,
> - now, CT_FTP_CTL_INTEREST, !!nat_action_info);
> - } else if (OVS_UNLIKELY(tftp_ctl && conn)) {
> - handle_tftp_ctl(ct, &conn_for_expectation, now);
> - }
> + handle_alg_ctl(ct, ctx, pkt, ct_alg_ctl, conn, now, !!nat_action_info,
> + &conn_for_expectation);
> }
>
> /* Sends the packets in '*pkt_batch' through the connection tracker 'ct'.
> All
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
