Skip to content
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

Jump to bottom

update sample scripts to use iproute #165

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

update sample scripts to use iproute #165

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
effe87b
update sample scripts to use iproute
kaechele Apr 10, 2022
ccff44d
ensure bridge port isolation is on before setting link up
kaechele Aug 8, 2023
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
4 changes: 1 addition & 3 deletions broker/scripts/bridge_functions.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ ensure_policy()
ensure_bridge()
{
local brname="$1"
brctl addbr $brname 2>/dev/null
ip link add $brname type bridge 2>/dev/null

if [[ "$?" == "0" ]]; then
# Bridge did not exist before, we have to initialize it
Expand All @@ -16,8 +16,6 @@ ensure_bridge()
ip addr add 10.254.0.2/16 dev $brname
# TODO Policy routing should probably not be hardcoded here?
ensure_policy from all iif $brname lookup mesh prio 1000
# Disable forwarding between bridge ports
ebtables -A FORWARD --logical-in $brname -j DROP
Copy link
Member

Choose a reason for hiding this comment

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

Why didn't you put the isolation on here? That would avoid having to audit all places where ensure_bridge is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

You have to do the isolation per port, and isolating the bridge interface itself would mean that none of the attached bridge porst would be able to communicate with host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pmelange is correct. isolation on is a function of the bridge port, not the bridge itself. As such it would functionally belong into the session handler scripts.

fi
}

2 changes: 1 addition & 1 deletion broker/scripts/session.down.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ UUID="$8"
LOCAL_BROKER_PORT="$9"

# Remove the interface from our bridge
brctl delif digger${MTU} $INTERFACE
ip link set dev $INTERFACE nomaster

10 changes: 3 additions & 7 deletions broker/scripts/session.mtu-changed.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,8 @@ NEW_MTU="$5"
. scripts/bridge_functions.sh

# Remove interface from old bridge
brctl delif digger${OLD_MTU} $INTERFACE
ip link set dev $INTERFACE nomaster

# Change interface MTU
ip link set dev $INTERFACE mtu $NEW_MTU

# Add interface to new bridge
# Change interface MTU and add to new bridge
ensure_bridge digger${NEW_MTU}
brctl addif digger${NEW_MTU} $INTERFACE

ip link set dev $INTERFACE master digger${NEW_MTU} mtu $NEW_MTU up
7 changes: 3 additions & 4 deletions broker/scripts/session.up.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,9 @@ LOCAL_BROKER_PORT="$9"

. scripts/bridge_functions.sh

# Set the interface to UP state
ip link set dev $INTERFACE up mtu $MTU

# Add the interface to our bridge
ensure_bridge digger${MTU}
brctl addif digger${MTU} $INTERFACE
ip link set dev $INTERFACE master digger${MTU} mtu $MTU up

# Turn on bridge port isolation
bridge link set dev $INTERFACE isolated on
Copy link
Member

Choose a reason for hiding this comment

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

This looks nicer than ebtables indeed, but OTOH there is a race condition here now, is there? Between the time this is added to the bridge, and when this bridge link set is executed, the host can communicate with all other hosts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I think you might be right. Maybe we can up the link only after setting isolated on. I'll test that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would a similar "isolated on" be needed in the mtu_changed script?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked, and yes, it would be necessary to set to isolated again when changing bridges with the mtu script. The port status can be seen in /sys/class/net/$INTERFACE/brport/isolated

Also, it's not necessary to use the bridge command. You can also simply echo 1 > /sys/class/net/$INTERFACE/brport/isolated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a fix to my branch. Thanks for testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still recommend doing the port isolation with echo 1 > /sys/class/net/$INTERFACE/brport/isolated since it does not depend on bridge being installed.

This is most likely only important for embedded devices which try to minimize the amount of packages installed on the system.