lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 30 Sep 2013 10:10:39 +0900 From: Simon Horman <horms@...ge.net.au> To: Joe Stringer <joe@...d.net.nz> Cc: Ben Pfaff <blp@...ira.com>, "dev@...nvswitch.org" <dev@...nvswitch.org>, netdev@...r.kernel.org, Jesse Gross <jesse@...ira.com>, Pravin B Shelar <pshelar@...ira.com>, Ravi K <rkerur@...il.com>, Isaku Yamahata <yamahata@...inux.co.jp> Subject: Re: [PATCH v2.40 2/7] odp: Allow VLAN actions after MPLS actions On Sun, Sep 29, 2013 at 02:51:19PM +1300, Joe Stringer wrote: > On Sat, Sep 28, 2013 at 7:21 AM, Ben Pfaff <blp@...ira.com> wrote: > > > On Fri, Sep 27, 2013 at 09:18:31AM +0900, Simon Horman wrote: > > > From: Joe Stringer <joe@...d.net.nz> > > > > > > OpenFlow 1.2 and 1.3 differ on their handling of MPLS actions in the > > > presence of VLAN tags. To allow correct behaviour to be committed in > > > each situation, this patch adds a second round of VLAN tag action > > > handling to commit_odp_actions(), which occurs after MPLS actions. This > > > is implemented with a new field in 'struct xlate_in' called 'vlan_tci'. > > > > > > When an push_mpls action is composed, the flow's current VLAN state is > > > stored into xin->vlan_tci, and flow->vlan_tci is set to 0 (pop_vlan). If > > > a VLAN tag is present, it is stripped; if not, then there is no change. > > > Any later modifications to the VLAN state is written to xin->vlan_tci. > > > When committing the actions, flow->vlan_tci is used before MPLS actions, > > > and xin->vlan_tci is used afterwards. This retains the current datapath > > > behaviour, but allows VLAN actions to be applied in a more flexible > > > manner. > > > > > > Signed-off-by: Joe Stringer <joe@...d.net.nz> > > > Signed-off-by: Simon Horman <horms@...ge.net.au> > > > > The commit message talks about handling OpenFlow 1.2 and 1.3 both > > properly, but I don't see how the code distinguishes between the cases. > > Can you explain? Maybe this is something added in a later patch, in > > which case it would be nice to mention that in the commit message. > > > > It is added in patch #5. IIRC this patch should cause no difference in > behaviour, but set up infrastructure to be used later. > > There seems to be a typo in the comment in vlan_tci_restore() here: > > > + /* If MPLS actions were executed after MPLS, copy the final > > vlan_tci out > > > + * and restore the intermediate VLAN state. */ > > > > Right, that should probably be "...executed after VLAN actions...". > > I was a little confused by the new local variable 'vlan_tci' in > > do_xlate_actions(). Partly this was because the fact that I didn't find > > it obvious that sometimes it points to different VLAN tags. I think > > this would be easier to see if it were initially assigned just under the > > big comment rather than in an initializer, so that one would know to > > look at the comment. > > > > Perhaps the big comment could be rearranged and put above the initialiser, > something like the following:- > > /* VLAN actions are stored to '*vlan_tci'. This variable initially points > to 'xin->flow->vlan_tci', so that > * VLAN actions are applied before any MPLS actions. When an MPLS action is > translated, > * 'vlan_tci' is updated to point to 'xin->vlan_tci'. This causes later > VLAN actions to be applied after MPLS actions. > * Each time through the loop, 'xin->vlan_tci' is updated to reflect the > final VLAN state of the flow. */ > > Then, the place where 'xin->vlan_tci' is updated to '*vlan_tci' could have > a simple comment to refer back:- > > /* Update the final vlan state to match the current state. */ > > Simon, do you mind handling the change? Not at all. I'll include this change the next time I post the series. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists