| Message ID | 1492205198-41758-1-git-send-email-jarno@ovn.org |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
On 14 April 2017 at 14:26, Jarno Rajahalme <jarno@ovn.org> wrote: > Conntrack helpers do not check for a potentially clashing conntrack > entry when creating a new expectation. Also, nf_conntrack_in() will > check expectations (via init_conntrack()) only if a conntrack entry > can not be found. The expectation for a packet which also matches an > existing conntrack entry will not be removed by conntrack, and is > currently handled inconsistently by OVS, as OVS expects the > expectation to be removed when the connection tracking entry matching > that expectation is confirmed. > > It should be noted that normally an IP stack would not allow reuse of > a 5-tuple of an old (possibly lingering) connection for a new data > connection, so this is somewhat unlikely corner case. However, it is > possible that a misbehaving source could cause conntrack entries be > created that could then interfere with new related connections. > > Fix this in the OVS module by deleting the clashing conntrack entry > after an expectation has been matched. This causes the following > nf_conntrack_in() call also find the expectation and remove it when > creating the new conntrack entry, as well as the forthcoming reply > direction packets to match the new related connection instead of the > old clashing conntrack entry. > > Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action") > Reported-by: Yang Song <yangsong@vmware.com> > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > --- Hi Jarno, > v3: Removed unnecessary if statement. > v2: Fixed commit title. > > net/openvswitch/conntrack.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c > index 7b2c2fc..d796ae7 100644 > --- a/net/openvswitch/conntrack.c > +++ b/net/openvswitch/conntrack.c > @@ -514,10 +514,39 @@ ovs_ct_expect_find(struct net *net, const struct nf_conntrack_zone *zone, > u16 proto, const struct sk_buff *skb) > { > struct nf_conntrack_tuple tuple; > + struct nf_conntrack_expect *exp; > > if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, net, &tuple)) > return NULL; > - return __nf_ct_expect_find(net, zone, &tuple); > + > + exp = __nf_ct_expect_find(net, zone, &tuple); > + Extraneous whitespace^ > + if (exp) { > + struct nf_conntrack_tuple_hash *h; > + > + /* Delete existing conntrack entry, if it clashes with the > + * expectation. This can happen since conntrack ALGs do not > + * check for clashes between (new) expectations and existing > + * conntrack entries. nf_conntrack_in() will check the > + * expectations only if a conntrack entry can not be found, > + * which can lead to OVS finding the expectation (here) in the > + * init direction, but which will not be removed by the > + * nf_conntrack_in() call, if a matching conntrack entry is > + * found instead. In this case all init direction packets > + * would be reported as new related packets, while reply > + * direction packets would be reported as un-related > + * established packets. */ > + Extraneous whitespace^ > + h = nf_conntrack_find_get(net, zone, &tuple); > + if (h) { > + struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h); > + > + nf_ct_delete(ct, 0, 0); > + nf_conntrack_put(&ct->ct_general); Do we need the extra nf_conntrack_put() here? If nf_conntrack_find_get() returns an entry, we'll call nf_ct_delete() which releases a reference on the CT entry. -- 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 Apr 18, 2017, at 11:27 AM, Joe Stringer <joe@ovn.org> wrote: > > On 14 April 2017 at 14:26, Jarno Rajahalme <jarno@ovn.org> wrote: >> Conntrack helpers do not check for a potentially clashing conntrack >> entry when creating a new expectation. Also, nf_conntrack_in() will >> check expectations (via init_conntrack()) only if a conntrack entry >> can not be found. The expectation for a packet which also matches an >> existing conntrack entry will not be removed by conntrack, and is >> currently handled inconsistently by OVS, as OVS expects the >> expectation to be removed when the connection tracking entry matching >> that expectation is confirmed. >> >> It should be noted that normally an IP stack would not allow reuse of >> a 5-tuple of an old (possibly lingering) connection for a new data >> connection, so this is somewhat unlikely corner case. However, it is >> possible that a misbehaving source could cause conntrack entries be >> created that could then interfere with new related connections. >> >> Fix this in the OVS module by deleting the clashing conntrack entry >> after an expectation has been matched. This causes the following >> nf_conntrack_in() call also find the expectation and remove it when >> creating the new conntrack entry, as well as the forthcoming reply >> direction packets to match the new related connection instead of the >> old clashing conntrack entry. >> >> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action") >> Reported-by: Yang Song <yangsong@vmware.com> >> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> >> --- > > Hi Jarno, > >> v3: Removed unnecessary if statement. >> v2: Fixed commit title. >> >> net/openvswitch/conntrack.c | 31 ++++++++++++++++++++++++++++++- >> 1 file changed, 30 insertions(+), 1 deletion(-) >> >> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c >> index 7b2c2fc..d796ae7 100644 >> --- a/net/openvswitch/conntrack.c >> +++ b/net/openvswitch/conntrack.c >> @@ -514,10 +514,39 @@ ovs_ct_expect_find(struct net *net, const struct nf_conntrack_zone *zone, >> u16 proto, const struct sk_buff *skb) >> { >> struct nf_conntrack_tuple tuple; >> + struct nf_conntrack_expect *exp; >> >> if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, net, &tuple)) >> return NULL; >> - return __nf_ct_expect_find(net, zone, &tuple); >> + >> + exp = __nf_ct_expect_find(net, zone, &tuple); >> + > > Extraneous whitespace^ > You mean the empty line? >> + if (exp) { >> + struct nf_conntrack_tuple_hash *h; >> + >> + /* Delete existing conntrack entry, if it clashes with the >> + * expectation. This can happen since conntrack ALGs do not >> + * check for clashes between (new) expectations and existing >> + * conntrack entries. nf_conntrack_in() will check the >> + * expectations only if a conntrack entry can not be found, >> + * which can lead to OVS finding the expectation (here) in the >> + * init direction, but which will not be removed by the >> + * nf_conntrack_in() call, if a matching conntrack entry is >> + * found instead. In this case all init direction packets >> + * would be reported as new related packets, while reply >> + * direction packets would be reported as un-related >> + * established packets. */ >> + > > Extraneous whitespace^ > >> + h = nf_conntrack_find_get(net, zone, &tuple); >> + if (h) { >> + struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h); >> + >> + nf_ct_delete(ct, 0, 0); >> + nf_conntrack_put(&ct->ct_general); > > Do we need the extra nf_conntrack_put() here? If > nf_conntrack_find_get() returns an entry, we'll call nf_ct_delete() > which releases a reference on the CT entry. There is one reference held by the table, but nf_conntrack_find_get() takes another. nf_ct_delete() releases the reference held by the table as the entry is removed, but we need to explicitly release the reference taken by nf_conntrack_find_get(). Jarno -- 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 18 April 2017 at 11:33, Jarno Rajahalme <jarno@ovn.org> wrote: > >> On Apr 18, 2017, at 11:27 AM, Joe Stringer <joe@ovn.org> wrote: >> >> On 14 April 2017 at 14:26, Jarno Rajahalme <jarno@ovn.org> wrote: >>> Conntrack helpers do not check for a potentially clashing conntrack >>> entry when creating a new expectation. Also, nf_conntrack_in() will >>> check expectations (via init_conntrack()) only if a conntrack entry >>> can not be found. The expectation for a packet which also matches an >>> existing conntrack entry will not be removed by conntrack, and is >>> currently handled inconsistently by OVS, as OVS expects the >>> expectation to be removed when the connection tracking entry matching >>> that expectation is confirmed. >>> >>> It should be noted that normally an IP stack would not allow reuse of >>> a 5-tuple of an old (possibly lingering) connection for a new data >>> connection, so this is somewhat unlikely corner case. However, it is >>> possible that a misbehaving source could cause conntrack entries be >>> created that could then interfere with new related connections. >>> >>> Fix this in the OVS module by deleting the clashing conntrack entry >>> after an expectation has been matched. This causes the following >>> nf_conntrack_in() call also find the expectation and remove it when >>> creating the new conntrack entry, as well as the forthcoming reply >>> direction packets to match the new related connection instead of the >>> old clashing conntrack entry. >>> >>> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action") >>> Reported-by: Yang Song <yangsong@vmware.com> >>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> >>> --- >> >> Hi Jarno, >> >>> v3: Removed unnecessary if statement. >>> v2: Fixed commit title. >>> >>> net/openvswitch/conntrack.c | 31 ++++++++++++++++++++++++++++++- >>> 1 file changed, 30 insertions(+), 1 deletion(-) >>> >>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c >>> index 7b2c2fc..d796ae7 100644 >>> --- a/net/openvswitch/conntrack.c >>> +++ b/net/openvswitch/conntrack.c >>> @@ -514,10 +514,39 @@ ovs_ct_expect_find(struct net *net, const struct nf_conntrack_zone *zone, >>> u16 proto, const struct sk_buff *skb) >>> { >>> struct nf_conntrack_tuple tuple; >>> + struct nf_conntrack_expect *exp; >>> >>> if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, net, &tuple)) >>> return NULL; >>> - return __nf_ct_expect_find(net, zone, &tuple); >>> + >>> + exp = __nf_ct_expect_find(net, zone, &tuple); >>> + >> >> Extraneous whitespace^ >> > > You mean the empty line? Yeah. >>> + if (exp) { >>> + struct nf_conntrack_tuple_hash *h; >>> + >>> + /* Delete existing conntrack entry, if it clashes with the >>> + * expectation. This can happen since conntrack ALGs do not >>> + * check for clashes between (new) expectations and existing >>> + * conntrack entries. nf_conntrack_in() will check the >>> + * expectations only if a conntrack entry can not be found, >>> + * which can lead to OVS finding the expectation (here) in the >>> + * init direction, but which will not be removed by the >>> + * nf_conntrack_in() call, if a matching conntrack entry is >>> + * found instead. In this case all init direction packets >>> + * would be reported as new related packets, while reply >>> + * direction packets would be reported as un-related >>> + * established packets. */ >>> + >> >> Extraneous whitespace^ >> >>> + h = nf_conntrack_find_get(net, zone, &tuple); >>> + if (h) { >>> + struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h); >>> + >>> + nf_ct_delete(ct, 0, 0); >>> + nf_conntrack_put(&ct->ct_general); >> >> Do we need the extra nf_conntrack_put() here? If >> nf_conntrack_find_get() returns an entry, we'll call nf_ct_delete() >> which releases a reference on the CT entry. > > There is one reference held by the table, but nf_conntrack_find_get() takes another. nf_ct_delete() releases the reference held by the table as the entry is removed, but we need to explicitly release the reference taken by nf_conntrack_find_get(). Ah, makes sense. Acked-by: Joe Stringer <joe@ovn.org> Did you intend for Pablo to take this? Pablo, is this fine or should Jarno resubmit against net? -- 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 Tue, Apr 18, 2017 at 12:24:28PM -0700, Joe Stringer wrote: > On 18 April 2017 at 11:33, Jarno Rajahalme <jarno@ovn.org> wrote: > > > >> On Apr 18, 2017, at 11:27 AM, Joe Stringer <joe@ovn.org> wrote: > >> > >> On 14 April 2017 at 14:26, Jarno Rajahalme <jarno@ovn.org> wrote: > >>> Conntrack helpers do not check for a potentially clashing conntrack > >>> entry when creating a new expectation. Also, nf_conntrack_in() will > >>> check expectations (via init_conntrack()) only if a conntrack entry > >>> can not be found. The expectation for a packet which also matches an > >>> existing conntrack entry will not be removed by conntrack, and is > >>> currently handled inconsistently by OVS, as OVS expects the > >>> expectation to be removed when the connection tracking entry matching > >>> that expectation is confirmed. > >>> > >>> It should be noted that normally an IP stack would not allow reuse of > >>> a 5-tuple of an old (possibly lingering) connection for a new data > >>> connection, so this is somewhat unlikely corner case. However, it is > >>> possible that a misbehaving source could cause conntrack entries be > >>> created that could then interfere with new related connections. > >>> > >>> Fix this in the OVS module by deleting the clashing conntrack entry > >>> after an expectation has been matched. This causes the following > >>> nf_conntrack_in() call also find the expectation and remove it when > >>> creating the new conntrack entry, as well as the forthcoming reply > >>> direction packets to match the new related connection instead of the > >>> old clashing conntrack entry. > >>> > >>> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action") > >>> Reported-by: Yang Song <yangsong@vmware.com> > >>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > >>> --- > >> > >> Hi Jarno, > >> > >>> v3: Removed unnecessary if statement. > >>> v2: Fixed commit title. > >>> > >>> net/openvswitch/conntrack.c | 31 ++++++++++++++++++++++++++++++- > >>> 1 file changed, 30 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c > >>> index 7b2c2fc..d796ae7 100644 > >>> --- a/net/openvswitch/conntrack.c > >>> +++ b/net/openvswitch/conntrack.c > >>> @@ -514,10 +514,39 @@ ovs_ct_expect_find(struct net *net, const struct nf_conntrack_zone *zone, > >>> u16 proto, const struct sk_buff *skb) > >>> { > >>> struct nf_conntrack_tuple tuple; > >>> + struct nf_conntrack_expect *exp; > >>> > >>> if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, net, &tuple)) > >>> return NULL; > >>> - return __nf_ct_expect_find(net, zone, &tuple); > >>> + > >>> + exp = __nf_ct_expect_find(net, zone, &tuple); > >>> + > >> > >> Extraneous whitespace^ > >> > > > > You mean the empty line? > > Yeah. I can remove this here before applying if that is fine to you, so you don't need to resubmit. > >>> + if (exp) { > >>> + struct nf_conntrack_tuple_hash *h; > >>> + > >>> + /* Delete existing conntrack entry, if it clashes with the > >>> + * expectation. This can happen since conntrack ALGs do not > >>> + * check for clashes between (new) expectations and existing > >>> + * conntrack entries. nf_conntrack_in() will check the > >>> + * expectations only if a conntrack entry can not be found, > >>> + * which can lead to OVS finding the expectation (here) in the > >>> + * init direction, but which will not be removed by the > >>> + * nf_conntrack_in() call, if a matching conntrack entry is > >>> + * found instead. In this case all init direction packets > >>> + * would be reported as new related packets, while reply > >>> + * direction packets would be reported as un-related > >>> + * established packets. */ > >>> + > >> > >> Extraneous whitespace^ We're converging to netdev comment style, ie. * ... * established packets. */ I know we have a bunch of comments in netfilter ending like the one above, but ideally it would be good to use this comment style. > >> > >>> + h = nf_conntrack_find_get(net, zone, &tuple); > >>> + if (h) { > >>> + struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h); > >>> + > >>> + nf_ct_delete(ct, 0, 0); > >>> + nf_conntrack_put(&ct->ct_general); > >> > >> Do we need the extra nf_conntrack_put() here? If > >> nf_conntrack_find_get() returns an entry, we'll call nf_ct_delete() > >> which releases a reference on the CT entry. > > > > There is one reference held by the table, but nf_conntrack_find_get() takes another. nf_ct_delete() releases the reference held by the table as the entry is removed, but we need to explicitly release the reference taken by nf_conntrack_find_get(). > > Ah, makes sense. > > Acked-by: Joe Stringer <joe@ovn.org> > > Did you intend for Pablo to take this? Pablo, is this fine or should > Jarno resubmit against net? I can take this, yes. -- 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 Apr 19, 2017, at 3:30 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Tue, Apr 18, 2017 at 12:24:28PM -0700, Joe Stringer wrote: >> On 18 April 2017 at 11:33, Jarno Rajahalme <jarno@ovn.org> wrote: >>> >>>> On Apr 18, 2017, at 11:27 AM, Joe Stringer <joe@ovn.org> wrote: >>>> >>>> On 14 April 2017 at 14:26, Jarno Rajahalme <jarno@ovn.org> wrote: >>>>> Conntrack helpers do not check for a potentially clashing conntrack >>>>> entry when creating a new expectation. Also, nf_conntrack_in() will >>>>> check expectations (via init_conntrack()) only if a conntrack entry >>>>> can not be found. The expectation for a packet which also matches an >>>>> existing conntrack entry will not be removed by conntrack, and is >>>>> currently handled inconsistently by OVS, as OVS expects the >>>>> expectation to be removed when the connection tracking entry matching >>>>> that expectation is confirmed. >>>>> >>>>> It should be noted that normally an IP stack would not allow reuse of >>>>> a 5-tuple of an old (possibly lingering) connection for a new data >>>>> connection, so this is somewhat unlikely corner case. However, it is >>>>> possible that a misbehaving source could cause conntrack entries be >>>>> created that could then interfere with new related connections. >>>>> >>>>> Fix this in the OVS module by deleting the clashing conntrack entry >>>>> after an expectation has been matched. This causes the following >>>>> nf_conntrack_in() call also find the expectation and remove it when >>>>> creating the new conntrack entry, as well as the forthcoming reply >>>>> direction packets to match the new related connection instead of the >>>>> old clashing conntrack entry. >>>>> >>>>> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action") >>>>> Reported-by: Yang Song <yangsong@vmware.com> >>>>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> >>>>> --- >>>> >>>> Hi Jarno, >>>> >>>>> v3: Removed unnecessary if statement. >>>>> v2: Fixed commit title. >>>>> >>>>> net/openvswitch/conntrack.c | 31 ++++++++++++++++++++++++++++++- >>>>> 1 file changed, 30 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c >>>>> index 7b2c2fc..d796ae7 100644 >>>>> --- a/net/openvswitch/conntrack.c >>>>> +++ b/net/openvswitch/conntrack.c >>>>> @@ -514,10 +514,39 @@ ovs_ct_expect_find(struct net *net, const struct nf_conntrack_zone *zone, >>>>> u16 proto, const struct sk_buff *skb) >>>>> { >>>>> struct nf_conntrack_tuple tuple; >>>>> + struct nf_conntrack_expect *exp; >>>>> >>>>> if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, net, &tuple)) >>>>> return NULL; >>>>> - return __nf_ct_expect_find(net, zone, &tuple); >>>>> + >>>>> + exp = __nf_ct_expect_find(net, zone, &tuple); >>>>> + >>>> >>>> Extraneous whitespace^ >>>> >>> >>> You mean the empty line? >> >> Yeah. > > I can remove this here before applying if that is fine to you, so you > don't need to resubmit. > Yes, fine by me! Thank you! Jarno >>>>> + if (exp) { >>>>> + struct nf_conntrack_tuple_hash *h; >>>>> + >>>>> + /* Delete existing conntrack entry, if it clashes with the >>>>> + * expectation. This can happen since conntrack ALGs do not >>>>> + * check for clashes between (new) expectations and existing >>>>> + * conntrack entries. nf_conntrack_in() will check the >>>>> + * expectations only if a conntrack entry can not be found, >>>>> + * which can lead to OVS finding the expectation (here) in the >>>>> + * init direction, but which will not be removed by the >>>>> + * nf_conntrack_in() call, if a matching conntrack entry is >>>>> + * found instead. In this case all init direction packets >>>>> + * would be reported as new related packets, while reply >>>>> + * direction packets would be reported as un-related >>>>> + * established packets. */ >>>>> + >>>> >>>> Extraneous whitespace^ > > We're converging to netdev comment style, ie. > > * ... > * established packets. > */ > > I know we have a bunch of comments in netfilter ending like the one > above, but ideally it would be good to use this comment style. > >>>> >>>>> + h = nf_conntrack_find_get(net, zone, &tuple); >>>>> + if (h) { >>>>> + struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h); >>>>> + >>>>> + nf_ct_delete(ct, 0, 0); >>>>> + nf_conntrack_put(&ct->ct_general); >>>> >>>> Do we need the extra nf_conntrack_put() here? If >>>> nf_conntrack_find_get() returns an entry, we'll call nf_ct_delete() >>>> which releases a reference on the CT entry. >>> >>> There is one reference held by the table, but nf_conntrack_find_get() takes another. nf_ct_delete() releases the reference held by the table as the entry is removed, but we need to explicitly release the reference taken by nf_conntrack_find_get(). >> >> Ah, makes sense. >> >> Acked-by: Joe Stringer <joe@ovn.org> >> >> Did you intend for Pablo to take this? Pablo, is this fine or should >> Jarno resubmit against net? > > I can take this, yes. -- 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
Pablo, Were you waiting for a v4 or are you ready to take this as-is? Jarno > On Apr 19, 2017, at 12:56 PM, Jarno Rajahalme <jarno@ovn.org> wrote: > >> >> On Apr 19, 2017, at 3:30 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: >> >> On Tue, Apr 18, 2017 at 12:24:28PM -0700, Joe Stringer wrote: >>> On 18 April 2017 at 11:33, Jarno Rajahalme <jarno@ovn.org> wrote: >>>> >>>>> On Apr 18, 2017, at 11:27 AM, Joe Stringer <joe@ovn.org> wrote: >>>>> >>>>> On 14 April 2017 at 14:26, Jarno Rajahalme <jarno@ovn.org> wrote: >>>>>> Conntrack helpers do not check for a potentially clashing conntrack >>>>>> entry when creating a new expectation. Also, nf_conntrack_in() will >>>>>> check expectations (via init_conntrack()) only if a conntrack entry >>>>>> can not be found. The expectation for a packet which also matches an >>>>>> existing conntrack entry will not be removed by conntrack, and is >>>>>> currently handled inconsistently by OVS, as OVS expects the >>>>>> expectation to be removed when the connection tracking entry matching >>>>>> that expectation is confirmed. >>>>>> >>>>>> It should be noted that normally an IP stack would not allow reuse of >>>>>> a 5-tuple of an old (possibly lingering) connection for a new data >>>>>> connection, so this is somewhat unlikely corner case. However, it is >>>>>> possible that a misbehaving source could cause conntrack entries be >>>>>> created that could then interfere with new related connections. >>>>>> >>>>>> Fix this in the OVS module by deleting the clashing conntrack entry >>>>>> after an expectation has been matched. This causes the following >>>>>> nf_conntrack_in() call also find the expectation and remove it when >>>>>> creating the new conntrack entry, as well as the forthcoming reply >>>>>> direction packets to match the new related connection instead of the >>>>>> old clashing conntrack entry. >>>>>> >>>>>> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action") >>>>>> Reported-by: Yang Song <yangsong@vmware.com> >>>>>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> >>>>>> --- >>>>> >>>>> Hi Jarno, >>>>> >>>>>> v3: Removed unnecessary if statement. >>>>>> v2: Fixed commit title. >>>>>> >>>>>> net/openvswitch/conntrack.c | 31 ++++++++++++++++++++++++++++++- >>>>>> 1 file changed, 30 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c >>>>>> index 7b2c2fc..d796ae7 100644 >>>>>> --- a/net/openvswitch/conntrack.c >>>>>> +++ b/net/openvswitch/conntrack.c >>>>>> @@ -514,10 +514,39 @@ ovs_ct_expect_find(struct net *net, const struct nf_conntrack_zone *zone, >>>>>> u16 proto, const struct sk_buff *skb) >>>>>> { >>>>>> struct nf_conntrack_tuple tuple; >>>>>> + struct nf_conntrack_expect *exp; >>>>>> >>>>>> if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, net, &tuple)) >>>>>> return NULL; >>>>>> - return __nf_ct_expect_find(net, zone, &tuple); >>>>>> + >>>>>> + exp = __nf_ct_expect_find(net, zone, &tuple); >>>>>> + >>>>> >>>>> Extraneous whitespace^ >>>>> >>>> >>>> You mean the empty line? >>> >>> Yeah. >> >> I can remove this here before applying if that is fine to you, so you >> don't need to resubmit. >> > > Yes, fine by me! Thank you! > > Jarno > >>>>>> + if (exp) { >>>>>> + struct nf_conntrack_tuple_hash *h; >>>>>> + >>>>>> + /* Delete existing conntrack entry, if it clashes with the >>>>>> + * expectation. This can happen since conntrack ALGs do not >>>>>> + * check for clashes between (new) expectations and existing >>>>>> + * conntrack entries. nf_conntrack_in() will check the >>>>>> + * expectations only if a conntrack entry can not be found, >>>>>> + * which can lead to OVS finding the expectation (here) in the >>>>>> + * init direction, but which will not be removed by the >>>>>> + * nf_conntrack_in() call, if a matching conntrack entry is >>>>>> + * found instead. In this case all init direction packets >>>>>> + * would be reported as new related packets, while reply >>>>>> + * direction packets would be reported as un-related >>>>>> + * established packets. */ >>>>>> + >>>>> >>>>> Extraneous whitespace^ >> >> We're converging to netdev comment style, ie. >> >> * ... >> * established packets. >> */ >> >> I know we have a bunch of comments in netfilter ending like the one >> above, but ideally it would be good to use this comment style. >> >>>>> >>>>>> + h = nf_conntrack_find_get(net, zone, &tuple); >>>>>> + if (h) { >>>>>> + struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h); >>>>>> + >>>>>> + nf_ct_delete(ct, 0, 0); >>>>>> + nf_conntrack_put(&ct->ct_general); >>>>> >>>>> Do we need the extra nf_conntrack_put() here? If >>>>> nf_conntrack_find_get() returns an entry, we'll call nf_ct_delete() >>>>> which releases a reference on the CT entry. >>>> >>>> There is one reference held by the table, but nf_conntrack_find_get() takes another. nf_ct_delete() releases the reference held by the table as the entry is removed, but we need to explicitly release the reference taken by nf_conntrack_find_get(). >>> >>> Ah, makes sense. >>> >>> Acked-by: Joe Stringer <joe@ovn.org> >>> >>> Did you intend for Pablo to take this? Pablo, is this fine or should >>> Jarno resubmit against net? >> >> I can take this, yes. -- 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 Fri, Apr 14, 2017 at 02:26:38PM -0700, Jarno Rajahalme wrote: > Conntrack helpers do not check for a potentially clashing conntrack > entry when creating a new expectation. Also, nf_conntrack_in() will > check expectations (via init_conntrack()) only if a conntrack entry > can not be found. The expectation for a packet which also matches an > existing conntrack entry will not be removed by conntrack, and is > currently handled inconsistently by OVS, as OVS expects the > expectation to be removed when the connection tracking entry matching > that expectation is confirmed. > > It should be noted that normally an IP stack would not allow reuse of > a 5-tuple of an old (possibly lingering) connection for a new data > connection, so this is somewhat unlikely corner case. However, it is > possible that a misbehaving source could cause conntrack entries be > created that could then interfere with new related connections. > > Fix this in the OVS module by deleting the clashing conntrack entry > after an expectation has been matched. This causes the following > nf_conntrack_in() call also find the expectation and remove it when > creating the new conntrack entry, as well as the forthcoming reply > direction packets to match the new related connection instead of the > old clashing conntrack entry. Applied, with minor changes that we have already discussed here. Thanks. -- 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 7b2c2fc..d796ae7 100644 --- a/net/openvswitch/conntrack.c +++ b/net/openvswitch/conntrack.c @@ -514,10 +514,39 @@ ovs_ct_expect_find(struct net *net, const struct nf_conntrack_zone *zone, u16 proto, const struct sk_buff *skb) { struct nf_conntrack_tuple tuple; + struct nf_conntrack_expect *exp; if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, net, &tuple)) return NULL; - return __nf_ct_expect_find(net, zone, &tuple); + + exp = __nf_ct_expect_find(net, zone, &tuple); + + if (exp) { + struct nf_conntrack_tuple_hash *h; + + /* Delete existing conntrack entry, if it clashes with the + * expectation. This can happen since conntrack ALGs do not + * check for clashes between (new) expectations and existing + * conntrack entries. nf_conntrack_in() will check the + * expectations only if a conntrack entry can not be found, + * which can lead to OVS finding the expectation (here) in the + * init direction, but which will not be removed by the + * nf_conntrack_in() call, if a matching conntrack entry is + * found instead. In this case all init direction packets + * would be reported as new related packets, while reply + * direction packets would be reported as un-related + * established packets. */ + + h = nf_conntrack_find_get(net, zone, &tuple); + if (h) { + struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h); + + nf_ct_delete(ct, 0, 0); + nf_conntrack_put(&ct->ct_general); + } + } + + return exp; } /* This replicates logic from nf_conntrack_core.c that is not exported. */
Conntrack helpers do not check for a potentially clashing conntrack entry when creating a new expectation. Also, nf_conntrack_in() will check expectations (via init_conntrack()) only if a conntrack entry can not be found. The expectation for a packet which also matches an existing conntrack entry will not be removed by conntrack, and is currently handled inconsistently by OVS, as OVS expects the expectation to be removed when the connection tracking entry matching that expectation is confirmed. It should be noted that normally an IP stack would not allow reuse of a 5-tuple of an old (possibly lingering) connection for a new data connection, so this is somewhat unlikely corner case. However, it is possible that a misbehaving source could cause conntrack entries be created that could then interfere with new related connections. Fix this in the OVS module by deleting the clashing conntrack entry after an expectation has been matched. This causes the following nf_conntrack_in() call also find the expectation and remove it when creating the new conntrack entry, as well as the forthcoming reply direction packets to match the new related connection instead of the old clashing conntrack entry. Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action") Reported-by: Yang Song <yangsong@vmware.com> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> --- v3: Removed unnecessary if statement. v2: Fixed commit title. net/openvswitch/conntrack.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)