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.

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

luci-mod-network: include prefix in interface name check #7289

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dannil
Copy link
Contributor

@dannil dannil commented Sep 20, 2024

The interface name check only considered the user input when validating the length, which could result in the actual name exceeding the kernel limit of 15 characters when the bridge/protocol prefix was included as well. For example the interface name abcdefgh1234567 was accepted as valid but with prefixes included (such as br-abcdefgh1234567 or gre-abcdefgh1234567) it exceeded the limit.

The following interface:
Screenshot_11

Results in the following logs:

Fri Sep 20 21:45:32 2024 daemon.notice netifd: Interface 'abcdefgh1234567' is setting up now
Fri Sep 20 21:45:32 2024 daemon.warn netifd: Cannot set device name: 'gre4-abcdefgh1234567' is longer than max size 15
Fri Sep 20 21:45:32 2024 daemon.warn netifd: Failed to initalize device 'gre4-abcdefgh1234567'
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363): Command failed: ubus call network.interface notify_proto { "action": 0, "ifname": "gre4-abcdefgh1234567", "link-up": true, "tunnel": { "mode": "greip", "mtu": 1280, "ipv6": true, "df": true, "multicast": true, "local": "192.168.80.10", "remote": "1.2.3.4", "link": "mng", "data": { } }, "data": { }, "keep": false, "interface": "abcdefgh1234567" } (Invalid argument)
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363): Usage: ubus [<options>] <command> [arguments...]
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363): Options:
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):  -s <socket>:		Set the unix domain socket to connect to
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):  -t <timeout>:		Set the timeout (in seconds) for a command to complete
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):  -S:			Use simplified output (for scripts)
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):  -v:			More verbose output
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):  -m <type>:		(for monitor): include a specific message type
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363): 			(can be used more than once)
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):  -M <r|t>		(for monitor): only capture received or transmitted traffic
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363): Commands:
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):  - list [<path>]			List objects
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):  - call <path> <method> [<message>]	Call an object method
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):  - subscribe <path> [<path>...]	Subscribe to object(s) notifications
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):  - listen [<path>...]			Listen for events
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):  - send <type> [<message>]		Send an event
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):  - wait_for <object> [<object>...]	Wait for multiple objects to appear on ubus
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):  - monitor				Monitor ubus traffic
Fri Sep 20 21:45:32 2024 daemon.notice netifd: abcdefgh1234567 (53363):
Fri Sep 20 21:45:32 2024 daemon.notice netifd: Interface 'abcdefgh1234567' is now down

After this change:

Effective length of gre-abcdefgh123 (15 characters) is allowed.
Screenshot_12

Effective length of gre-abcdefgh1234 (16 characters) isn't allowed.
Screenshot_13

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <[email protected]> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Incremented 🆙 any PKG_VERSION in the Makefile
  • Tested on: (x86/64, SNAPSHOT r27466-f368e2d5ec, Firefox) ✅
  • ( Preferred ) Mention: @ the original code author for feedback @jow-
  • ( Preferred ) Screenshot or mp4 of changes:
  • ( Optional ) Closes: e.g. openwrt/luci#issue-number
  • ( Optional ) Depends on: e.g. openwrt/packages#pr-number in sister repo
  • Description: (describe the changes proposed in this PR)

@dannil
Copy link
Contributor Author

dannil commented Sep 20, 2024

Opening as draft as there's some cases I haven't tested yet if switching the check to prefix + interface name causes any problem.

I also just noticed that gre-abcdefgh1234567 is being brought up by netifd as gre4-abcdefgh1234567, which would mean that even with this change, an interface with name abcdefgh123 (11 characters) with prefix gre would be valid in the interface setup as gre-abcdefgh123 (15 characters), even if it isn't (as seen by the logs below), a bit unsure how to fix that at the moment as it seems we can't rely entirely on Protocol.getProtocol.

config interface 'abcdefgh123'
        option proto 'gre'
        option force_link '1'
        option peeraddr '4.5.6.7'
        option tunlink 'internal'
Fri Sep 20 21:59:48 2024 daemon.notice netifd: Interface 'abcdefgh123' is setting up now
Fri Sep 20 21:59:48 2024 daemon.warn netifd: Cannot set device name: 'gre4-abcdefgh123' is longer than max size 15
Fri Sep 20 21:59:48 2024 daemon.warn netifd: Failed to initalize device 'gre4-abcdefgh123'

The interface name check only considered the user input when validating
the length, which could result in the actual name exceeding the kernel
limit of 15 characters when the bridge/protocol prefix was included as
well. For example the interface name abcdefgh1234567 was accepted as
valid but with prefixes included (such as br-abcdefgh1234567 or
gre-abcdefgh1234567) it exceeded the limit.

Signed-off-by: Daniel Nilsson <[email protected]>
@hnyman
Copy link
Contributor

hnyman commented Sep 21, 2024

Just for reference, the issue (protocol name prefix) has been there for ages.
Links to a few relevant items, starting from year 2015...

@dannil
Copy link
Contributor Author

dannil commented Oct 5, 2024

This proved a bit harder than I first thought, since there doesn't seem to be any consistency for some virtual protocols in how they define their interface names, for example:

  • modemmanager invokes network.registerPatternVirtual(/^mobiledata-.+$/) to register every virtual interface under mobiledata- but the interface name in the protocol is defined as modemmanager-<interface name> and getProtocol() returns modemmanager due to network.registerProtocol('modemmanager').
  • batman-adv inokes network.registerPatternVirtual(/^bat\d+/) to register every virtual interface under bat<number> but the interface name in the protocol is defined as <interface name> and getProtocol() returns batadv due to network.registerProtocol('batadv').

Short of redefining how the naming strategy works for bringing up virtual protocols' interfaces, I'm pretty sure this won't be possible to solve in an nice and concise manner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants