In D18546#395727, @sobomax wrote:

This is not quite what I am talking about here. We want the value returned by the DHCP server to have an effect when system boots, but we don't want dhclient to continuously restore the state back in case it has been changed by the administrator. My point is that dhclient does not do this periodic "check-up/fix-up" for any other part of configuration that it makes (e.g. ip address, netmask, default gateway, dns server), so there is no reason for an administrator to expect MTU to be in a different bucket. I believe "leave interface alone unless there are some changes on the DHCP side" is the proper approach.

That makes sense, thanks.

In D18546#395707, @eugen_grosbein.net wrote:

In D18546#395642, @sobomax wrote:

2. At some point administrator logins and changes MTU from value X prescribed by the DHCP to some other value Y of his liking.

JFYI: the PR 229432 mentiones working way to ignore option 26 (MTU) supplied by DHCP server:

interface "em0" {

supersede interface-mtu 0;

}

In D18546#395642, @sobomax wrote:

In D18546#395596, @sobomax wrote:

In D18546#395445, @eugen_grosbein.net wrote:

Hmm, there was https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229432 and corresponding commit https://svnweb.freebsd.org/base?view=revision&revision=336195 fixing the problem.

Was it insufficient or your tree does not have that fix?

Yes, you are right. We were looking at the FreeBSD 11.2 code here, I have not noticed there is another change in a trunk to fix the same issue. :(

@eugen_grosbein.net actually upon thinking a little bit more about the difference between this proposed change and r336195 I came to a conclusion that those two are not necessarily mutually exclusive, but rather complementing each other. The problem with just r336195 alone is that it invalidates implicit expectations that dhclient should not try to touch interface at all after initial configuration, unless there are some changes on the DHCP side. As an administrator, I'd not expect dhclient to be actively policing interface parameters and try to enforce them, which would be the case for MTU now. Consider the following hypothetical scenario:

In D18535#395442, @eugen_grosbein.net wrote:

In D18535#395370, @sobomax wrote:

In D18535#395273, @eugen_grosbein.net wrote:

Yes, it requires corruption of private node memory. As owner of multiple routers mass servicing thousands of customers using multiple NETGRAPH nodes I can assure you that panic is not appropriatie action. Appropriate action is some form of block for traffic flow trough the node in question (with logging) leaving other nodes working.

Well, that's where I respectively disagree. As an owner of hundreds of FreeBSD systems servicing many millions of customers I think that rebooting the system immediately after any slight kernel heap/stack memory corruption is detected is not just appropriate but the only sane action available. Shutting down particular netgraph node and hope for the best would just leave the service down indefinitely with no hope for any sorts of automatic recovery.

Node destruction and recreation is easily implemented automatic recovery. We obviously have different usage patterns. I use net/mpd5 that can use ng_nat connecting it to the session that can be destroyed and re-created by any of "client" or "server" recovering from corruption and not interfering with other sessions. Other usage patterns can react on such event to re-create nodes too.

In D18546#395445, @eugen_grosbein.net wrote:

Hmm, there was https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229432 and corresponding commit https://svnweb.freebsd.org/base?view=revision&revision=336195 fixing the problem.

Was it insufficient or your tree does not have that fix?

Hmm, there was https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229432 and corresponding commit https://svnweb.freebsd.org/base?view=revision&revision=336195 fixing the problem.

The general idea seems sound to me. Ideally NIC drivers would also handle it gracefully, but this is nice in that it is close to the source (well, one common source). In fact, iflib handles this gracefully already:

In D18535#395370, @sobomax wrote:

In D18535#395273, @eugen_grosbein.net wrote:

Yes, it requires corruption of private node memory. As owner of multiple routers mass servicing thousands of customers using multiple NETGRAPH nodes I can assure you that panic is not appropriatie action. Appropriate action is some form of block for traffic flow trough the node in question (with logging) leaving other nodes working.

Well, that's where I respectively disagree. As an owner of hundreds of FreeBSD systems servicing many millions of customers I think that rebooting the system immediately after any slight kernel heap/stack memory corruption is detected is not just appropriate but the only sane action available. Shutting down particular netgraph node and hope for the best would just leave the service down indefinitely with no hope for any sorts of automatic recovery.

In D18535#395273, @eugen_grosbein.net wrote:

Looks good with one exception: additional plain panic(). Can it be replaced with KASSERT?

IDK, there is no way this option to be set to anything but DLT_RAW or DLT_EN10MB in the course of normal operation of the node. So it would require some form of memory corruption to actually happen. IDK, panic(9) seems an appropriate action in that case. There are other panic(9) call in the code in similar situations.

Yes, it requires corruption of private node memory. As owner of multiple routers mass servicing thousands of customers using multiple NETGRAPH nodes I can assure you that panic is not appropriatie action. Appropriate action is some form of block for traffic flow trough the node in question (with logging) leaving other nodes working.

I would like to see a generic code in netgraph that marks hooks with DLT. So, no special messages to be needed, nodes will autosense what's connected to them. However, this is just a wish not a blocker for this change.

Looks good with one exception: additional plain panic(). Can it be replaced with KASSERT?

IDK, there is no way this option to be set to anything but DLT_RAW or DLT_EN10MB in the course of normal operation of the node. So it would require some form of memory corruption to actually happen. IDK, panic(9) seems an appropriate action in that case. There are other panic(9) call in the code in similar situations.

In D18535#395220, @eugen_grosbein.net wrote:

Looks good with one exception: additional plain panic(). Can it be replaced with KASSERT?

Looks good with one exception: additional plain panic(). Can it be replaced with KASSERT?

Make sure we have enough data for ethernet header.

I have no specific objections. Looks good to me. It just looks slightly odd, that MTU is handled different from other properties -- set before interface addition to LAGG, while multicast filters set after just before protocol initialization, and capabilities are set after the protocol initialization. As one of downsides this different order required addition of duplicate checks for LAGG nesting.

Addressed feedback by adding handling of new ports, so creating a lagg or adding new ports to an existing lagg sets all the port interfaces to the correct MTU automatically, too.

Addressed feedback I received here and elsewhere on the previous patch. Please consider this alternative approach.

For additional context, here is the code from if_bridge.c this is based on:

case SIOCSIFMTU:
        if (ifr->ifr_mtu < 576) {
                error = EINVAL;
                break;
        }
        if (LIST_EMPTY(&sc->sc_iflist)) {
                sc->sc_ifp->if_mtu = ifr->ifr_mtu;
                break;
        }
        BRIDGE_LOCK(sc);
        LIST_FOREACH(bif, &sc->sc_iflist, bif_next) {
                if (bif->bif_ifp->if_mtu != ifr->ifr_mtu) {
                        log(LOG_NOTICE, "%s: invalid MTU: %u(%s)"
                            " != %d\n", sc->sc_ifp->if_xname,
                            bif->bif_ifp->if_mtu,
                            bif->bif_ifp->if_xname, ifr->ifr_mtu);
                        error = EINVAL;
                        break;
                }
        }
        if (!error)
                sc->sc_ifp->if_mtu = ifr->ifr_mtu;
        BRIDGE_UNLOCK(sc);
        break;

It was not obvious to me why the minimum MTU should be limited by IPv4 but I included it here erring on the side of caution.

The value seems to come from RFC791, quote:

Also, the manpage patch looks good to me.

Bumped the date in the man page.

You need to bump the .Dd at the beginning of the document to the date of the commit since this is a content change.

LGTM! I have spoke with Ryan to get a better scenario about this issue and here there is a reference for that: https://redmine.ixsystems.com/issues/25092

An update as regards the links issues. According to the Primer,

Also the second part has been completely revisited, to be more concise and more uniform to the original text.
Links to specific versions of the OS are still present. Please, refer to this previous comment for more details. How to fix this?

Thanks for the help kristof. I couldn't find the documentation to bound input of sysctl's but I definitely agree it's better to validate it on entry.

Updated with help from kristof to validate sysctl on change.

In D10600#337622, @bcr wrote:

So, you're saying the we need testers for this configuration before we put it in the handbook as official instructions?

So, you're saying the we need testers for this configuration before we put it in the handbook as official instructions?

Applied all the modifications suggested by @bcr . Thank you!
The line-breaking were a first attempt to handle too-long lines, your solution is far better. A couple of comments:

A few suggestions, mostly about line breaks with a few textual corrections.