| Message ID | 1445018901-18839-3-git-send-email-joestringer@nicira.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On 10/16/15 at 11:08am, Joe Stringer wrote: > New, related connections are marked as such as part of ovs_ct_lookup(), > but they are not marked as "new" if the commit flag is used. Make this > consistent by treating IP_CT_RELATED as new as well. > > Reported-by: Jarno Rajahalme <jrajahalme@nicira.com> > Signed-off-by: Joe Stringer <joestringer@nicira.com> > Acked-by: Pravin B Shelar <pshelar@nicira.com> > --- > v2: Acked. > --- > net/openvswitch/conntrack.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c > index 80bf702715bb..480dbb9095b7 100644 > --- a/net/openvswitch/conntrack.c > +++ b/net/openvswitch/conntrack.c > @@ -86,6 +86,8 @@ static u8 ovs_ct_get_state(enum ip_conntrack_info ctinfo) > ct_state |= OVS_CS_F_ESTABLISHED; > break; > case IP_CT_RELATED: > + ct_state |= OVS_CS_F_NEW; > + /* Fall through */ > case IP_CT_RELATED_REPLY: > ct_state |= OVS_CS_F_RELATED; > break; I'm probably missing something obvious. Why is the reply direction not considered NEW? Wouldn't this consider an ICMPv6 as related+new depending on simply the direction? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17 October 2015 at 00:52, Thomas Graf <tgraf@suug.ch> wrote: > On 10/16/15 at 11:08am, Joe Stringer wrote: >> New, related connections are marked as such as part of ovs_ct_lookup(), >> but they are not marked as "new" if the commit flag is used. Make this >> consistent by treating IP_CT_RELATED as new as well. >> >> Reported-by: Jarno Rajahalme <jrajahalme@nicira.com> >> Signed-off-by: Joe Stringer <joestringer@nicira.com> >> Acked-by: Pravin B Shelar <pshelar@nicira.com> >> --- >> v2: Acked. >> --- >> net/openvswitch/conntrack.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c >> index 80bf702715bb..480dbb9095b7 100644 >> --- a/net/openvswitch/conntrack.c >> +++ b/net/openvswitch/conntrack.c >> @@ -86,6 +86,8 @@ static u8 ovs_ct_get_state(enum ip_conntrack_info ctinfo) >> ct_state |= OVS_CS_F_ESTABLISHED; >> break; >> case IP_CT_RELATED: >> + ct_state |= OVS_CS_F_NEW; >> + /* Fall through */ >> case IP_CT_RELATED_REPLY: >> ct_state |= OVS_CS_F_RELATED; >> break; > > I'm probably missing something obvious. Why is the reply direction > not considered NEW? Wouldn't this consider an ICMPv6 as related+new > depending on simply the direction? My thoughts were along the lines "If something is a reply, that implies that state is held, and therefore it cannot be NEW (where NEW means no state is available)". However, if you consider that the 'related' connection is an independent connection with its own state, but the 'reply' bit refers to the original connection, my original premise breaks. Furthermore, looking at how it's used in netfilter core and the ICMP proto handler, it looks like both of these cases should be considered NEW. I can respin. Do you have a specific case in mind here? It would be useful for extending the OVS testsuite. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/19/15 at 12:07am, Joe Stringer wrote: > > I'm probably missing something obvious. Why is the reply direction > > not considered NEW? Wouldn't this consider an ICMPv6 as related+new > > depending on simply the direction? > > My thoughts were along the lines "If something is a reply, that > implies that state is held, and therefore it cannot be NEW (where NEW > means no state is available)". However, if you consider that the > 'related' connection is an independent connection with its own state, > but the 'reply' bit refers to the original connection, my original > premise breaks. Furthermore, looking at how it's used in netfilter > core and the ICMP proto handler, it looks like both of these cases > should be considered NEW. I can respin. > > Do you have a specific case in mind here? It would be useful for > extending the OVS testsuite. It's tricky. A typical use case would be an active FTP connection where the data connection is established in the reply direction and marked related if I'm not mistaken. OTOH, an ICMP sent in response should not be considered NEW. It really depends on our definition of NEW towards the user. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19 October 2015 at 02:03, Thomas Graf <tgraf@suug.ch> wrote: > On 10/19/15 at 12:07am, Joe Stringer wrote: >> > I'm probably missing something obvious. Why is the reply direction >> > not considered NEW? Wouldn't this consider an ICMPv6 as related+new >> > depending on simply the direction? >> >> My thoughts were along the lines "If something is a reply, that >> implies that state is held, and therefore it cannot be NEW (where NEW >> means no state is available)". However, if you consider that the >> 'related' connection is an independent connection with its own state, >> but the 'reply' bit refers to the original connection, my original >> premise breaks. Furthermore, looking at how it's used in netfilter >> core and the ICMP proto handler, it looks like both of these cases >> should be considered NEW. I can respin. >> >> Do you have a specific case in mind here? It would be useful for >> extending the OVS testsuite. > > It's tricky. A typical use case would be an active FTP connection > where the data connection is established in the reply direction > and marked related if I'm not mistaken. > > OTOH, an ICMP sent in response should not be considered NEW. It > really depends on our definition of NEW towards the user. ICMP and FTP are handled a bit differently: ICMP protocol handling is based on the original connection; however for expected connections like FTP data, there is a separate conntrack entry. I agree that for ICMP errors, they should not be NEW; but for active FTP connections, I'd expect the data connection to be NEW (if it wasn't already seen&established). Along these lines, my earlier assertion that the 'reply' flag is based on the original connection is only applicable for ICMP responses, but not for FTP data connections. I think that the proper solution instead of this patch is to set NEW if !nf_ct_is_confirmed(ct). This is more accurately the meaning for 'NEW' that we are actually trying to expose. As long as this is done before confirming the connection during a "commit", this will make the behaviour consistent with the lack of commit flag. I'm sending a patch. Thanks for the feedback! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/19/15 at 04:13pm, Joe Stringer wrote: > I think that the proper solution instead of this patch is to set NEW > if !nf_ct_is_confirmed(ct). This is more accurately the meaning for > 'NEW' that we are actually trying to expose. As long as this is done > before confirming the connection during a "commit", this will make the > behaviour consistent with the lack of commit flag. I'm sending a > patch. Thanks for the feedback! Agreed. This sounds perfect. -- To unsubscribe from this list: send the line "unsubscribe netdev" 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 80bf702715bb..480dbb9095b7 100644 --- a/net/openvswitch/conntrack.c +++ b/net/openvswitch/conntrack.c @@ -86,6 +86,8 @@ static u8 ovs_ct_get_state(enum ip_conntrack_info ctinfo) ct_state |= OVS_CS_F_ESTABLISHED; break; case IP_CT_RELATED: + ct_state |= OVS_CS_F_NEW; + /* Fall through */ case IP_CT_RELATED_REPLY: ct_state |= OVS_CS_F_RELATED; break;