Page MenuHomeFreeBSD

Add lle_event handler to ARP
Needs ReviewPublic

Authored by hrs on Oct 12 2019, 8:02 PM.

Details

Reviewers
melifaro
bz
Group Reviewers
network
Summary

This patch adds an lle_event handler to make an incoming ARP reply send an RTM_ADD/DELETE message to the routing socket. ARP on FreeBSD did not report changes to the ARP table to the routing socket while IPv6 NDP does. It is reasonable to send out an RTM_ADD/DELETE message when an L2-L3 address mapping is resolved even for IPv4 because userland programs can receive it.

This patch also includes a minor change to nd6_lle_event() to make it report the L2 address on expiration of the entry. This additional information is sometimes useful, and reporting it is harmless even if it is not used on the receiver side.

Test Plan

"route -n monitor" and watch if an RTM_ADD message whose sa_family is AF_INET appears when receiving an ARP reply.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 29450
Build 27327: arc lint + arc unit

Event Timeline

hrs created this revision.Oct 12 2019, 8:02 PM
kp added a subscriber: kp.Oct 12 2019, 8:22 PM
kp added inline comments.
sys/netinet/if_ether.c
1539

Not really related to your change, but don't we need to deregister this one as well?

It probably doesn't matter because the default vnet only goes away when we shut down, but it's probably still worth fixing.

bz added inline comments.Oct 13 2019, 4:21 PM
sys/netinet/if_ether.c
1539

No we nowhere do to my best memory cleanup default VNET things and vnet_sysunint will never be run for the default vnet so there is no point in deregistering them ever. Ignore me if I am wrong.

kp added inline comments.Oct 13 2019, 6:06 PM
sys/netinet/if_ether.c
1539

Okay thanks.
In that case we shouldn't unregister either one of them.

hrs added inline comments.Oct 13 2019, 6:22 PM
sys/netinet/if_ether.c
1539

OK. Thank you for pointing out it. I will update the patch shortly.

hrs updated this revision to Diff 63345.Oct 16 2019, 7:11 AM

Update patch:

  • Do not deregister the lle event handlers for each protocol in VNET_SYSUNINIT().
  • Fix a panic when lle->ll_addr == NULL.
bz requested changes to this revision.Oct 16 2019, 7:34 AM

If the netisr logic is restored I am happy with this. I'd commit the IPv6 "style" changes separately though.

sys/netinet/if_ether.c
1500

I'd do the KASSERT before the assignment but that's just personal taste and no need to do.

1537

Why did the logic for the netisr_register change? You are now going to call the global netisr_register for every vnet created rather than just enabling the netisr for the specific vnet. The old logic there was correct.

sys/netinet6/nd6.c
200

This is a style change and not necessarily needed (though I also prefer sizeof(gw) :-) ).

202

The entire rtinfo block again is a style change and no functional change, right? Maybe you can commit these upfront so they are not part of the functional changes?

This revision now requires changes to proceed.Oct 16 2019, 7:35 AM

Looks like a really good change, unifying IPv4/IPv6 lltable behaviour.

Any chance you have some cycles to update the patch?

hrs updated this revision to Diff 68464.Feb 17 2020, 6:37 PM

Updated the patch based on feedback from bz@:

  • Remove unrelated style changes.
  • Revert the wrong netisr_register() change.
  • Put KASSERT() earlier.
hrs marked 4 inline comments as done.Feb 17 2020, 6:39 PM
melifaro accepted this revision as: melifaro.Feb 17 2020, 7:53 PM
hrs added a comment.Feb 27 2020, 7:41 PM

Ping? I will commit the revised patch if there is no objection.

bz accepted this revision as: bz.Feb 27 2020, 9:28 PM

I'd probably still commit the nd6 change separately.

@bz: do I understand correctly that you're okay with the patch but suggesting committing the exact same change but in 2 separate commits?
Both referencing this review, one for if_ether.c and another for nd6.c?

bz added a comment.Mar 10 2020, 9:46 PM

@bz: do I understand correctly that you're okay with the patch but suggesting committing the exact same change but in 2 separate commits?
Both referencing this review, one for if_ether.c and another for nd6.c?

Yes, with two commit messages explaining the changes of each (as they are not directly related).