Page MenuHomeFreeBSD

Fix bug in ifconfig preventing proper vlan creation
ClosedPublic

Authored by hselasky on Dec 9 2020, 11:45 AM.

Details

Summary

Detection of interface type by filter must happen before detection of interface type by prefix. Else the following sequence of commands will try to create a LAGG interface instead of a VLAN interface, which accidentially worked previously, because the date pointed to by the ifr_data pointer was not parsed for VLAN's. This is a regression after r366917 (adding MT_FILTER) and r368229 (VLAN creation now parses the ifr_data field).

ifconfig lagg0 create
ifconfig lagg0.256 create

MFC after: 3 days
Sponsored by: Mellanox Technologies // NVIDIA Networking

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib added inline comments.
sbin/ifconfig/ifclone.c
137 ↗(On Diff #80474)

Extra ()

146 ↗(On Diff #80474)

Extra ()

This revision is now accepted and ready to land.Dec 9 2020, 1:56 PM

Address comments from Konstantin.

This revision now requires review to proceed.Dec 9 2020, 3:17 PM

I would have perhaps kept the old structure (still dropping the weird tautological comparison) so we only walk once and explore both possibilities in one loop (just stop looking at prefix-based callbacks if we found one, stop looking at filter-based callbacks if we found one, stop if we found one of each), but this is fine too and probably less error prone. There's also not much benefit to grabbing both for any in-tree stuff.

This revision is now accepted and ready to land.Dec 9 2020, 3:48 PM

stop if we found one of each

That's exactly what happens. lagg0.256 matches both lagg prefix and the vlan filter.

stop if we found one of each

That's exactly what happens. lagg0.256 matches both lagg prefix and the vlan filter.

Right, so in the consolidated view we'd have dcp_prefix and dcp_filter if it matched one of each, then make a decision based on that.

@kevans: I think the current code is pretty clear. I'm not sure how much we'll gain introducing another variable ...