Skip to content

bridge: allow IP addresses on members to be disabled #1641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

bridge: allow IP addresses on members to be disabled #1641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion share/man/man4/bridge.4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
.\" ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
.\" POSSIBILITY OF SUCH DAMAGE.
.\"
.Dd April 10, 2024
.Dd April 5, 2025
.Dt IF_BRIDGE 4
.Os
.Sh NAME
Expand Down Expand Up @@ -158,6 +158,19 @@ This can be used to multiplex the input of two or more interfaces into a single
stream.
This is useful for reconstructing the traffic for network taps
that transmit the RX/TX signals out through two separate interfaces.
.Pp
To allow the host to communicate with bridge members, IP addresses
should be assigned to the
.Nm
interface itself, not to the bridge's member interfaces.
Assigning IP addresses to bridge member interfaces is unsupported, but
for backward compatibility, it is permitted if the
.Xr sysctl 8
variable
.Va net.link.bridge.member_ifaddrs
is set to 1, which is the default.
In a future release, this sysctl may be set to 0 by default, or may be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tempted to say we should immediately announce this will be default in 16.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my preference is to make this the default in 15 and remove it in 16. since i posted this, i met another user on IRC who was running into a strange problem caused by assigning an IP address to a member. it just doesn't work properly and we should remove it rather than fixing it.

this might be a bit too aggressive though, so i'm okay with making it the default in 16 and removing it in 17.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to that plan either. Let's get this landed and start a thread on freebsd-net@ to discuss it.

removed entirely.
.Sh IPV6 SUPPORT
.Nm
supports the
Expand Down
61 changes: 52 additions & 9 deletions sys/net/if_bridge.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@

static void bridge_forward(struct bridge_softc *, struct bridge_iflist *,
struct mbuf *m);
static bool bridge_member_ifaddrs(void);

static void bridge_timer(void *);

Expand Down Expand Up @@ -502,6 +503,19 @@
CTLFLAG_RW | CTLFLAG_VNET, &VNET_NAME(log_mac_flap), true,
"Log MAC address port flapping");

/* allow IP addresses on bridge members */
VNET_DEFINE_STATIC(bool, member_ifaddrs) = true;
#define V_member_ifaddrs VNET(member_ifaddrs)
SYSCTL_BOOL(_net_link_bridge, OID_AUTO, member_ifaddrs,
CTLFLAG_RW | CTLFLAG_VNET, &VNET_NAME(member_ifaddrs), true,
"Allow layer 3 addresses on bridge members");

static bool
bridge_member_ifaddrs(void)
{
return (V_member_ifaddrs);
}

VNET_DEFINE_STATIC(int, log_interval) = 5;
VNET_DEFINE_STATIC(int, log_count) = 0;
VNET_DEFINE_STATIC(struct timeval, log_last) = { 0 };
Expand Down Expand Up @@ -665,6 +679,7 @@
bridge_dn_p = bridge_dummynet;
bridge_same_p = bridge_same;
bridge_get_softc_p = bridge_get_softc;
bridge_member_ifaddrs_p = bridge_member_ifaddrs;
bridge_detach_cookie = EVENTHANDLER_REGISTER(
ifnet_departure_event, bridge_ifdetach, NULL,
EVENTHANDLER_PRI_ANY);
Expand All @@ -675,6 +690,7 @@
bridge_dn_p = NULL;
bridge_same_p = NULL;
bridge_get_softc_p = NULL;
bridge_member_ifaddrs_p = NULL;
break;
default:
return (EOPNOTSUPP);
Expand Down Expand Up @@ -1313,6 +1329,25 @@
return (EINVAL);
}

/*
* If member_ifaddrs is disabled, do not allow an interface with
* assigned IP addresses to be added to a bridge.
*/
if (!V_member_ifaddrs) {
struct ifaddr *ifa;

CK_STAILQ_FOREACH(ifa, &ifs->if_addrhead, ifa_link) {
#ifdef INET
if (ifa->ifa_addr->sa_family == AF_INET)
return (EINVAL);
#endif
#ifdef INET6
if (ifa->ifa_addr->sa_family == AF_INET6)
return (EINVAL);
#endif
}
}

