netdev - Re: [PATCH net-next] openvswitch: add ct

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 10 Oct 2017 10:24:20 -0700
From:   Joe Stringer <joe@....org>
To:     Eric Garver <e@...g.me>, Joe Stringer <joe@....org>,
        Pravin Shelar <pshelar@....org>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        ovs dev <dev@...nvswitch.org>
Subject: Re: [PATCH net-next] openvswitch: add ct_clear action

On 10 October 2017 at 08:09, Eric Garver <e@...g.me> wrote:
> On Tue, Oct 10, 2017 at 05:33:48AM -0700, Joe Stringer wrote:
>> On 9 October 2017 at 21:41, Pravin Shelar <pshelar@....org> wrote:
>> > On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver <e@...g.me> wrote:
>> >> This adds a ct_clear action for clearing conntrack state. ct_clear is
>> >> currently implemented in OVS userspace, but is not backed by an action
>> >> in the kernel datapath. This is useful for flows that may modify a
>> >> packet tuple after a ct lookup has already occurred.
>> >>
>> >> Signed-off-by: Eric Garver <e@...g.me>
>> > Patch mostly looks good. I have following comments.
>> >
>> >> ---
>> >>  include/uapi/linux/openvswitch.h |  2 ++
>> >>  net/openvswitch/actions.c        |  5 +++++
>> >>  net/openvswitch/conntrack.c      | 12 ++++++++++++
>> >>  net/openvswitch/conntrack.h      |  7 +++++++
>> >>  net/openvswitch/flow_netlink.c   |  5 +++++
>> >>  5 files changed, 31 insertions(+)
>> >>
>> >> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
>> >> index 156ee4cab82e..1b6e510e2cc6 100644
>> >> --- a/include/uapi/linux/openvswitch.h
>> >> +++ b/include/uapi/linux/openvswitch.h
>> >> @@ -806,6 +806,7 @@ struct ovs_action_push_eth {
>> >>   * packet.
>> >>   * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the
>> >>   * packet.
>> >> + * @OVS_ACTION_ATTR_CT_CLEAR: Clear conntrack state from the packet.
>> >>   *
>> >>   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
>> >>   * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
>> >> @@ -835,6 +836,7 @@ enum ovs_action_attr {
>> >>         OVS_ACTION_ATTR_TRUNC,        /* u32 struct ovs_action_trunc. */
>> >>         OVS_ACTION_ATTR_PUSH_ETH,     /* struct ovs_action_push_eth. */
>> >>         OVS_ACTION_ATTR_POP_ETH,      /* No argument. */
>> >> +       OVS_ACTION_ATTR_CT_CLEAR,     /* No argument. */
>> >>
>> >>         __OVS_ACTION_ATTR_MAX,        /* Nothing past this will be accepted
>> >>                                        * from userspace. */
>> >> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> >> index a54a556fcdb5..db9c7f2e662b 100644
>> >> --- a/net/openvswitch/actions.c
>> >> +++ b/net/openvswitch/actions.c
>> >> @@ -1203,6 +1203,10 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>> >>                                 return err == -EINPROGRESS ? 0 : err;
>> >>                         break;
>> >>
>> >> +               case OVS_ACTION_ATTR_CT_CLEAR:
>> >> +                       err = ovs_ct_clear(skb, key);
>> >> +                       break;
>> >> +
>> >>                 case OVS_ACTION_ATTR_PUSH_ETH:
>> >>                         err = push_eth(skb, key, nla_data(a));
>> >>                         break;
>> >> @@ -1210,6 +1214,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>> >>                 case OVS_ACTION_ATTR_POP_ETH:
>> >>                         err = pop_eth(skb, key);
>> >>                         break;
>> >> +
>> >>                 }
>> > Unrelated change.
>> >
>> >>
>> >>                 if (unlikely(err)) {
>> >> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> >> index d558e882ca0c..f9b73c726ad7 100644
>> >> --- a/net/openvswitch/conntrack.c
>> >> +++ b/net/openvswitch/conntrack.c
>> >> @@ -1129,6 +1129,18 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
>> >>         return err;
>> >>  }
>> >>
>> >> +int ovs_ct_clear(struct sk_buff *skb, struct sw_flow_key *key)
>> >> +{
>> >> +       if (skb_nfct(skb)) {
>> >> +               nf_conntrack_put(skb_nfct(skb));
>> >> +               nf_ct_set(skb, NULL, 0);
>> > Can the new conntract state be appropriate? may be IP_CT_UNTRACKED?
>> >
>> >> +       }
>> >> +
>> >> +       ovs_ct_fill_key(skb, key);
>> >> +
>> > I do not see need to refill the key if there is no skb-nf-ct.
>>
>> Really this is trying to just zero the CT key fields, but reuses
>> existing functions, right? This means that subsequent upcalls, for
>
> Right.
>
>> instance, won't have the outdated view of the CT state from the
>> previous lookup (that was prior to the ct_clear). I'd expect these key
>> fields to be cleared.
>
> I assumed Pravin was saying that we don't need to clear them if there is
> no conntrack state. They should already be zero.

The conntrack calls aren't going to clear it, so I don't see what else
would clear it?

If you execute ct(),ct_clear(), then the first ct will set the
values.. what will zero them?