netdev - Re: [PATCH nf-next v7 4/7] openvswitch: Find existing conntrack entry after upcall.

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:	Wed, 17 Feb 2016 14:03:17 -0800
From:	Joe Stringer <joe@....org>
To:	Jarno Rajahalme <jarno@....org>
Cc:	netfilter-devel@...r.kernel.org, netdev <netdev@...r.kernel.org>,
	ovs dev <dev@...nvswitch.org>
Subject: Re: [PATCH nf-next v7 4/7] openvswitch: Find existing conntrack entry
 after upcall.

On 5 February 2016 at 17:41, Jarno Rajahalme <jarno@....org> wrote:
> Add a new function ovs_ct_find_existing() to find an existing
> conntrack entry for which this packet was already applied to.  This is
> only to be called when there is evidence that the packet was already
> tracked and committed, but we lost the ct reference due to an
> userspace upcall.
>
> ovs_ct_find_existing() is called from skb_nfct_cached(), which can now
> hide the fact that the ct reference may have been lost due to an
> upcall.  This allows ovs_ct_commit() to be simplified.
>
> This patch is needed by later "openvswitch: Interface with NAT" patch,
> as we need to be able to pass the packet through NAT using the
> original ct reference also after the reference is lost after an
> upcall.
>
> Signed-off-by: Jarno Rajahalme <jarno@....org>

Please run checkpatch.pl against your series; there are various style
issues and also things like we should not hit BUG_ON() in packet
processing path.

> +/* Find an existing conntrack entry for which this packet was already applied
> + * to.  This is only called when there is evidence that the packet was already
> + * tracked and commited, but we lost the ct reference due to an userspace
> + * upcall. This means that on entry skb->nfct is NULL.
> + * On success, returns conntrack ptr, sets skb->nfct and ctinfo.
> + * Must be called rcu_read_lock()ed. */

I think this reads a bit more natural:

/* Find an existing connection which this packet belongs to without
re-attributing
 * statistics or modifying the connection state. During upcall processing,
 * skb->nfct is lost, so this allows it to be recovered during actions
execution.
 * Must be called with rcu_read_lock.
 *
 * On success, populates skb->nfct and skb->nfctinfo, and returns the
 * connection. Returns NULL if there is no existing entry.
 */

> +static struct nf_conn *
> +ovs_ct_find_existing(struct net *net, const struct nf_conntrack_zone *zone,
> +                    u_int8_t l3num, struct sk_buff *skb,
> +                    enum ip_conntrack_info *ctinfo)

The caller doesn't use ctinfo, so this argument could be dropped?

<snip>

>         ct = nf_ct_get(skb, &ctinfo);
> +       /* If no ct, check if we have evidence that an existing conntrack entry
> +        * might be found for this skb.  This happens when we lose a skb->nfct
> +        * due to an upcall.  If the connection was not confirmed, it is not
> +        * cached and needs to be run through conntrack again. */
> +       if (!ct && key->ct.state & OVS_CS_F_TRACKED
> +           && !(key->ct.state & OVS_CS_F_INVALID)
> +           && key->ct.zone == info->zone.id)
> +               ct = ovs_ct_find_existing(net, &info->zone, info->family, skb,
> +                                         &ctinfo);

Operator style in net is a bit different from OVS userspace.

>         if (!ct)
>                 return false;
> +
>         if (!net_eq(net, read_pnet(&ct->ct_net)))
>                 return false;
>         if (!nf_ct_zone_equal_any(info->ct, nf_ct_zone(ct)))

Unrelated whitespace.

> @@ -421,6 +508,13 @@ static int ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>  {
>         struct nf_conntrack_expect *exp;
>
> +       /* If we pass an expected packet through nf_conntrack_in() the
> +        * expectiation will be removed, but the packet could still be lost in
> +        * upcall processing.  To prevent this from happening we perform an
> +        * explicit expectation lookup.  Expected connections are always new,
> +        * and will be passed through conntrack only when they are committed,
> +        * as it is OK to remove the expectation at that time.
> +        */

The expectation /may/ be removed, but as I understand it depends on
the protocol handler. Minor wording tweak, but this comment makes
sense. This hunk could be a separate patch to document existing
behaviour, but I'm not fussed how it's submitted.

Thanks,
Joe