#ifdef INET6
/*
* Two valid inet6 addresses with link-local scope must not be
Expand Down Expand Up @@ -2739,20 +2774,28 @@
/*
* Unicast. Make sure it's not for the bridge.
*/
do { GRAB_OUR_PACKETS(bifp) } while (0);

Check failure on line 2777 in sys/net/if_bridge.c

View workflow job for this annotation

GitHub Actions / Style Checker 

suspicious ; after while (0)

/*
* Give a chance for ifp at first priority. This will help when the
* packet comes through the interface like VLAN's with the same MACs
* on several interfaces from the same bridge. This also will save
* some CPU cycles in case the destination interface and the input
* interface (eq ifp) are the same.
* We only need to check members interfaces if member_ifaddrs is
* enabled; otherwise we should have never traffic destined for a
* member's lladdr.
*/
do { GRAB_OUR_PACKETS(ifp) } while (0);

/* Now check the all bridge members. */
CK_LIST_FOREACH(bif2, &sc->sc_iflist, bif_next) {
GRAB_OUR_PACKETS(bif2->bif_ifp)
if (V_member_ifaddrs) {
/*
* Give a chance for ifp at first priority. This will help when
* the packet comes through the interface like VLAN's with the
* same MACs on several interfaces from the same bridge. This
* also will save some CPU cycles in case the destination
* interface and the input interface (eq ifp) are the same.
*/
do { GRAB_OUR_PACKETS(ifp) } while (0);

Check failure on line 2793 in sys/net/if_bridge.c

View workflow job for this annotation

GitHub Actions / Style Checker 

suspicious ; after while (0)

/* Now check the all bridge members. */
CK_LIST_FOREACH(bif2, &sc->sc_iflist, bif_next) {
GRAB_OUR_PACKETS(bif2->bif_ifp)
}
}

#undef CARP_CHECK_WE_ARE_DST
Expand Down
1 change: 1 addition & 0 deletions sys/net/if_bridgevar.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ struct ifbpstpconf {
extern void (*bridge_dn_p)(struct mbuf *, struct ifnet *);
extern bool (*bridge_same_p)(const void *, const void *);
extern void *(*bridge_get_softc_p)(struct ifnet *);
extern bool (*bridge_member_ifaddrs_p)(void);

#endif /* _KERNEL */

Expand Down
1 change: 1 addition & 0 deletions sys/net/if_ethersubr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ void (*vlan_input_p)(struct ifnet *, struct mbuf *);
void (*bridge_dn_p)(struct mbuf *, struct ifnet *);
bool (*bridge_same_p)(const void *, const void *);
void *(*bridge_get_softc_p)(struct ifnet *);
bool (*bridge_member_ifaddrs_p)(void);

/* if_lagg(4) support */
struct mbuf *(*lagg_input_ethernet_p)(struct ifnet *, struct mbuf *);
Expand Down
8 changes: 8 additions & 0 deletions sys/netinet/in.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
#include <net/if_llatbl.h>
#include <net/if_private.h>
#include <net/if_types.h>
#include <net/if_bridgevar.h>
#include <net/route.h>
#include <net/route/nhop.h>
#include <net/route/route_ctl.h>
Expand Down Expand Up @@ -518,6 +519,13 @@ in_aifaddr_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp, struct ucred *cred
return (error);
#endif

/*
* Check if bridge wants to allow adding addrs to member interfaces.
*/
if (ifp->if_bridge && bridge_member_ifaddrs_p &&
!bridge_member_ifaddrs_p())
return (EINVAL);

/*
* See whether address already exist.
*/
Expand Down
8 changes: 8 additions & 0 deletions sys/netinet6/in6.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
#include <net/if_var.h>
#include <net/if_private.h>
#include <net/if_types.h>
#include <net/if_bridgevar.h>
#include <net/route.h>
#include <net/route/route_ctl.h>
#include <net/route/nhop.h>
Expand Down Expand Up @@ -1234,6 +1235,13 @@ in6_addifaddr(struct ifnet *ifp, struct in6_aliasreq *ifra, struct in6_ifaddr *i
int carp_attached = 0;
int error;

/* Check if this interface is a bridge member */
if (ifp->if_bridge && bridge_member_ifaddrs_p &&
!bridge_member_ifaddrs_p()) {
error = EINVAL;
goto out;
}

/*
* first, make or update the interface address structure,
* and link it to the list.
Expand Down
78 changes: 78 additions & 0 deletions tests/sys/net/if_bridge_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,82 @@
vnet_cleanup
}

atf_test_case "member_ifaddrs_enabled" "cleanup"
member_ifaddrs_enabled_head()
{
atf_set descr 'bridge with member_ifaddrs=1'
atf_set require.user root
}

member_ifaddrs_enabled_body()
{
vnet_init
vnet_init_bridge

ep=$(vnet_mkepair)
ifconfig ${ep}a inet 192.0.2.1/24 up

vnet_mkjail one ${ep}b
jexec one sysctl net.link.bridge.member_ifaddrs=1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve of setting the sysctl explicitly, despise '1' being default, because now the test will pass even when we change the default.

jexec one ifconfig ${ep}b inet 192.0.2.2/24 up
jexec one ifconfig bridge0 create addm ${ep}b

atf_check -s exit:0 -o ignore ping -c3 -t1 192.0.2.2
}

member_ifaddrs_enabled_cleanup()
{
vnet_cleanup
}

atf_test_case "member_ifaddrs_disabled" "cleanup"
member_ifaddrs_disabled_head()
{
atf_set descr 'bridge with member_ifaddrs=0'
atf_set require.user root
}

member_ifaddrs_disabled_body()
{
vnet_init
vnet_init_bridge

vnet_mkjail one
jexec one sysctl net.link.bridge.member_ifaddrs=0

bridge=$(jexec one ifconfig bridge create)

# adding an interface with an IPv4 address
ep=$(jexec one ifconfig epair create)
jexec one ifconfig ${ep} 192.0.2.1/32
atf_check -s exit:1 -e ignore jexec one ifconfig ${bridge} addm ${ep}

# adding an interface with an IPv6 address
ep=$(jexec one ifconfig epair create)
jexec one ifconfig ${ep} inet6 2001:db8::1/128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with an interface that only has a link-local IPv6 address? I'd assume that also fails to be added, but it might be worth extending the test case to cover that scenario too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it fails to be added. this causes a bit of a negative UX experience in one case where you create an interface, add an IPv6 GUA, remove the GUA, and the LLA is still there so you can't put it in a bridge. i wonder if it's worth having ifconfig check for this to print a better warning.

i didn't want to have bridge(4) itself remove the addresses automatically because i think this will silently break existing setups.

either way i'll add a test for this though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with not doing so automatically. Getting ifconfig to print a useful warning (e.g. "Cannot add interfaces that have an IP(v6) address") would be nice too, but doesn't have to be done with this commit.

atf_check -s exit:1 -e ignore jexec one ifconfig ${bridge} addm ${ep}

# adding an interface with an IPv6 link-local address
ep=$(jexec one ifconfig epair create)
jexec one ifconfig ${ep} inet6 -ifdisabled auto_linklocal up
atf_check -s exit:1 -e ignore jexec one ifconfig ${bridge} addm ${ep}

# adding an IPv4 address to a member
ep=$(jexec one ifconfig epair create)
jexec one ifconfig ${bridge} addm ${ep}
atf_check -s exit:1 -e ignore jexec one ifconfig ${ep} inet 192.0.2.2/32

# adding an IPv6 address to a member
ep=$(jexec one ifconfig epair create)
jexec one ifconfig ${bridge} addm ${ep}
atf_check -s exit:1 -e ignore jexec one ifconfig ${ep} inet6 2001:db8::1/128

Check warning on line 774 in tests/sys/net/if_bridge_test.sh

View workflow job for this annotation

GitHub Actions / Style Checker 

line over 80 characters
}

member_ifaddrs_disabled_cleanup()
{
vnet_cleanup
}

atf_init_test_cases()
{
atf_add_test_case "bridge_transmit_ipv4_unicast"
Expand All @@ -718,4 +794,6 @@
atf_add_test_case "mtu"
atf_add_test_case "vlan"
atf_add_test_case "many_bridge_members"
atf_add_test_case "member_ifaddrs_enabled"
atf_add_test_case "member_ifaddrs_disabled"
}
Loading