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

Queries are sometimes sent from all-zeroes source MAC address #4

Open
wkz opened this issue Sep 4, 2024 · 2 comments
Open

Queries are sometimes sent from all-zeroes source MAC address #4

wkz opened this issue Sep 4, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@wkz
Copy link
Contributor

wkz commented Sep 4, 2024

When starting mcd on a newly created interface, the MAC address can not always be determined. When that happens, mcd soldiers on and uses an SA of all zeroes.

Preferably, we would not allow any queries to be sent on an interface until a proper SA has been selected.

@troglobit troglobit added the bug Something isn't working label Sep 10, 2024
@troglobit
Copy link
Contributor

Indeed, with no SA mcd should go back to sleep and wake up on the next startup-query-interval, see #5.

@wkz
Copy link
Contributor Author

wkz commented Sep 11, 2024

I looked a bit at the source. Making some notes here.

It seems like we only get one shot, one opportunity, to capture the interface SA - and we unfortunately let it slip:

mcd/src/config.c

Lines 133 to 134 in 23cba46

if (getmac(ifname, ifi->ifi_hwaddr, sizeof(ifi->ifi_hwaddr)))
warn("failed finding hw address for iface %s", ifname);

This is the only caller of getmac() AFAIK, so it seems like if the config parser runs before the interface exists, then we're never going to fill in the SA.

Unless we want to do some fancy netlink parsing, I think we need to call getmac() on every RTM_{NEW,SET}LINK to make sure we keep mcd in sync with the interface config.

While we're there I think we need to consider the condition for starting the interface. At the moment, we settle for IFF_UP, i.e. admin up:

mcd/src/iface.c

Lines 249 to 252 in 23cba46

if (flags & IFF_UP) {
ifi->ifi_flags &= ~IFIF_DOWN;
start_iface(ifi);
}

Should we maybe also require that the link is oper up, i.e. IFF_RUNNING?

If so, that might require us to handle RTM_SETLINK notifications as well:

mcd/src/netlink.c

Lines 48 to 58 in 23cba46

if (nlh->nlmsg_type == RTM_NEWLINK || nlh->nlmsg_type == RTM_DELLINK) {
struct ifinfomsg *ifi = (struct ifinfomsg *)NLMSG_DATA(nlh);
if (nlh->nlmsg_type == RTM_NEWLINK) {
if (!config_find_iface(ifi->ifi_index, 0))
iface_add(ifi->ifi_index, ifi->ifi_flags);
else
iface_check(ifi->ifi_index, ifi->ifi_flags);
} else
iface_del(ifi->ifi_index, ifi->ifi_flags);
}

@troglobit troglobit self-assigned this Sep 24, 2024
@troglobit troglobit removed their assignment Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: No status
Development

No branches or pull requests

2 participants