Thanks for the patch. After you resubmit, I'd like to try it on the Clearfog board as well.

Added some comments.
Keep up the good work :)

I also have these changes up on Github at: https://github.com/freebsd/freebsd/compare/master...theasylum:fix/e6000sw-fdt if it is easier to review there due to more context. I did re-create the diff as requested by @manu with -U999, so hopefully that helps.

Updated to match style(9) thanks to @dteske's comment.

Try to get ahold of mw@ and loos@ to review

In D10600#397152, @rockyhotas_post.com wrote:

In D10600#397149, @bcr wrote:

Yes, I've changed those.

Ok!

Now that the server part is about to be committed, do you also plan to do a section about the client? We have an article mentioning pam_ldap, but that article should either be reworked or put into the handbook altogether.

Yes, maybe you are referring to this section.

In D10600#397149, @bcr wrote:

Yes, I've changed those.

Yes, I've changed those.
Now that the server part is about to be committed, do you also plan to do a section about the client? We have an article mentioning pam_ldap, but that article should either be reworked or put into the handbook altogether.

In D10600#397141, @bcr wrote:

I think the patch is fine now. I'll see to it that it gets committed.
Thanks for your patience, Rocky Hotas.

I think the patch is fine now. I'll see to it that it gets committed.
Thanks for your patience, Rocky Hotas.

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.