netdev - Re: [PATCH net-next 6/7] openvswitch: Add force commit.

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Date:   Tue, 7 Feb 2017 17:32:20 -0800
From:   Joe Stringer <joe@....org>
To:     Jarno Rajahalme <jarno@....org>
Cc:     netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 6/7] openvswitch: Add force commit.

On 7 February 2017 at 17:03, Jarno Rajahalme <jarno@....org> wrote:
>
> On Feb 7, 2017, at 2:15 PM, Joe Stringer <joe@....org> wrote:
>
> On 2 February 2017 at 17:10, Jarno Rajahalme <jarno@....org> wrote:
>
> Stateful network admission policy may allow connections to one
> direction and reject connections initiated in the other direction.
> After policy change it is possible that for a new connection an
> overlapping conntrack entry already exist, where the connection
> original direction is opposed to the new connection's initial packet.
>
> Most importantly, conntrack state relating to the current packet gets
> the "reply" designation based on whether the original direction tuple
> or the reply direction tuple matched.  If this "directionality" is
> wrong w.r.t. to the stateful network admission policy it may happen
> that packets in neither direction are correctly admitted.
>
> This patch adds a new "force commit" option to the OVS conntrack
> action that checks the original direction of an existing conntrack
> entry.  If that direction is opposed to the current packet, the
> existing conntrack entry is deleted and a new one is subsequently
> created in the correct direction.
>
> Signed-off-by: Jarno Rajahalme <jarno@....org>
> ---
> include/uapi/linux/openvswitch.h | 10 ++++++++++
> net/openvswitch/conntrack.c      | 27 +++++++++++++++++++++++++--
> 2 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
> index 90af8b8..d5ba9a9 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -674,6 +674,10 @@ struct ovs_action_hash {
>  * @OVS_CT_ATTR_HELPER: variable length string defining conntrack ALG.
>  * @OVS_CT_ATTR_NAT: Nested OVS_NAT_ATTR_* for performing L3 network address
>  * translation (NAT) on the packet.
> + * @OVS_CT_ATTR_FORCE_COMMIT: Like %OVS_CT_ATTR_COMMIT, but instead of
> doing
> + * nothing if the connection is already committed will check that the
> current
> + * packet is in conntrack entry's original direction.  If directionality
> does
> + * not match, will delete the existing conntrack entry and commit a new
> one.
>  */
> enum ovs_ct_attr {
>        OVS_CT_ATTR_UNSPEC,
> @@ -684,6 +688,12 @@ enum ovs_ct_attr {
>        OVS_CT_ATTR_HELPER,     /* netlink helper to assist detection of
>                                   related connections. */
>        OVS_CT_ATTR_NAT,        /* Nested OVS_NAT_ATTR_* */
> +       OVS_CT_ATTR_FORCE_COMMIT,  /* No argument, commits connection.  If
> the
> +                                   * conntrack entry original direction
> tuple
> +                                   * does not match the current packet
> header
> +                                   * values, will delete the current
> conntrack
> +                                   * entry and create a new one.
> +                                   */
>
>
> We only need one copy of the explanation, keep it above the enum, then
> the inline comment can be /* No argument */.
>
>
> OK.
>
>        __OVS_CT_ATTR_MAX
> };
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 1afe153..1f27f44 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -65,6 +65,7 @@ struct ovs_conntrack_info {
>        struct nf_conn *ct;
>        u8 commit : 1;
>        u8 nat : 3;                 /* enum ovs_ct_nat */
> +       u8 force : 1;
>        u16 family;
>        struct md_mark mark;
>        struct md_labels labels;
> @@ -631,10 +632,13 @@ static bool skb_nfct_cached(struct net *net,
>         */
>        if (!ct && key->ct.state & OVS_CS_F_TRACKED &&
>            !(key->ct.state & OVS_CS_F_INVALID) &&
> -           key->ct.zone == info->zone.id)
> +           key->ct.zone == info->zone.id) {
>                ct = ovs_ct_find_existing(net, &info->zone, info->family,
> skb,
>                                          !!(key->ct.state
>                                             & OVS_CS_F_NAT_MASK));
> +               if (ct)
> +                       nf_ct_get(skb, &ctinfo);
> +       }
>
>
> If ctinfo is only used with the new call below, we can unconditionally
> fetch this just before it's used...
>
>        if (!ct)
>                return false;
>        if (!net_eq(net, read_pnet(&ct->ct_net)))
> @@ -648,6 +652,19 @@ static bool skb_nfct_cached(struct net *net,
>                if (help && rcu_access_pointer(help->helper) != info->helper)
>                        return false;
>        }
> +       /* Force conntrack entry direction to the current packet? */
>
>
> Here.
>
>
> But then we would be executing nf_ct_get() twice in the common case?

Ah, fair enough. It's fine here.