Details

Reviewers

zlei
cy
pauamma_gundo.com

Group Reviewers

network
manpages

Commits

rG3eaffc626589: netinet6: allow disabling excess log messages

Summary

RFC 4443 specifies cases where certain packets, like those originating from local-scope addresses destined outside of the scope shouldn't be forwarded. The current practice is to drop them, send ICMPv6 message where appropriate, and log the message:

cannot forward src fe80:10::426:82ff:fe36:1d8, dst 2001:db8:db8::10, nxt 58, rcvif vlan5, outif vlan2

At times the volume of such messages cat get very high. Let's allow local admins to disable such messages on per vnet basis, keeping the current default (log).

Reported by: zarychtam@plan-b.pwste.edu.pl

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kaktus created this revision.Feb 16 2023, 6:39 PM

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald TranscriptFeb 16 2023, 6:39 PM

kaktus requested review of this revision.Feb 16 2023, 6:39 PM

kaktus updated this revision to Diff 117469.Feb 16 2023, 8:18 PM

Looks good to me

sys/netinet6/in6_proto.c
406	I think `for the reasons outside of firewall rules` can be omitted. People utilize firewalls should be aware that firewalls have their own logging facilities for the dropped packets (those can't be forwarded).
sys/netinet6/ip6_forward.c
117	As `V_ip6_log_discarded` is mostly constant then I'd prefer to check it first.

This revision is now accepted and ready to land.Feb 17 2023, 2:17 AM

Thanks for taking care of it, looks to me like another positive change.

sys/netinet6/in6_proto.c
405	Perhaps a sentence like "Log packets originating from link-local source addresses that weren't forwarded" will be more accurate (if a precise description is really required), but I am not the best at phrasing it.

kaktus updated this revision to Diff 117767.Feb 22 2023, 3:54 PM

kaktus edited the summary of this revision. (Show Details)

kaktus added a reviewer: manpages.

This revision now requires review to proceed.Feb 22 2023, 3:54 PM

kaktus added inline comments.Feb 22 2023, 3:56 PM

sys/netinet6/in6_proto.c
406	I believe they do but the extra 41 bytes of description could save some time for those who may be in a hurry to find a solution for a local firewall issue :-)

kaktus updated this revision to Diff 117768.Feb 22 2023, 4:02 PM

Rename to match log statement, stats variable and counter description presented by netstat -s.

Discussed with: zarychtam@plan-b.pwste.edu.pl

cy requested changes to this revision.Feb 22 2023, 6:01 PM

cy added a subscriber: cy.

cy added inline comments.

sys/netinet6/in6_proto.c
406	There are other examples (MIBs) which enable dropping packets. They do not mention firewalls. I agree with zlei. The extra verbiage should be removed.

This revision now requires changes to proceed.Feb 22 2023, 6:01 PM

pauamma_gundo.com requested changes to this revision.Feb 22 2023, 7:36 PM

pauamma_gundo.com added a subscriber: pauamma_gundo.com.

pauamma_gundo.com added inline comments.

share/man/man4/inet6.4
32 ↗	(On Diff #117774)	Remember to bump.
348 ↗	(On Diff #117774)
450 ↗	(On Diff #117774)	Add RFC 4443?
451–459 ↗	(On Diff #117774)	Out of scope for this change, but noting while here that a 23 year old draft isn't the best reference.

Address comments:

by popular demand don't mention firewall
update man page wording

share/man/man4/inet6.4
450 ↗	(On Diff #117774)	Other protocols use the SEE ALSO section for that, for example RFC4443 is mentioned like that in icmp6(4) and I don't know which way is preferred by the docs team. This manual page as a whole could use some love as there are many other RFCs to be mentioned in the STANDARDS / SEE ALSO sections.
451–459 ↗	(On Diff #117774)	It seems that this extension hasn't been ratified to official standard[1] but our implementation supports it so maybe as a future rework of this whole manpage at least the "work in progress material" should be removed. https://datatracker.ietf.org/doc/draft-ietf-ipngwg-scopedaddr-format/

pauamma_gundo.com added inline comments.Feb 22 2023, 11:17 PM

share/man/man4/inet6.4
450 ↗	(On Diff #117774)	"SEE ALSO" works for me. I have no marked preference, but it would be puzzling, when the ICMP6 RFC is there in icmp6(4), to have it as a standard in this page.

as requested mention RFC4443 in man page

tidy up

the standard approach is to rate limit with ppsratecheck

i would say this should be 2 patches: 1. do the rate check 2. add the tunable

cy added inline comments.Feb 23 2023, 4:15 PM

sys/netinet6/in6_proto.c
350	Just a nit but cannot is a little more formal than can't. Seems to flow a little better too.

pauamma_gundo.com accepted this revision.Feb 23 2023, 10:23 PM

In D38644#881703, @mjg wrote:

Please refrain from making further requests if you're unwilling to review them later.

kaktus removed a subscriber: mjg.Feb 28 2023, 1:08 PM

This revision was not accepted when it landed; it landed in state Needs Review.Mar 13 2023, 4:50 PM

Closed by commit rG3eaffc626589: netinet6: allow disabling excess log messages (authored by kaktus). · Explain Why

This revision was automatically updated to reflect the committed changes.

kaktus added a commit: rG3eaffc626589: netinet6: allow disabling excess log messages.

netinet6: allow disabling excess log messages
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 117469

sys/netinet6/in6_proto.c

sys/netinet6/ip6_forward.c

sys/netinet6/ip6_mroute.c

sys/netinet6/ip6_var.h

netinet6: allow disabling excess log messagesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 117469

sys/netinet6/in6_proto.c

sys/netinet6/ip6_forward.c

sys/netinet6/ip6_mroute.c

sys/netinet6/ip6_var.h

netinet6: allow disabling excess log messages
ClosedPublic
Actions

Revision Contents
Changeset List