luci icon indicating copy to clipboard operation
luci copied to clipboard

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

Open dannil opened this issue 1 year ago • 3 comments

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

  • [X] This PR is not from my main or master branch :poop:, but a separate branch :white_check_mark:
  • [X] Each commit has a valid :black_nib: Signed-off-by: <[email protected]> row (via git commit --signoff)
  • [X] Each commit and PR title has a valid :memo: <package name>: title first line subject for packages
  • [X] Incremented :up: any PKG_VERSION in the Makefile
  • [X] Tested on: (x86/64, SNAPSHOT r27466-f368e2d5ec, Firefox) :white_check_mark:
  • [X] ( Preferred ) Mention: @ the original code author for feedback @jow-
  • [X] ( 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
  • [X] Description: (describe the changes proposed in this PR)

dannil avatar Sep 20 '24 19:09 dannil

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'

dannil avatar Sep 20 '24 19:09 dannil

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

  • https://dev.archive.openwrt.org/ticket/20380.html#comment:3
  • #2944
  • #2740

hnyman avatar Sep 21 '24 07:09 hnyman

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.

dannil avatar Oct 05 '24 18:10 dannil