Re: [PATCH nf-next v7 4/7] openvswitch: Find existing conntrack entry after upcall.
From: Joe Stringer <hidden>
Date: 2016-02-17 22:03:17
Also in:
netfilter-devel
On 5 February 2016 at 17:41, Jarno Rajahalme [off-list ref] 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 <redacted>
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.
quoted hunk
@@ -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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev