Re: [ovs-dev] [PATCH v2 1/5] ofproto-dpif-xlate: Don't unwildcard tunnel attributes on set.
OK, thanks. I'll mark this series as "superseded" in patchwork. On Tue, Oct 04, 2016 at 12:42:19AM +, Daniele Di Proietto wrote: > I'm rebasing the series and considering a different approach for this. > > I'll send something shortly. > > Thanks, > > Daniele > > On 03/10/2016 17:38, "Ben Pfaff" wrote: > > >Does this patch need anything more? I don't see it on master and it's > >still in patchwork. Same thing for patch 4/5 in this series. > > > >On Thu, Sep 01, 2016 at 01:31:06AM +, Daniele Di Proietto wrote: > >> > >> On 31/08/2016 11:32, "Jarno Rajahalme" wrote: > >> > >> >I’d put the registers and metadata field to the ‘false’ and also maybe > >> >non-writeable fields (ether type, frags, nw_proto, etc.) in to > >> >OVS_NOT_REACHED() case, just in case. > >> > > >> > Jarno > >> > >> Agreed, done > >> > >> Thanks, > >> > >> Daniele > >> > >> > > >> >> On Aug 31, 2016, at 10:38 AM, Jesse Gross wrote: > >> >> > >> >> On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto > >> >> wrote: > >> >>> When translating a set action we also unwildcard the field in question. > >> >>> This is done to correctly translate set actions with the value > >> >>> identical > >> >>> to the ingress flow, like in the following example: > >> >>> > >> >>> flow table: > >> >>> > >> >>> tcp,actions=set_field:80->tcp_dst,output:5 > >> >>> > >> >>> ingress packet: > >> >>> > >> >>> ...,tcp,tcp_dst=80 > >> >>> > >> >>> datapath flow > >> >>> > >> >>> ...,tcp(dst=80) actions:5 > >> >>> > >> >>> The datapath flow must exact match the target field, because the > >> >>> actions > >> >>> do not include a set field. (Otherwise a packet coming in with a > >> >>> different tcp_dst would be matched, and its port wouldn't be altered). > >> >>> > >> >>> Tunnel attributes behave differently: at the datapath layer, before > >> >>> action processing they're cleared (we do the same at the ofproto layer > >> >>> in xlate_actions()). Therefore there's no need to unwildcard them, > >> >>> because a set action would always be detected (since we zero them at > >> >>> the > >> >>> beginning of xlate_ations()). > >> >>> > >> >>> This fixes a problem related to the handling of Geneve options. > >> >>> Unwildcarding non existing Geneve options (as done by a > >> >>> set_field:X->tun_metadata on a packet coming from a non-tunnel > >> >>> interface) would be problematic for the datapaths: the ODP translation > >> >>> functions cannot express a match on non existing Geneve options (unlike > >> >>> on other attributes), and the userspace datapath wouldn't be able to > >> >>> translate them to "userspace datapath format". In both cases the > >> >>> installed flow would be deleted by revalidation at the first > >> >>> opportunity. > >> >>> > >> >>> Signed-off-by: Daniele Di Proietto > >> >> > >> >> I think there might be some more obscure ways of triggering this > >> >> problem that still exist. Basically anything that can use a register > >> >> as a target is a potential issue. For example, stack_pop, bundle, and > >> >> multipath all look like they have the same masking behavior. > >> >> > >> >> I do have a general solution in this patch (look at the bottom of > >> >> xlate_actions() where it is adjusting the wildcards): > >> >> http://openvswitch.org/pipermail/dev/2016-August/078484.html > >> >> > >> >> I didn't realize that it was addressing an existing issue though and > >> >> that patch certainly isn't suitable for anything other than master. > >> >> > >> >> I'm also not really a big fan of the way I handled it there since it's > >> >> a pretty coarse way to do it. I would be happy to drop it if we can > >> >> feel comfortable that we got all of the callsites (now and in the > >> >> future) using your method. Perhaps we can just create a helper > >> >> function with this check builtin and then use it everywhere? That > >> >> might be enough to be confident about the future. > >> >> ___ > >> >> dev mailing list > >> >> [email protected] > >> >> http://openvswitch.org/mailman/listinfo/dev > >> > > >> ___ > >> dev mailing list > >> [email protected] > >> http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 1/5] ofproto-dpif-xlate: Don't unwildcard tunnel attributes on set.
I'm rebasing the series and considering a different approach for this. I'll send something shortly. Thanks, Daniele On 03/10/2016 17:38, "Ben Pfaff" wrote: >Does this patch need anything more? I don't see it on master and it's >still in patchwork. Same thing for patch 4/5 in this series. > >On Thu, Sep 01, 2016 at 01:31:06AM +, Daniele Di Proietto wrote: >> >> On 31/08/2016 11:32, "Jarno Rajahalme" wrote: >> >> >I’d put the registers and metadata field to the ‘false’ and also maybe >> >non-writeable fields (ether type, frags, nw_proto, etc.) in to >> >OVS_NOT_REACHED() case, just in case. >> > >> > Jarno >> >> Agreed, done >> >> Thanks, >> >> Daniele >> >> > >> >> On Aug 31, 2016, at 10:38 AM, Jesse Gross wrote: >> >> >> >> On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto >> >> wrote: >> >>> When translating a set action we also unwildcard the field in question. >> >>> This is done to correctly translate set actions with the value identical >> >>> to the ingress flow, like in the following example: >> >>> >> >>> flow table: >> >>> >> >>> tcp,actions=set_field:80->tcp_dst,output:5 >> >>> >> >>> ingress packet: >> >>> >> >>> ...,tcp,tcp_dst=80 >> >>> >> >>> datapath flow >> >>> >> >>> ...,tcp(dst=80) actions:5 >> >>> >> >>> The datapath flow must exact match the target field, because the actions >> >>> do not include a set field. (Otherwise a packet coming in with a >> >>> different tcp_dst would be matched, and its port wouldn't be altered). >> >>> >> >>> Tunnel attributes behave differently: at the datapath layer, before >> >>> action processing they're cleared (we do the same at the ofproto layer >> >>> in xlate_actions()). Therefore there's no need to unwildcard them, >> >>> because a set action would always be detected (since we zero them at the >> >>> beginning of xlate_ations()). >> >>> >> >>> This fixes a problem related to the handling of Geneve options. >> >>> Unwildcarding non existing Geneve options (as done by a >> >>> set_field:X->tun_metadata on a packet coming from a non-tunnel >> >>> interface) would be problematic for the datapaths: the ODP translation >> >>> functions cannot express a match on non existing Geneve options (unlike >> >>> on other attributes), and the userspace datapath wouldn't be able to >> >>> translate them to "userspace datapath format". In both cases the >> >>> installed flow would be deleted by revalidation at the first >> >>> opportunity. >> >>> >> >>> Signed-off-by: Daniele Di Proietto >> >> >> >> I think there might be some more obscure ways of triggering this >> >> problem that still exist. Basically anything that can use a register >> >> as a target is a potential issue. For example, stack_pop, bundle, and >> >> multipath all look like they have the same masking behavior. >> >> >> >> I do have a general solution in this patch (look at the bottom of >> >> xlate_actions() where it is adjusting the wildcards): >> >> http://openvswitch.org/pipermail/dev/2016-August/078484.html >> >> >> >> I didn't realize that it was addressing an existing issue though and >> >> that patch certainly isn't suitable for anything other than master. >> >> >> >> I'm also not really a big fan of the way I handled it there since it's >> >> a pretty coarse way to do it. I would be happy to drop it if we can >> >> feel comfortable that we got all of the callsites (now and in the >> >> future) using your method. Perhaps we can just create a helper >> >> function with this check builtin and then use it everywhere? That >> >> might be enough to be confident about the future. >> >> ___ >> >> dev mailing list >> >> [email protected] >> >> http://openvswitch.org/mailman/listinfo/dev >> > >> ___ >> dev mailing list >> [email protected] >> http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 1/5] ofproto-dpif-xlate: Don't unwildcard tunnel attributes on set.
Does this patch need anything more? I don't see it on master and it's still in patchwork. Same thing for patch 4/5 in this series. On Thu, Sep 01, 2016 at 01:31:06AM +, Daniele Di Proietto wrote: > > On 31/08/2016 11:32, "Jarno Rajahalme" wrote: > > >I’d put the registers and metadata field to the ‘false’ and also maybe > >non-writeable fields (ether type, frags, nw_proto, etc.) in to > >OVS_NOT_REACHED() case, just in case. > > > > Jarno > > Agreed, done > > Thanks, > > Daniele > > > > >> On Aug 31, 2016, at 10:38 AM, Jesse Gross wrote: > >> > >> On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto > >> wrote: > >>> When translating a set action we also unwildcard the field in question. > >>> This is done to correctly translate set actions with the value identical > >>> to the ingress flow, like in the following example: > >>> > >>> flow table: > >>> > >>> tcp,actions=set_field:80->tcp_dst,output:5 > >>> > >>> ingress packet: > >>> > >>> ...,tcp,tcp_dst=80 > >>> > >>> datapath flow > >>> > >>> ...,tcp(dst=80) actions:5 > >>> > >>> The datapath flow must exact match the target field, because the actions > >>> do not include a set field. (Otherwise a packet coming in with a > >>> different tcp_dst would be matched, and its port wouldn't be altered). > >>> > >>> Tunnel attributes behave differently: at the datapath layer, before > >>> action processing they're cleared (we do the same at the ofproto layer > >>> in xlate_actions()). Therefore there's no need to unwildcard them, > >>> because a set action would always be detected (since we zero them at the > >>> beginning of xlate_ations()). > >>> > >>> This fixes a problem related to the handling of Geneve options. > >>> Unwildcarding non existing Geneve options (as done by a > >>> set_field:X->tun_metadata on a packet coming from a non-tunnel > >>> interface) would be problematic for the datapaths: the ODP translation > >>> functions cannot express a match on non existing Geneve options (unlike > >>> on other attributes), and the userspace datapath wouldn't be able to > >>> translate them to "userspace datapath format". In both cases the > >>> installed flow would be deleted by revalidation at the first > >>> opportunity. > >>> > >>> Signed-off-by: Daniele Di Proietto > >> > >> I think there might be some more obscure ways of triggering this > >> problem that still exist. Basically anything that can use a register > >> as a target is a potential issue. For example, stack_pop, bundle, and > >> multipath all look like they have the same masking behavior. > >> > >> I do have a general solution in this patch (look at the bottom of > >> xlate_actions() where it is adjusting the wildcards): > >> http://openvswitch.org/pipermail/dev/2016-August/078484.html > >> > >> I didn't realize that it was addressing an existing issue though and > >> that patch certainly isn't suitable for anything other than master. > >> > >> I'm also not really a big fan of the way I handled it there since it's > >> a pretty coarse way to do it. I would be happy to drop it if we can > >> feel comfortable that we got all of the callsites (now and in the > >> future) using your method. Perhaps we can just create a helper > >> function with this check builtin and then use it everywhere? That > >> might be enough to be confident about the future. > >> ___ > >> dev mailing list > >> [email protected] > >> http://openvswitch.org/mailman/listinfo/dev > > > ___ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 1/5] ofproto-dpif-xlate: Don't unwildcard tunnel attributes on set.
On 31/08/2016 11:32, "Jarno Rajahalme" wrote: >I’d put the registers and metadata field to the ‘false’ and also maybe >non-writeable fields (ether type, frags, nw_proto, etc.) in to >OVS_NOT_REACHED() case, just in case. > > Jarno Agreed, done Thanks, Daniele > >> On Aug 31, 2016, at 10:38 AM, Jesse Gross wrote: >> >> On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto >> wrote: >>> When translating a set action we also unwildcard the field in question. >>> This is done to correctly translate set actions with the value identical >>> to the ingress flow, like in the following example: >>> >>> flow table: >>> >>> tcp,actions=set_field:80->tcp_dst,output:5 >>> >>> ingress packet: >>> >>> ...,tcp,tcp_dst=80 >>> >>> datapath flow >>> >>> ...,tcp(dst=80) actions:5 >>> >>> The datapath flow must exact match the target field, because the actions >>> do not include a set field. (Otherwise a packet coming in with a >>> different tcp_dst would be matched, and its port wouldn't be altered). >>> >>> Tunnel attributes behave differently: at the datapath layer, before >>> action processing they're cleared (we do the same at the ofproto layer >>> in xlate_actions()). Therefore there's no need to unwildcard them, >>> because a set action would always be detected (since we zero them at the >>> beginning of xlate_ations()). >>> >>> This fixes a problem related to the handling of Geneve options. >>> Unwildcarding non existing Geneve options (as done by a >>> set_field:X->tun_metadata on a packet coming from a non-tunnel >>> interface) would be problematic for the datapaths: the ODP translation >>> functions cannot express a match on non existing Geneve options (unlike >>> on other attributes), and the userspace datapath wouldn't be able to >>> translate them to "userspace datapath format". In both cases the >>> installed flow would be deleted by revalidation at the first >>> opportunity. >>> >>> Signed-off-by: Daniele Di Proietto >> >> I think there might be some more obscure ways of triggering this >> problem that still exist. Basically anything that can use a register >> as a target is a potential issue. For example, stack_pop, bundle, and >> multipath all look like they have the same masking behavior. >> >> I do have a general solution in this patch (look at the bottom of >> xlate_actions() where it is adjusting the wildcards): >> http://openvswitch.org/pipermail/dev/2016-August/078484.html >> >> I didn't realize that it was addressing an existing issue though and >> that patch certainly isn't suitable for anything other than master. >> >> I'm also not really a big fan of the way I handled it there since it's >> a pretty coarse way to do it. I would be happy to drop it if we can >> feel comfortable that we got all of the callsites (now and in the >> future) using your method. Perhaps we can just create a helper >> function with this check builtin and then use it everywhere? That >> might be enough to be confident about the future. >> ___ >> dev mailing list >> [email protected] >> http://openvswitch.org/mailman/listinfo/dev > ___ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 1/5] ofproto-dpif-xlate: Don't unwildcard tunnel attributes on set.
On 31/08/2016 10:38, "Jesse Gross" wrote: >On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto > wrote: >> When translating a set action we also unwildcard the field in question. >> This is done to correctly translate set actions with the value identical >> to the ingress flow, like in the following example: >> >> flow table: >> >> tcp,actions=set_field:80->tcp_dst,output:5 >> >> ingress packet: >> >> ...,tcp,tcp_dst=80 >> >> datapath flow >> >> ...,tcp(dst=80) actions:5 >> >> The datapath flow must exact match the target field, because the actions >> do not include a set field. (Otherwise a packet coming in with a >> different tcp_dst would be matched, and its port wouldn't be altered). >> >> Tunnel attributes behave differently: at the datapath layer, before >> action processing they're cleared (we do the same at the ofproto layer >> in xlate_actions()). Therefore there's no need to unwildcard them, >> because a set action would always be detected (since we zero them at the >> beginning of xlate_ations()). >> >> This fixes a problem related to the handling of Geneve options. >> Unwildcarding non existing Geneve options (as done by a >> set_field:X->tun_metadata on a packet coming from a non-tunnel >> interface) would be problematic for the datapaths: the ODP translation >> functions cannot express a match on non existing Geneve options (unlike >> on other attributes), and the userspace datapath wouldn't be able to >> translate them to "userspace datapath format". In both cases the >> installed flow would be deleted by revalidation at the first >> opportunity. >> >> Signed-off-by: Daniele Di Proietto > >I think there might be some more obscure ways of triggering this >problem that still exist. Basically anything that can use a register >as a target is a potential issue. For example, stack_pop, bundle, and >multipath all look like they have the same masking behavior. You're right, thanks. I added two extra checks (bundle and multipath share part of the code). > >I do have a general solution in this patch (look at the bottom of >xlate_actions() where it is adjusting the wildcards): >http://openvswitch.org/pipermail/dev/2016-August/078484.html > >I didn't realize that it was addressing an existing issue though and >that patch certainly isn't suitable for anything other than master. Perhaps the commit message was too specific about Geneve options, because that's how I found this originally, but the issue applies also to other tunnel metadata (tunnel id, tunnel flags, ...) > >I'm also not really a big fan of the way I handled it there since it's >a pretty coarse way to do it. I would be happy to drop it if we can >feel comfortable that we got all of the callsites (now and in the >future) using your method. Perhaps we can just create a helper >function with this check builtin and then use it everywhere? That >might be enough to be confident about the future. I agree with you, I'd prefer to fix it on the set action, if possible. Thanks for you input! ___ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 1/5] ofproto-dpif-xlate: Don't unwildcard tunnel attributes on set.
I’d put the registers and metadata field to the ‘false’ and also maybe non-writeable fields (ether type, frags, nw_proto, etc.) in to OVS_NOT_REACHED() case, just in case. Jarno > On Aug 31, 2016, at 10:38 AM, Jesse Gross wrote: > > On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto > wrote: >> When translating a set action we also unwildcard the field in question. >> This is done to correctly translate set actions with the value identical >> to the ingress flow, like in the following example: >> >> flow table: >> >> tcp,actions=set_field:80->tcp_dst,output:5 >> >> ingress packet: >> >> ...,tcp,tcp_dst=80 >> >> datapath flow >> >> ...,tcp(dst=80) actions:5 >> >> The datapath flow must exact match the target field, because the actions >> do not include a set field. (Otherwise a packet coming in with a >> different tcp_dst would be matched, and its port wouldn't be altered). >> >> Tunnel attributes behave differently: at the datapath layer, before >> action processing they're cleared (we do the same at the ofproto layer >> in xlate_actions()). Therefore there's no need to unwildcard them, >> because a set action would always be detected (since we zero them at the >> beginning of xlate_ations()). >> >> This fixes a problem related to the handling of Geneve options. >> Unwildcarding non existing Geneve options (as done by a >> set_field:X->tun_metadata on a packet coming from a non-tunnel >> interface) would be problematic for the datapaths: the ODP translation >> functions cannot express a match on non existing Geneve options (unlike >> on other attributes), and the userspace datapath wouldn't be able to >> translate them to "userspace datapath format". In both cases the >> installed flow would be deleted by revalidation at the first >> opportunity. >> >> Signed-off-by: Daniele Di Proietto > > I think there might be some more obscure ways of triggering this > problem that still exist. Basically anything that can use a register > as a target is a potential issue. For example, stack_pop, bundle, and > multipath all look like they have the same masking behavior. > > I do have a general solution in this patch (look at the bottom of > xlate_actions() where it is adjusting the wildcards): > http://openvswitch.org/pipermail/dev/2016-August/078484.html > > I didn't realize that it was addressing an existing issue though and > that patch certainly isn't suitable for anything other than master. > > I'm also not really a big fan of the way I handled it there since it's > a pretty coarse way to do it. I would be happy to drop it if we can > feel comfortable that we got all of the callsites (now and in the > future) using your method. Perhaps we can just create a helper > function with this check builtin and then use it everywhere? That > might be enough to be confident about the future. > ___ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 1/5] ofproto-dpif-xlate: Don't unwildcard tunnel attributes on set.
On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto wrote: > When translating a set action we also unwildcard the field in question. > This is done to correctly translate set actions with the value identical > to the ingress flow, like in the following example: > > flow table: > > tcp,actions=set_field:80->tcp_dst,output:5 > > ingress packet: > > ...,tcp,tcp_dst=80 > > datapath flow > > ...,tcp(dst=80) actions:5 > > The datapath flow must exact match the target field, because the actions > do not include a set field. (Otherwise a packet coming in with a > different tcp_dst would be matched, and its port wouldn't be altered). > > Tunnel attributes behave differently: at the datapath layer, before > action processing they're cleared (we do the same at the ofproto layer > in xlate_actions()). Therefore there's no need to unwildcard them, > because a set action would always be detected (since we zero them at the > beginning of xlate_ations()). > > This fixes a problem related to the handling of Geneve options. > Unwildcarding non existing Geneve options (as done by a > set_field:X->tun_metadata on a packet coming from a non-tunnel > interface) would be problematic for the datapaths: the ODP translation > functions cannot express a match on non existing Geneve options (unlike > on other attributes), and the userspace datapath wouldn't be able to > translate them to "userspace datapath format". In both cases the > installed flow would be deleted by revalidation at the first > opportunity. > > Signed-off-by: Daniele Di Proietto I think there might be some more obscure ways of triggering this problem that still exist. Basically anything that can use a register as a target is a potential issue. For example, stack_pop, bundle, and multipath all look like they have the same masking behavior. I do have a general solution in this patch (look at the bottom of xlate_actions() where it is adjusting the wildcards): http://openvswitch.org/pipermail/dev/2016-August/078484.html I didn't realize that it was addressing an existing issue though and that patch certainly isn't suitable for anything other than master. I'm also not really a big fan of the way I handled it there since it's a pretty coarse way to do it. I would be happy to drop it if we can feel comfortable that we got all of the callsites (now and in the future) using your method. Perhaps we can just create a helper function with this check builtin and then use it everywhere? That might be enough to be confident about the future. ___ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
