| Message ID | 20220207100742.15087-2-schultz.hans+netdev@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for locked bridge ports (for 802.1X) | expand |
On Mon, Feb 07, 2022 at 11:07:39AM +0100, Hans Schultz wrote: > In a 802.1X scenario, clients connected to a bridge port shall not > be allowed to have traffic forwarded until fully authenticated. > A static fdb entry of the clients MAC address for the bridge port > unlocks the client and allows bidirectional communication. > > This scenario is facilitated with setting the bridge port in locked > mode, which is also supported by various switchcore chipsets. > > Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com> > --- > include/linux/if_bridge.h | 1 + > include/uapi/linux/if_link.h | 1 + > net/bridge/br_input.c | 10 +++++++++- > net/bridge/br_netlink.c | 6 +++++- > 4 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h > index 509e18c7e740..3aae023a9353 100644 > --- a/include/linux/if_bridge.h > +++ b/include/linux/if_bridge.h > @@ -58,6 +58,7 @@ struct br_ip_list { > #define BR_MRP_LOST_CONT BIT(18) > #define BR_MRP_LOST_IN_CONT BIT(19) > #define BR_TX_FWD_OFFLOAD BIT(20) > +#define BR_PORT_LOCKED BIT(21) > > #define BR_DEFAULT_AGEING_TIME (300 * HZ) > > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h > index 6218f93f5c1a..8fa2648fbc83 100644 > --- a/include/uapi/linux/if_link.h > +++ b/include/uapi/linux/if_link.h > @@ -532,6 +532,7 @@ enum { > IFLA_BRPORT_GROUP_FWD_MASK, > IFLA_BRPORT_NEIGH_SUPPRESS, > IFLA_BRPORT_ISOLATED, > + IFLA_BRPORT_LOCKED, Please add it at the end to avoid breaking uAPI > IFLA_BRPORT_BACKUP_PORT, > IFLA_BRPORT_MRP_RING_OPEN, > IFLA_BRPORT_MRP_IN_OPEN, > diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c > index b50382f957c1..469e3adbce07 100644 > --- a/net/bridge/br_input.c > +++ b/net/bridge/br_input.c > @@ -69,6 +69,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb > struct net_bridge_port *p = br_port_get_rcu(skb->dev); > enum br_pkt_type pkt_type = BR_PKT_UNICAST; > struct net_bridge_fdb_entry *dst = NULL; > + struct net_bridge_fdb_entry *fdb_entry; > struct net_bridge_mcast_port *pmctx; > struct net_bridge_mdb_entry *mdst; > bool local_rcv, mcast_hit = false; > @@ -81,6 +82,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb > if (!p || p->state == BR_STATE_DISABLED) > goto drop; > > + br = p->br; > + > brmctx = &p->br->multicast_ctx; > pmctx = &p->multicast_ctx; > state = p->state; > @@ -88,10 +91,15 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb > &state, &vlan)) > goto out; > > + if (p->flags & BR_PORT_LOCKED) { > + fdb_entry = br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid); > + if (!(fdb_entry && fdb_entry->dst == p)) > + goto drop; I'm not familiar with 802.1X so I have some questions: 1. Do we need to differentiate between no FDB entry and an FDB entry pointing to a different port than we expect? 2. Does user space care about SAs that did not pass the check? That is, does it need to see notifications? Counters? > + } > + > nbp_switchdev_frame_mark(p, skb); > > /* insert into forwarding database after filtering to avoid spoofing */ > - br = p->br; > if (p->flags & BR_LEARNING) > br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, 0); > > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > index 2ff83d84230d..7d4432ca9a20 100644 > --- a/net/bridge/br_netlink.c > +++ b/net/bridge/br_netlink.c > @@ -184,6 +184,7 @@ static inline size_t br_port_info_size(void) > + nla_total_size(1) /* IFLA_BRPORT_VLAN_TUNNEL */ > + nla_total_size(1) /* IFLA_BRPORT_NEIGH_SUPPRESS */ > + nla_total_size(1) /* IFLA_BRPORT_ISOLATED */ > + + nla_total_size(1) /* IFLA_BRPORT_LOCKED */ > + nla_total_size(sizeof(struct ifla_bridge_id)) /* IFLA_BRPORT_ROOT_ID */ > + nla_total_size(sizeof(struct ifla_bridge_id)) /* IFLA_BRPORT_BRIDGE_ID */ > + nla_total_size(sizeof(u16)) /* IFLA_BRPORT_DESIGNATED_PORT */ > @@ -269,7 +270,8 @@ static int br_port_fill_attrs(struct sk_buff *skb, > BR_MRP_LOST_CONT)) || > nla_put_u8(skb, IFLA_BRPORT_MRP_IN_OPEN, > !!(p->flags & BR_MRP_LOST_IN_CONT)) || > - nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED))) > + nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED)) || > + nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & BR_PORT_LOCKED))) > return -EMSGSIZE; > > timerval = br_timer_value(&p->message_age_timer); > @@ -827,6 +829,7 @@ static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = { > [IFLA_BRPORT_GROUP_FWD_MASK] = { .type = NLA_U16 }, > [IFLA_BRPORT_NEIGH_SUPPRESS] = { .type = NLA_U8 }, > [IFLA_BRPORT_ISOLATED] = { .type = NLA_U8 }, > + [IFLA_BRPORT_LOCKED] = { .type = NLA_U8 }, > [IFLA_BRPORT_BACKUP_PORT] = { .type = NLA_U32 }, > [IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT] = { .type = NLA_U32 }, > }; > @@ -893,6 +896,7 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[], > br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL); > br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS); > br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED); > + br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED); > > changed_mask = old_flags ^ p->flags; > > -- > 2.30.2 >
> > + if (p->flags & BR_PORT_LOCKED) { > > + fdb_entry = br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid); > > + if (!(fdb_entry && fdb_entry->dst == p)) > > + goto drop; > > I'm not familiar with 802.1X so I have some questions: Me neither. > > 1. Do we need to differentiate between no FDB entry and an FDB entry > pointing to a different port than we expect? And extending that question, a static vs a dynamic entry? Andrew
On Mon, 7 Feb 2022 11:07:39 +0100 Hans Schultz <schultz.hans@gmail.com> wrote: > --- a/include/uapi/linux/if_link.h > +++ b/include/uapi/linux/if_link.h > @@ -532,6 +532,7 @@ enum { > IFLA_BRPORT_GROUP_FWD_MASK, > IFLA_BRPORT_NEIGH_SUPPRESS, > IFLA_BRPORT_ISOLATED, > + IFLA_BRPORT_LOCKED, > IFLA_BRPORT_BACKUP_PORT, > IFLA_BRPORT_MRP_RING_OPEN, Adding new element in middle of enum causes them to be renumbered. That breaks the kernel ABI. Please add only at end.
On mån, feb 07, 2022 at 14:53, Andrew Lunn <andrew@lunn.ch> wrote: >> > + if (p->flags & BR_PORT_LOCKED) { >> > + fdb_entry = br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid); >> > + if (!(fdb_entry && fdb_entry->dst == p)) >> > + goto drop; >> >> I'm not familiar with 802.1X so I have some questions: > > Me neither. > >> >> 1. Do we need to differentiate between no FDB entry and an FDB entry >> pointing to a different port than we expect? > > And extending that question, a static vs a dynamic entry? > > Andrew The question is - if there is an fdb entry or not - for the specific client mac address behind the locked port in the bridge associated with the respective locked port and vlan taken into consideration. Normally you would have learning disabled, or from a fresh start if a port is locked, it will not learn on incoming from that port, so you need to add the fdb entry from user-space. In the common case you will want to use static entries and remember the master flag for the entry to go to the bridge module.
On mån, feb 07, 2022 at 12:49, Ido Schimmel <idosch@idosch.org> wrote: > On Mon, Feb 07, 2022 at 11:07:39AM +0100, Hans Schultz wrote: >> In a 802.1X scenario, clients connected to a bridge port shall not >> be allowed to have traffic forwarded until fully authenticated. >> A static fdb entry of the clients MAC address for the bridge port >> unlocks the client and allows bidirectional communication. >> >> This scenario is facilitated with setting the bridge port in locked >> mode, which is also supported by various switchcore chipsets. >> >> Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com> >> --- >> include/linux/if_bridge.h | 1 + >> include/uapi/linux/if_link.h | 1 + >> net/bridge/br_input.c | 10 +++++++++- >> net/bridge/br_netlink.c | 6 +++++- >> 4 files changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h >> index 509e18c7e740..3aae023a9353 100644 >> --- a/include/linux/if_bridge.h >> +++ b/include/linux/if_bridge.h >> @@ -58,6 +58,7 @@ struct br_ip_list { >> #define BR_MRP_LOST_CONT BIT(18) >> #define BR_MRP_LOST_IN_CONT BIT(19) >> #define BR_TX_FWD_OFFLOAD BIT(20) >> +#define BR_PORT_LOCKED BIT(21) >> >> #define BR_DEFAULT_AGEING_TIME (300 * HZ) >> >> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h >> index 6218f93f5c1a..8fa2648fbc83 100644 >> --- a/include/uapi/linux/if_link.h >> +++ b/include/uapi/linux/if_link.h >> @@ -532,6 +532,7 @@ enum { >> IFLA_BRPORT_GROUP_FWD_MASK, >> IFLA_BRPORT_NEIGH_SUPPRESS, >> IFLA_BRPORT_ISOLATED, >> + IFLA_BRPORT_LOCKED, > > Please add it at the end to avoid breaking uAPI > Shall do. >> IFLA_BRPORT_BACKUP_PORT, >> IFLA_BRPORT_MRP_RING_OPEN, >> IFLA_BRPORT_MRP_IN_OPEN, >> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c >> index b50382f957c1..469e3adbce07 100644 >> --- a/net/bridge/br_input.c >> +++ b/net/bridge/br_input.c >> @@ -69,6 +69,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >> struct net_bridge_port *p = br_port_get_rcu(skb->dev); >> enum br_pkt_type pkt_type = BR_PKT_UNICAST; >> struct net_bridge_fdb_entry *dst = NULL; >> + struct net_bridge_fdb_entry *fdb_entry; >> struct net_bridge_mcast_port *pmctx; >> struct net_bridge_mdb_entry *mdst; >> bool local_rcv, mcast_hit = false; >> @@ -81,6 +82,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >> if (!p || p->state == BR_STATE_DISABLED) >> goto drop; >> >> + br = p->br; >> + >> brmctx = &p->br->multicast_ctx; >> pmctx = &p->multicast_ctx; >> state = p->state; >> @@ -88,10 +91,15 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb >> &state, &vlan)) >> goto out; >> >> + if (p->flags & BR_PORT_LOCKED) { >> + fdb_entry = br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid); >> + if (!(fdb_entry && fdb_entry->dst == p)) >> + goto drop; > > I'm not familiar with 802.1X so I have some questions: > > 1. Do we need to differentiate between no FDB entry and an FDB entry > pointing to a different port than we expect? > > 2. Does user space care about SAs that did not pass the check? That is, > does it need to see notifications? Counters? > 2. As of now there are no counters, notifications on a locked port. >> + } >> + >> nbp_switchdev_frame_mark(p, skb); >> >> /* insert into forwarding database after filtering to avoid spoofing */ >> - br = p->br; >> if (p->flags & BR_LEARNING) >> br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, 0); >> >> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c >> index 2ff83d84230d..7d4432ca9a20 100644 >> --- a/net/bridge/br_netlink.c >> +++ b/net/bridge/br_netlink.c >> @@ -184,6 +184,7 @@ static inline size_t br_port_info_size(void) >> + nla_total_size(1) /* IFLA_BRPORT_VLAN_TUNNEL */ >> + nla_total_size(1) /* IFLA_BRPORT_NEIGH_SUPPRESS */ >> + nla_total_size(1) /* IFLA_BRPORT_ISOLATED */ >> + + nla_total_size(1) /* IFLA_BRPORT_LOCKED */ >> + nla_total_size(sizeof(struct ifla_bridge_id)) /* IFLA_BRPORT_ROOT_ID */ >> + nla_total_size(sizeof(struct ifla_bridge_id)) /* IFLA_BRPORT_BRIDGE_ID */ >> + nla_total_size(sizeof(u16)) /* IFLA_BRPORT_DESIGNATED_PORT */ >> @@ -269,7 +270,8 @@ static int br_port_fill_attrs(struct sk_buff *skb, >> BR_MRP_LOST_CONT)) || >> nla_put_u8(skb, IFLA_BRPORT_MRP_IN_OPEN, >> !!(p->flags & BR_MRP_LOST_IN_CONT)) || >> - nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED))) >> + nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED)) || >> + nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & BR_PORT_LOCKED))) >> return -EMSGSIZE; >> >> timerval = br_timer_value(&p->message_age_timer); >> @@ -827,6 +829,7 @@ static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = { >> [IFLA_BRPORT_GROUP_FWD_MASK] = { .type = NLA_U16 }, >> [IFLA_BRPORT_NEIGH_SUPPRESS] = { .type = NLA_U8 }, >> [IFLA_BRPORT_ISOLATED] = { .type = NLA_U8 }, >> + [IFLA_BRPORT_LOCKED] = { .type = NLA_U8 }, >> [IFLA_BRPORT_BACKUP_PORT] = { .type = NLA_U32 }, >> [IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT] = { .type = NLA_U32 }, >> }; >> @@ -893,6 +896,7 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[], >> br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL); >> br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS); >> br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED); >> + br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED); >> >> changed_mask = old_flags ^ p->flags; >> >> -- >> 2.30.2 >>
On Mon, 7 Feb 2022 11:07:39 +0100 Hans Schultz <schultz.hans@gmail.com> wrote: > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h > index 6218f93f5c1a..8fa2648fbc83 100644 > --- a/include/uapi/linux/if_link.h > +++ b/include/uapi/linux/if_link.h > @@ -532,6 +532,7 @@ enum { > IFLA_BRPORT_GROUP_FWD_MASK, > IFLA_BRPORT_NEIGH_SUPPRESS, > IFLA_BRPORT_ISOLATED, > + IFLA_BRPORT_LOCKED, > IFLA_BRPORT_BACKUP_PORT, > IFLA_BRPORT_MRP_RING_OPEN, > IFLA_BRPORT_MRP_IN_OPEN, NAK This is userspace API, adding a new value in enum in the middle will reorder the numbers and break ABI.
diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h index 509e18c7e740..3aae023a9353 100644 --- a/include/linux/if_bridge.h +++ b/include/linux/if_bridge.h @@ -58,6 +58,7 @@ struct br_ip_list { #define BR_MRP_LOST_CONT BIT(18) #define BR_MRP_LOST_IN_CONT BIT(19) #define BR_TX_FWD_OFFLOAD BIT(20) +#define BR_PORT_LOCKED BIT(21) #define BR_DEFAULT_AGEING_TIME (300 * HZ) diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index 6218f93f5c1a..8fa2648fbc83 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -532,6 +532,7 @@ enum { IFLA_BRPORT_GROUP_FWD_MASK, IFLA_BRPORT_NEIGH_SUPPRESS, IFLA_BRPORT_ISOLATED, + IFLA_BRPORT_LOCKED, IFLA_BRPORT_BACKUP_PORT, IFLA_BRPORT_MRP_RING_OPEN, IFLA_BRPORT_MRP_IN_OPEN, diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c index b50382f957c1..469e3adbce07 100644 --- a/net/bridge/br_input.c +++ b/net/bridge/br_input.c @@ -69,6 +69,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb struct net_bridge_port *p = br_port_get_rcu(skb->dev); enum br_pkt_type pkt_type = BR_PKT_UNICAST; struct net_bridge_fdb_entry *dst = NULL; + struct net_bridge_fdb_entry *fdb_entry; struct net_bridge_mcast_port *pmctx; struct net_bridge_mdb_entry *mdst; bool local_rcv, mcast_hit = false; @@ -81,6 +82,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb if (!p || p->state == BR_STATE_DISABLED) goto drop; + br = p->br; + brmctx = &p->br->multicast_ctx; pmctx = &p->multicast_ctx; state = p->state; @@ -88,10 +91,15 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb &state, &vlan)) goto out; + if (p->flags & BR_PORT_LOCKED) { + fdb_entry = br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid); + if (!(fdb_entry && fdb_entry->dst == p)) + goto drop; + } + nbp_switchdev_frame_mark(p, skb); /* insert into forwarding database after filtering to avoid spoofing */ - br = p->br; if (p->flags & BR_LEARNING) br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, 0); diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 2ff83d84230d..7d4432ca9a20 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -184,6 +184,7 @@ static inline size_t br_port_info_size(void) + nla_total_size(1) /* IFLA_BRPORT_VLAN_TUNNEL */ + nla_total_size(1) /* IFLA_BRPORT_NEIGH_SUPPRESS */ + nla_total_size(1) /* IFLA_BRPORT_ISOLATED */ + + nla_total_size(1) /* IFLA_BRPORT_LOCKED */ + nla_total_size(sizeof(struct ifla_bridge_id)) /* IFLA_BRPORT_ROOT_ID */ + nla_total_size(sizeof(struct ifla_bridge_id)) /* IFLA_BRPORT_BRIDGE_ID */ + nla_total_size(sizeof(u16)) /* IFLA_BRPORT_DESIGNATED_PORT */ @@ -269,7 +270,8 @@ static int br_port_fill_attrs(struct sk_buff *skb, BR_MRP_LOST_CONT)) || nla_put_u8(skb, IFLA_BRPORT_MRP_IN_OPEN, !!(p->flags & BR_MRP_LOST_IN_CONT)) || - nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED))) + nla_put_u8(skb, IFLA_BRPORT_ISOLATED, !!(p->flags & BR_ISOLATED)) || + nla_put_u8(skb, IFLA_BRPORT_LOCKED, !!(p->flags & BR_PORT_LOCKED))) return -EMSGSIZE; timerval = br_timer_value(&p->message_age_timer); @@ -827,6 +829,7 @@ static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = { [IFLA_BRPORT_GROUP_FWD_MASK] = { .type = NLA_U16 }, [IFLA_BRPORT_NEIGH_SUPPRESS] = { .type = NLA_U8 }, [IFLA_BRPORT_ISOLATED] = { .type = NLA_U8 }, + [IFLA_BRPORT_LOCKED] = { .type = NLA_U8 }, [IFLA_BRPORT_BACKUP_PORT] = { .type = NLA_U32 }, [IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT] = { .type = NLA_U32 }, }; @@ -893,6 +896,7 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[], br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL); br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS); br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED); + br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED); changed_mask = old_flags ^ p->flags;
In a 802.1X scenario, clients connected to a bridge port shall not be allowed to have traffic forwarded until fully authenticated. A static fdb entry of the clients MAC address for the bridge port unlocks the client and allows bidirectional communication. This scenario is facilitated with setting the bridge port in locked mode, which is also supported by various switchcore chipsets. Signed-off-by: Hans Schultz <schultz.hans+netdev@gmail.com> --- include/linux/if_bridge.h | 1 + include/uapi/linux/if_link.h | 1 + net/bridge/br_input.c | 10 +++++++++- net/bridge/br_netlink.c | 6 +++++- 4 files changed, 16 insertions(+), 2 deletions(-)