Thanks for the reviews, Joe! I just posted a v8 addressing your concerns,
Jarno > On Feb 17, 2016, at 4:00 PM, Joe Stringer <j...@ovn.org> wrote: > > On 5 February 2016 at 17:41, Jarno Rajahalme <ja...@ovn.org > <mailto:ja...@ovn.org>> wrote: >> There is no need to help connections that are not confirmed, so we can >> delay helping new connections to the time when they are confirmed. >> This change is needed for NAT support, and having this as a separate >> patch will make the following NAT patch a bit easier to review. >> >> Signed-off-by: Jarno Rajahalme <ja...@ovn.org> >> --- >> net/openvswitch/conntrack.c | 20 +++++++++++++++----- >> 1 file changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c >> index fa9ab25..fc0ef11 100644 >> --- a/net/openvswitch/conntrack.c >> +++ b/net/openvswitch/conntrack.c >> @@ -464,6 +464,7 @@ static bool skb_nfct_cached(struct net *net, >> /* Pass 'skb' through conntrack in 'net', using zone configured in 'info', if >> * not done already. Update key with new CT state after passing the packet >> * through conntrack. >> + * Note that invalid packets are accepted while the skb->nfct remains unset! >> */ >> static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key, >> const struct ovs_conntrack_info *info, > > This seems unrelated to the patch. > > I *think* that you're trying to say that 'if the packet is deemed > invalid by conntrack, skb->nfct will be set to NULL and 0 will be > returned.' I think that this is a documentation issue separate from > this patch (Maybe could be combined with one of the earlier fragments > in the series?). > >> @@ -474,7 +475,11 @@ static int __ovs_ct_lookup(struct net *net, struct >> sw_flow_key *key, >> * actually run the packet through conntrack twice unless it's for a >> * different zone. >> */ >> - if (!skb_nfct_cached(net, key, info, skb)) { >> + bool cached = skb_nfct_cached(net, key, info, skb); >> + enum ip_conntrack_info ctinfo; >> + struct nf_conn *ct; >> + >> + if (!cached) { >> struct nf_conn *tmpl = info->ct; >> int err; >> >> @@ -497,11 +502,16 @@ static int __ovs_ct_lookup(struct net *net, struct >> sw_flow_key *key, >> return -ENOENT; >> >> ovs_ct_update_key(skb, info, key, true); >> + } >> >> - if (ovs_ct_helper(skb, info->family) != NF_ACCEPT) { >> - WARN_ONCE(1, "helper rejected packet"); >> - return -EINVAL; >> - } >> + /* Call the helper right after nf_conntrack_in() for confirmed >> + * connections, but only when commiting for unconfirmed connections. >> + */ >> + ct = nf_ct_get(skb, &ctinfo); >> + if (ct && (nf_ct_is_confirmed(ct) ? !cached : info->commit) >> + && ovs_ct_helper(skb, info->family) != NF_ACCEPT) { >> + WARN_ONCE(1, "helper rejected packet"); >> + return -EINVAL; >> } > > The comment points out what I think is the more obvious piece here: > that the helper should be executed when the connection is/will be > committed. The piece that I found less obvious was that "cached" > implies that the helper was already executed (and therefore it > shouldn't be run again). I don't know if there's a way to make this > more obvious, for example with a "helper_executed" variable or > similar. Up to you. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
