| Message ID | 1446854768-38299-8-git-send-email-jrajahalme@nicira.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On 06.11, Jarno Rajahalme 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. For the first packet a helper receives the connection is always unconfirmed. It makes no sense to confirm it if the helper drops the packet. > Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> > --- > 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 7aa38fa..ba44287 100644 > --- a/net/openvswitch/conntrack.c > +++ b/net/openvswitch/conntrack.c > @@ -458,6 +458,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, > @@ -468,7 +469,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; > > @@ -491,11 +496,16 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key, > return -ENOENT; > > ovs_ct_update_key(skb, 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; > } > > return 0; > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
> On Nov 9, 2015, at 5:26 AM, Patrick McHardy <kaber@trash.net> wrote: > > On 06.11, Jarno Rajahalme 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. > > For the first packet a helper receives the connection is always unconfirmed. > It makes no sense to confirm it if the helper drops the packet. > Right, the nf_conntrack_confirm() call is still done only if helper accepts the packet. The issue this patch deals with is that in a fairly typical pattern the packet will be passed through conntrack by a CT action and then recirculated (for matching on the connection state, using the RECIRC action), and later an another CT action is used to confirm the connection, possibly with NAT. Before this patch, __ovs_ct_lookup() would have passed the packet through the helper in the first step, while the NAT call would only happen with the second step, i.e., in the wrong order. With this patch the helper for new connections is called with the second step, after calling the NAT code (as added by the patch 8/8). For non-new packets we must call the helper with the first conntrack lookup, as there typically are no later CT actions for packet belonging to existing connections. For new connections we know that if the first CT action does not have the ‘commit’ flag (which causes nf_conntrack_confirm() to be called), we can safely postpone the helper call, as there has to be a later CT action for the connection to be confirmed. Jarno >> Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> >> --- >> 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 7aa38fa..ba44287 100644 >> --- a/net/openvswitch/conntrack.c >> +++ b/net/openvswitch/conntrack.c >> @@ -458,6 +458,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, >> @@ -468,7 +469,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; >> >> @@ -491,11 +496,16 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key, >> return -ENOENT; >> >> ovs_ct_update_key(skb, 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; >> } >> >> return 0; >> -- >> 2.1.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >>
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c index 7aa38fa..ba44287 100644 --- a/net/openvswitch/conntrack.c +++ b/net/openvswitch/conntrack.c @@ -458,6 +458,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, @@ -468,7 +469,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; @@ -491,11 +496,16 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key, return -ENOENT; ovs_ct_update_key(skb, 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; } return 0;
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 <jrajahalme@nicira.com> --- net/openvswitch/conntrack.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)