Page MenuHomeFreeBSD

Add lle_event handler to ARP
AcceptedPublic

Authored by hrs on Oct 12 2019, 8:02 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 23, 9:43 PM
Unknown Object (File)
Jan 8 2024, 8:12 PM
Unknown Object (File)
Jan 4 2024, 3:08 PM
Unknown Object (File)
Dec 22 2023, 11:20 PM
Unknown Object (File)
Nov 11 2023, 12:09 PM
Unknown Object (File)
Nov 11 2023, 5:40 AM
Unknown Object (File)
Nov 11 2023, 1:38 AM
Unknown Object (File)
Nov 6 2023, 7:49 AM
Subscribers

Details

Reviewers
melifaro
bz
gnn
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 - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 27063
Build 25348: arc lint + arc unit

Event Timeline

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.

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.

sys/netinet/if_ether.c
1539

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

sys/netinet/if_ether.c
1539

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

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
193

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

204

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?

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

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

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: 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).

Ping :-)
Any chance we can land it in 13?

This revision is now accepted and ready to land.Jan 22 2021, 3:20 PM