[ovs-dev] [PATCH 1/2] ct-dpif, dpif-netlink: Support conntrack flush by ct 5-tuple

[Justin Pettit]

> On Nov 13, 2017, at 12:12 PM, Yi-Hung Wei <yihung.wei at gmail.com> wrote:
> 
> This patch adds support of flushing a conntrack entry specified by the
> conntrack 5-tuple, and provides the implementation in dpif-netlink.
> The implementation of dpif-netlink in the Linux datapath utilizes the
> NFNL_SUBSYS_CTNETLINK netlink subsystem to delete a conntrack entry in
> nf_conntrack.
> 
> VMWare-BZ: #1983178
> Signed-off-by: Yi-Hung Wei <yihung.wei at gmail.com>
> ---
> lib/ct-dpif.c           |  23 ++++++++---
> lib/ct-dpif.h           |   3 +-
> lib/dpctl.c             |   2 +-
> lib/dpif-netdev.c       |   6 ++-
> lib/dpif-netlink.c      |   7 +++-
> lib/dpif-provider.h     |  13 ++++--
> lib/netlink-conntrack.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++
> lib/netlink-conntrack.h |   1 +
> ofproto/ofproto-dpif.c  |   2 +-
> tests/ovs-ofctl.at      |   2 +-
> 10 files changed, 148 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index c79e69e23970..a9f58f4eb449 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -111,19 +111,30 @@ ct_dpif_dump_done(struct ct_dpif_dump_state *dump)
> }
> 
> /* Flush the entries in the connection tracker used by 'dpif'.
> - *
> - * If 'zone' is not NULL, flush only the entries in '*zone'. */
> + * If both 'zone' and 'tuple' are NULL, flush all the conntrack entries.
> + * If 'zone' is not NULL, and 'tuple's is NULL, flush all the conntrack

I'd drop the "s" in "'tuple's".

> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 599308d257a8..0e1696a94e31 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -5741,10 +5741,14 @@ dpif_netdev_ct_dump_done(struct dpif *dpif OVS_UNUSED,
> }
> 
> static int
> -dpif_netdev_ct_flush(struct dpif *dpif, const uint16_t *zone)
> +dpif_netdev_ct_flush(struct dpif *dpif, const uint16_t *zone,
> +                     const struct ct_dpif_tuple *tuple)
> {
>     struct dp_netdev *dp = get_dp_netdev(dpif);
> 
> +    if (tuple) {
> +        return EOPNOTSUPP;
> +    }
>     return conntrack_flush(&dp->conntrack, zone);
> }

Do you plan on implementing this?

> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 1d82a0939aa1..083d67c8d5b0 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> ...
> -    /* Flushes the connection tracking tables. If 'zone' is not NULL,
> -     * only deletes connections in '*zone'. */
> -    int (*ct_flush)(struct dpif *, const uint16_t *zone);
> +    /* Flushes the connection tracking tables.
> +     * If both 'zone' and 'tuple' are NULL, flush all the conntrack entries.
> +     * If 'zone' is not NULL, and 'tuple's is NULL, flush all the conntrack
> +     * entries in '*zone'.

Same comment as above about "'tuple's'".

> diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
> index 1e1bb2f79d1d..9094779aaa55 100644
> --- a/lib/netlink-conntrack.c
> +++ b/lib/netlink-conntrack.c
> ....
> +int
> +nl_ct_delete(const struct ct_dpif_tuple *tuple, uint16_t zone)
> +{
> +    int err;
> +    struct ofpbuf buf;
> +
> +    ofpbuf_init(&buf, NL_DUMP_BUFSIZE);
> +    nl_msg_put_nfgenmsg(&buf, 0, tuple->l3_type, NFNL_SUBSYS_CTNETLINK,
> +                        IPCTNL_MSG_CT_DELETE, NLM_F_REQUEST);
> +
> +    nl_msg_put_be16(&buf, CTA_ZONE, htons(zone));
> +    if (!nl_ct_put_ct_tuple(&buf, tuple, CTA_TUPLE_ORIG)) {
> +        err = EOPNOTSUPP;
> +        goto out;
> +    }
> +    err = nl_transact(NETLINK_NETFILTER, &buf, NULL);
> +out:
> +    ofpbuf_uninit(&buf);
> +    return err;
> +}

The Linux code for nl_ct_flush_zone() seems to go through some contortions due to there being a problem with flushing a specific zone.  I took a brief look at the kernel code, and it looks like it does properly handle zones if the tuple is provided, but not otherwise.  Is that a correct reading?

On a more cosmetic note, most of the external interface refers to deleting a specific conntrack entry as a "flush", but the internal code refers to it as a "delete". I think this is a reasonable distinction, but if it's not carried externally, then it doesn't seem necessary internally.  Also, this delete and flush code is nearly identical, so it seems like there's some opportunity to just call it "flush" and consolidate to a single implementation that handles a tuple if it's provided.

> +static bool
> +nl_ct_put_tuple(struct ofpbuf *buf, const struct ct_dpif_tuple *tuple)
> +{
> +    if (!nl_ct_put_tuple_ip(buf, tuple)) {
> +        return false;
> +    }
> +    if (!nl_ct_put_tuple_proto(buf, tuple)) {
> +        return false;
> +    }
> +    return true;
> +}
> +
> +static bool
> +nl_ct_put_ct_tuple(struct ofpbuf *buf, const struct ct_dpif_tuple *tuple,
> +                   enum ctattr_type type)
> +{
> +    if (type != CTA_TUPLE_ORIG && type != CTA_TUPLE_REPLY &&
> +        type != CTA_TUPLE_MASTER) {
> +        return false;
> +    }
> +
> +    size_t offset = nl_msg_start_nested(buf, type);
> +    if (!nl_ct_put_tuple(buf, tuple)) {
> +        return false;
> +    }
> +
> +    nl_msg_end_nested(buf, offset);
> +    return true;
> +}

The names for nl_ct_put_ct_tuple() and nl_ct_put_tuple() are very similar.  nl_ct_put_tuple() doesn't do much, what about just folding that into nl_ct_put_ct_tuple()?

Thanks,

--Justin