On 5 February 2016 at 17:41, Jarno Rajahalme <jarno@xxxxxxx> 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@xxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
