| Message ID | 20231019112243.2421-2-viacheslav.galaktionov@arknetworks.am |
|---|---|
| State | Superseded |
| Delegated to: | aaron conole |
| Headers | show |
| Series | [ovs-dev,v3,1/3] lib/conntrack: Only use given packet in protocol detection. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Viacheslav Galaktionov <Viacheslav.Galaktionov@arknetworks.am> writes: > On 11/27/23 19:33, Aaron Conole wrote: >> Viacheslav Galaktionov <Viacheslav.Galaktionov@arknetworks.am> writes: >> >>> On 11/22/23 16:14, Aaron Conole wrote: >>>>> Viacheslav Galaktionov writes: >>>>> >>>>> When a packet hits a flow rule without an explicitly specified helper, >>>>> OvS has to rely on automatic application layer gateway detection to >>>>> find related connections. This works as long as services are running on >>>>> their standard ports, e.g. when FTP servers use TCP port 21. >>>>> >>>>> However, sometimes it's necessary to run services on non-standard ports. >>>>> In that case, there is no way for OvS to guess which protocol is used >>>>> within a given flow. Of course, this means that no related connections >>>>> can be recognized. >>>>> >>>>> When a connection is committed with a particular helper, it's reasonable >>>>> to assume this helper will be used in subsequent CT actions, as long as >>>>> they don't override it. Achieve this behaviour by using the committed >>>>> connection's helper when a flow rule does not specify one. >>>>> >>>>> Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@arknetworks.am> >>>>> Acked-by: Ivan Malov <ivan.malov@arknetworks.am> >>>>> --- >>>>> lib/conntrack.c | 9 +++++++++ >>>>> 1 file changed, 9 insertions(+) >>>> Hi Viacheslav, >>>> >>>> Do you plan to send a v4 which has news and faq updated? >>> Hi Aaron! >>> >>> I was actually waiting for your comments, so I could address them and >>> update the NEWS and FAQ at the same time. I can send a v4 in the next >>> couple of days if you want. >> Sorry - that's my mistake. Please send the v4. >> > Hi Aaron, > > Sorry, I've only just now taken a proper look at the FAQ, and I don't > see what > I should change there. There doesn't seem to be any mention of this subject > in the file and my patches actually make netdev datapaths behave in the same > manner as system datapaths when it comes to conntrack helpers, so there's > probably no point describing a difference that's no longer there. I was thinking that it would make sense to update the following from Documentation/faq/releases.rst: > Q: Are all features available with all datapaths? > > A: Open vSwitch supports different datapaths on different platforms. Each > datapath has a different feature set: the following tables try to summarize > the status. There is a table there where we try to list any compatibility or differences. Helper behavior here is a difference in older versions so we could put something like: Conntrack Helper Assign. YES YES 3.2 Maybe it would introduce more confusion that it removes? But since it is a behavior change which aligns the datapaths, it would be good to document it and that question does deal with some of it.
Viacheslav Galaktionov <Viacheslav.Galaktionov@arknetworks.am> writes: > On 12/4/23 11:23, Viacheslav Galaktionov wrote: >> >> >> On 12/1/23 15:32, Aaron Conole wrote: >>> Viacheslav Galaktionov <Viacheslav.Galaktionov@arknetworks.am> writes: >>> >>>> On 11/27/23 19:33, Aaron Conole wrote: >>>>> Viacheslav Galaktionov <Viacheslav.Galaktionov@arknetworks.am> writes: >>>>> >>>>>> On 11/22/23 16:14, Aaron Conole wrote: >>>>>>>> Viacheslav Galaktionov writes: >>>>>>>> >>>>>>>> When a packet hits a flow rule without an explicitly specified >>>>>>>> helper, >>>>>>>> OvS has to rely on automatic application layer gateway detection to >>>>>>>> find related connections. This works as long as services are >>>>>>>> running on >>>>>>>> their standard ports, e.g. when FTP servers use TCP port 21. >>>>>>>> >>>>>>>> However, sometimes it's necessary to run services on >>>>>>>> non-standard ports. >>>>>>>> In that case, there is no way for OvS to guess which protocol >>>>>>>> is used >>>>>>>> within a given flow. Of course, this means that no related >>>>>>>> connections >>>>>>>> can be recognized. >>>>>>>> >>>>>>>> When a connection is committed with a particular helper, it's >>>>>>>> reasonable >>>>>>>> to assume this helper will be used in subsequent CT actions, >>>>>>>> as long as >>>>>>>> they don't override it. Achieve this behaviour by using the >>>>>>>> committed >>>>>>>> connection's helper when a flow rule does not specify one. >>>>>>>> >>>>>>>> Signed-off-by: Viacheslav Galaktionov >>>>>>>> <viacheslav.galaktionov@arknetworks.am> >>>>>>>> Acked-by: Ivan Malov <ivan.malov@arknetworks.am> >>>>>>>> --- >>>>>>>> lib/conntrack.c | 9 +++++++++ >>>>>>>> 1 file changed, 9 insertions(+) >>>>>>> Hi Viacheslav, >>>>>>> >>>>>>> Do you plan to send a v4 which has news and faq updated? >>>>>> Hi Aaron! >>>>>> >>>>>> I was actually waiting for your comments, so I could address them and >>>>>> update the NEWS and FAQ at the same time. I can send a v4 in the next >>>>>> couple of days if you want. >>>>> Sorry - that's my mistake. Please send the v4. >>>>> >>>> Hi Aaron, >>>> >>>> Sorry, I've only just now taken a proper look at the FAQ, and I don't >>>> see what >>>> I should change there. There doesn't seem to be any mention of >>>> this subject >>>> in the file and my patches actually make netdev datapaths behave >>>> in the same >>>> manner as system datapaths when it comes to conntrack helpers, so >>>> there's >>>> probably no point describing a difference that's no longer there. >>> I was thinking that it would make sense to update the following from >>> Documentation/faq/releases.rst: >>> >>>> Q: Are all features available with all datapaths? >>>> >>>> A: Open vSwitch supports different datapaths on different >>>> platforms. Each >>>> datapath has a different feature set: the following tables >>>> try to summarize >>>> the status. >>> There is a table there where we try to list any compatibility or >>> differences. Helper behavior here is a difference in older versions so >>> we could put something like: >>> >>> Conntrack Helper Assign. YES YES 3.2 >>> >>> Maybe it would introduce more confusion that it removes? But since it >>> is a behavior change which aligns the datapaths, it would be good to >>> document it and that question does deal with some of it. >>> >> Ok, I guess that makes sense. I've looked at the Hyper-V datapath, >> seems like >> it only uses the helper specified in a given rule, so I'm going to >> put a NO in >> the table. I don't really have any means to test changes to Hyper-V, >> so I can't >> implement it myself. I hope that's okay. > Although now it looks like Hyper-V can't use helpers at all. Should we > change > "Assign." to "Persist." to indicate that helpers persist across > different flow > rules? I'm not absolutely sure about this phrasing too, I think it may > require > some additional explanation somewhere? Maybe Alin has some suggestion.
diff --git a/lib/conntrack.c b/lib/conntrack.c index c27ac5a6f..59a4a413f 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -1245,6 +1245,10 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, conn = NULL; } + if (conn && helper == NULL) { + helper = conn->alg; + } + enum ct_alg_ctl_type ct_alg_ctl = get_alg_ctl_type(pkt, helper); if (OVS_LIKELY(conn)) { @@ -1334,6 +1338,11 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch, DP_PACKET_BATCH_FOR_EACH (i, packet, pkt_batch) { struct conn *conn = packet->md.conn; + + if (helper == NULL && conn != NULL) { + helper = conn->alg; + } + if (OVS_UNLIKELY(packet->md.ct_state == CS_INVALID)) { write_ct_md(packet, zone, NULL, NULL, NULL); } else if (conn &&