| Message ID | 1355939304-21804-4-git-send-email-vyasevic@redhat.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 19 Dec 2012 at 17:48 GMT, Vlad Yasevich <vyasevic@redhat.com> wrote: > +static inline u16 br_get_vlan(const struct sk_buff *skb) > +{ > + u16 tag; > + > + if (vlan_tx_tag_present(skb)) > + return vlan_tx_tag_get(skb) & VLAN_VID_MASK; > + > + if (vlan_get_tag(skb, &tag)) > + return 0; > + > + return tag & VLAN_VID_MASK; > +} > + Nitpick: The name br_get_vlan() can easily confuse people with br_vlan_find(). Also, this function looks like not bridge-specific, how about moving it to if_vlan.h? -- 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
Hi Vlad, On Wed, 19 Dec 2012 12:48:14 -0500 Vlad Yasevich <vyasevic@redhat.com> wrote: > +static bool br_allowed_ingress(struct net_bridge_port *p, struct sk_buff *skb) > +{ > + struct net_port_vlan *pve; > + u16 vid; > + > + /* If there are no vlan in the permitted list, all packets are > + * permitted. > + */ > + if (list_empty(&p->vlan_list)) > + return true; I assumed the default policy would be Drop in such case, otherwise leaking between vlan domains is possible. Or maybe, ingress policy when port isn't a member of ingress VID should be configurable (drop/allow). > + vid = br_get_vlan(skb); > + pve = nbp_vlan_find(p, vid); Why search by iterating through NBP's vlan_list? You know the VID (hence may fetch the net_bridge_vlan from the hash), so why don't you directly consult the net_bridge_vlan's port_bitmap? > @@ -54,6 +74,9 @@ int br_handle_frame_finish(struct sk_buff *skb) > if (!p || p->state == BR_STATE_DISABLED) > goto drop; > > + if (!br_allowed_ingress(p, skb)) > + goto drop; > + This condition should be also encorporated upon "ingress" at the "bridge master port" (that is, early at br_dev_xmit). Think of the "bridge master port" as yet another port: upon "ingress" (meaning, tx packets from the ip stack), we should also enforce any ingress permission rules. Regards, Shmulik -- 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 12/20/2012 09:07 AM, Shmulik Ladkani wrote: > Hi Vlad, > > On Wed, 19 Dec 2012 12:48:14 -0500 Vlad Yasevich <vyasevic@redhat.com> wrote: >> +static bool br_allowed_ingress(struct net_bridge_port *p, struct sk_buff *skb) >> +{ >> + struct net_port_vlan *pve; >> + u16 vid; >> + >> + /* If there are no vlan in the permitted list, all packets are >> + * permitted. >> + */ >> + if (list_empty(&p->vlan_list)) >> + return true; > > I assumed the default policy would be Drop in such case, otherwise > leaking between vlan domains is possible. > Or maybe, ingress policy when port isn't a member of ingress VID should > be configurable (drop/allow). We have have to default to allow since we want to retain original bridge functionality if there is no configuration. > >> + vid = br_get_vlan(skb); >> + pve = nbp_vlan_find(p, vid); > > Why search by iterating through NBP's vlan_list? > You know the VID (hence may fetch the net_bridge_vlan from the hash), so > why don't you directly consult the net_bridge_vlan's port_bitmap? It's an alternative... I am betting that this port isn't in too many vlans and that searching the list might be faster. > >> @@ -54,6 +74,9 @@ int br_handle_frame_finish(struct sk_buff *skb) >> if (!p || p->state == BR_STATE_DISABLED) >> goto drop; >> >> + if (!br_allowed_ingress(p, skb)) >> + goto drop; >> + > > This condition should be also encorporated upon "ingress" at the "bridge > master port" (that is, early at br_dev_xmit). > Think of the "bridge master port" as yet another port: > upon "ingress" (meaning, tx packets from the ip stack), we should > also enforce any ingress permission rules. > I've tried that before and now can't think of a reason why I rejected it. I'll try to remember... Thanks -vlad > Regards, > Shmulik > -- 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 Thu, 20 Dec 2012 10:41:28 -0500 Vlad Yasevich <vyasevic@redhat.com> wrote: > >> +static bool br_allowed_ingress(struct net_bridge_port *p, struct sk_buff *skb) > >> +{ > >> + struct net_port_vlan *pve; > >> + u16 vid; > >> + > >> + /* If there are no vlan in the permitted list, all packets are > >> + * permitted. > >> + */ > >> + if (list_empty(&p->vlan_list)) > >> + return true; > > > > I assumed the default policy would be Drop in such case, otherwise > > leaking between vlan domains is possible. > > Or maybe, ingress policy when port isn't a member of ingress VID should > > be configurable (drop/allow). > > We have have to default to allow since we want to retain original bridge > functionality if there is no configuration. Ok; so having the port not a member of ANY vlan is a "port vlan disabled" configuration knob, and as such, it is a member of ANY vlan, meaning that: (1) every "non-vlan port" is connected to any other "non-vlan port" (2) frame ingress on a "non-vlan" port may egress on a "vlan enabled" port, depending on the ingress VID and the port-membership map of the egress port (and thus, PVID should be defined even to "non-vlan" ports, for the case where untagged frame is received on the non-vlan port) (3) frame ingress on a "vlan-enabled" port would always egress on "non-vlan" ports Seems ok. However this is an additional nuance that might not be expected by the user configuring the bridge; maybe this needs some clarification. > >> + vid = br_get_vlan(skb); > >> + pve = nbp_vlan_find(p, vid); > > > > Why search by iterating through NBP's vlan_list? > > You know the VID (hence may fetch the net_bridge_vlan from the hash), so > > why don't you directly consult the net_bridge_vlan's port_bitmap? > > It's an alternative... I am betting that this port isn't in too many > vlans and that searching the list might be faster. I assumed the opposite: finding the hash bucket is just a bitwise mask, and number of items in a bucket would rarely be grater than 1. I expect such code to be shorter, but this needs to be verified. Regards, Shmulik -- 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 12/20/2012 11:24 AM, Shmulik Ladkani wrote: > On Thu, 20 Dec 2012 10:41:28 -0500 Vlad Yasevich <vyasevic@redhat.com> wrote: >>>> +static bool br_allowed_ingress(struct net_bridge_port *p, struct sk_buff *skb) >>>> +{ >>>> + struct net_port_vlan *pve; >>>> + u16 vid; >>>> + >>>> + /* If there are no vlan in the permitted list, all packets are >>>> + * permitted. >>>> + */ >>>> + if (list_empty(&p->vlan_list)) >>>> + return true; >>> >>> I assumed the default policy would be Drop in such case, otherwise >>> leaking between vlan domains is possible. >>> Or maybe, ingress policy when port isn't a member of ingress VID should >>> be configurable (drop/allow). >> >> We have have to default to allow since we want to retain original bridge >> functionality if there is no configuration. > > Ok; so having the port not a member of ANY vlan is a "port vlan > disabled" configuration knob, and as such, it is a member of ANY vlan, > meaning that: > > (1) every "non-vlan port" is connected to any other "non-vlan port" Technically, it would be connected to every over port. > (2) frame ingress on a "non-vlan" port may egress on a "vlan enabled" > port, depending on the ingress VID and the port-membership map of the > egress port > (and thus, PVID should be defined even to "non-vlan" ports, for the > case where untagged frame is received on the non-vlan port) Sort of. The way I did it (testing now), is like this: if there is egress policy apply policy and forward. else if there was ingress policy (pvid) apply it and forward else forward as is (old bridge behavior). This way if there was a pvid on an ingress port and nothing on egress, then pvid applies. If there was nothing configured on ingress port, but we have a egress policy, we'll apply any vlan information from the frame to egress policy. In this case, ingress untagged traffic would be assigned vlan 0. > (3) frame ingress on a "vlan-enabled" port would always egress on > "non-vlan" ports yes and they would egress based on their ingress policy. > > Seems ok. > However this is an additional nuance that might not be expected by the > user configuring the bridge; maybe this needs some clarification. I'll try to document things sufficiently. This hybrid approach may produce some unintended results. We could always remove it or introduce the tunable to change default policy to drop once vlan configuration is in effect. > >>>> + vid = br_get_vlan(skb); >>>> + pve = nbp_vlan_find(p, vid); >>> >>> Why search by iterating through NBP's vlan_list? >>> You know the VID (hence may fetch the net_bridge_vlan from the hash), so >>> why don't you directly consult the net_bridge_vlan's port_bitmap? >> >> It's an alternative... I am betting that this port isn't in too many >> vlans and that searching the list might be faster. > > I assumed the opposite: finding the hash bucket is just a bitwise mask, > and number of items in a bucket would rarely be grater than 1. > I expect such code to be shorter, but this needs to be verified. I'll try to set something up, but that will probably be next year... -vlad > > Regards, > Shmulik > -- 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
Hi Vlad, On Thu, 20 Dec 2012 11:54:15 -0500 Vlad Yasevich <vyasevic@redhat.com> wrote: > > (2) frame ingress on a "non-vlan" port may egress on a "vlan enabled" > > port, depending on the ingress VID and the port-membership map of the > > egress port > > (and thus, PVID should be defined even to "non-vlan" ports, for the > > case where untagged frame is received on the non-vlan port) > > Sort of. The way I did it (testing now), is like this: > if there is egress policy > apply policy and forward. > else if there was ingress policy (pvid) > apply it and forward > else > forward as is (old bridge behavior). > > This way if there was a pvid on an ingress port and nothing on egress, > then pvid applies. If there was nothing configured on ingress port, > but we have a egress policy, we'll apply any vlan information from > the frame to egress policy. In this case, ingress untagged traffic > would be assigned vlan 0. Sorry, got too cryptic too me ;) We're probably aligned, but if you don't mind I'd like to make sure I got it right. I'd expect the following logic if the bridge is a vlan bridge: 1. Frame ingress on a port Frame's VID is collected: if frame was tagged, its the VID found in the tag; if frame was untagged (or prio-tagged), the VID would be port's PVID. 2. Ingress membership verification Verify the ingress port is a member of the frame's VID vlan (collected on step 1). (Usually policy is 'drop' in case port is not a member). Easily calculated by testing if the port bit is set in vlan's port membership map. 3. Switching logic Consult FDB for a forward/flood/drop decision, resulting in a map of candidate ports the frame might egress upon (e.g. in the common case, a valid existing unicast entry, the result is just one candidate port). 4. Egress membership verification For each candidate port found on step 3, verify it is a member of the frame's VID vlan. (Usually, candidate ports that aren't members of the vlan will not be selected for actual egress). This can be easily calculated by masking the candidates port map (found on step 3) with the vlan's port membership map. The resulting masked map is final egress portmap. 5. Frame tag construction and egress For each final egress port (found on step 4), verify its tagged/untagged policy in the vlan's egress-policy map. Properly add/remove the vlan tag (if needed) according to port's egress policy, and transmit. To my best understanding, if all the ports are "vlan-enabled" (having a non-empty vid list, i.e. belonging to at least one vlan), the behavior of the implemented bridge is aligned with the above scheme. For "non-vlan" ports (having en empty vid list), we treat them as if they belong to ANY POSSIBLE vlan (as if their bit is always set in every vlan port membeship map). Meaning, in step (2) verification always suceeds for such ports, and in step (4) such ports will never be masked out of the egress candidates portmap. Please let me know if the implementation fits this. > I'll try to document things sufficiently. This hybrid approach may > produce some unintended results. We could always remove it or introduce > the tunable to change default policy to drop once vlan configuration is > in effect. Ok. Regards, Shmulik -- 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 Thu, 20 Dec 2012 10:41:28 -0500 Vlad Yasevich <vyasevic@redhat.com> wrote: > >> +static bool br_allowed_ingress(struct net_bridge_port *p, struct sk_buff *skb) > >> +{ > >> + struct net_port_vlan *pve; > >> + u16 vid; > >> + > >> + /* If there are no vlan in the permitted list, all packets are > >> + * permitted. > >> + */ > >> + if (list_empty(&p->vlan_list)) > >> + return true; > >> + if (!br_allowed_ingress(p, skb)) > >> + goto drop; > >> + > > > > This condition should be also encorporated upon "ingress" at the "bridge > > master port" (that is, early at br_dev_xmit). > > Think of the "bridge master port" as yet another port: > > upon "ingress" (meaning, tx packets from the ip stack), we should > > also enforce any ingress permission rules. > > > > I've tried that before and now can't think of a reason why I rejected > it. I'll try to remember... (Current code does not allow you to put the test into 'br_dev_xmit', since br_allow_ingress expects an 'nbp' as its 1st argument, and the "master port" is not represented as an 'nbp') But this is a must. The "master port" shouldn't be treated differently with respect to vlan ingress/egress membership verifications. Take for example why egress membership verification is important: m | +----+----+ | | p1 p2 p1 expects "normal" (untagged) traffic, and should be connected with the IP stack (master port 'm'), but not leak to p2. Traffic of some specific service, marked with VLAN 2, arrives at p2 and should be forwardable with p1 (keeping the tag), but should not go to the IP stack. p1 pvid: 1 m pvid: 1 vlan 1 membership map: p1 m vlan 1 egress policy map: U U vlan 2 membership map: p1 p2 vlan 2 egress policy map: T T If you don't verify membership upon egress towards IP stack (at the "master port"), VLAN 2 packets from p2 will leak into the stack. Regards, Shmulik -- 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/bridge/br_input.c b/net/bridge/br_input.c index 4b34207..54c0894 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -17,6 +17,7 @@ #include <linux/etherdevice.h> #include <linux/netfilter_bridge.h> #include <linux/export.h> +#include <linux/rculist.h> #include "br_private.h" /* Hook for brouter */ @@ -41,6 +42,25 @@ static int br_pass_frame_up(struct sk_buff *skb) netif_receive_skb); } +static bool br_allowed_ingress(struct net_bridge_port *p, struct sk_buff *skb) +{ + struct net_port_vlan *pve; + u16 vid; + + /* If there are no vlan in the permitted list, all packets are + * permitted. + */ + if (list_empty(&p->vlan_list)) + return true; + + vid = br_get_vlan(skb); + pve = nbp_vlan_find(p, vid); + if (pve) + return true; + + return false; +} + /* note: already called with rcu_read_lock */ int br_handle_frame_finish(struct sk_buff *skb) { @@ -54,6 +74,9 @@ int br_handle_frame_finish(struct sk_buff *skb) if (!p || p->state == BR_STATE_DISABLED) goto drop; + if (!br_allowed_ingress(p, skb)) + goto drop; + /* insert into forwarding database after filtering to avoid spoofing */ br = p->br; br_fdb_update(br, p, eth_hdr(skb)->h_source); diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 76d9fbc..1ba76b4 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -66,8 +66,6 @@ struct br_ip }; #define BR_INVALID_VID (1<<15) -#define BR_UNTAGGED_VID (1<<14) - #define BR_VID_HASH_SIZE (1<<6) #define br_vlan_hash(vid) ((vid) % (BR_VID_HASH_SIZE - 1)) @@ -197,6 +195,19 @@ static inline struct net_bridge_port *br_port_get_rtnl(struct net_device *dev) rtnl_dereference(dev->rx_handler_data) : NULL; } +static inline u16 br_get_vlan(const struct sk_buff *skb) +{ + u16 tag; + + if (vlan_tx_tag_present(skb)) + return vlan_tx_tag_get(skb) & VLAN_VID_MASK; + + if (vlan_get_tag(skb, &tag)) + return 0; + + return tag & VLAN_VID_MASK; +} + struct br_cpu_netstats { u64 rx_packets; u64 rx_bytes;
When a frame arrives on a port, if we have VLANs configured, validate that a given VLAN is allowed to ingress on a given port. Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> --- net/bridge/br_input.c | 23 +++++++++++++++++++++++ net/bridge/br_private.h | 15 +++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-)