Similar to ng_bridge, but there was hope that if_bridge could avoid such problems.

LGTM

In D31941#772286, @jhb wrote:

udphdr doesn't need 4 byte alignment, just 2 byte. struct ip has an alignment of 2. What makes the compiler unhappy is the abuse of using a uint32_t pointer as a lazy way to multiply hlen by 4. The compiler (correctly) thinks this means that the intermediate up pointer requires 4 byte alignment. One could instead do something rather gross like:

All the solutions do not address the underlying problem: Do we know, that the payload is correctly assigned or not?
If we know, that we should use

alignas(int32_t) void * pstart = &up[p->ip_hl];
struct udphdr *u = pstart;

Thank you for reviewing, @pauamma_gundo.com

• Fix man page grammar

It's not possible to remove a hook while creating it.
Find a better solution.

Thank you. The bug is simple:

In D33673#761884, @kp wrote:

Sorry for the delay so far. This is on my todo list for Monday.

Good catch

In D31871#760752, @melifaro wrote:

In D31871#760716, @donner wrote:

@melifaro may you please point me to the problem with QinQ processing?
How can the processing work, if the ethertype information is lost?

Regardless of what encap is (vlan or qinq) the vlan code effectively does the same - pullups the header and record the tag.
If NIC does not support QINIQ tag removal (or vlanhwtag is explicitly turned off), such frames will be dropped after the change, which is the breakage I'm talking about

This is the first approach to implement the reattach process.
It does compile. I did not test anything yet.

Only alias_nbt.c may return any negative result. Even if no handler is suitable, the returned value from alias_mod.c would be positive (2). Every other handler always returns zero.

Rebase to main.

Thank you for including the fib.

If I understand correctly, the requirement is to align the IP header unconditional.
So there is no need for an extra message, but all other nodes might be urged to add this, too.

The reason for the change of the interface might be located in the pfil_run_hooks.
May be @kp can tell something more about the magic there.

Others call this "host model" instead of "end system", hence I was confused without reading the documentation.

I welcome this extension to other architectures. Thank you.

LGTM

Because both types are "int", the calculation does work.
Good catch!

Thank you for taking care of this.

Thank you for simplifying the code without dropping the functionality.

In D32550#735434, @glebius wrote:

I agree that the ng_ether design with automatic creation of a node proved to be problematic. Ideally it should be explicitly attached and detached. The suggested patch will do that.

Oh, I'm impressed that this fixes the "intended break" triggered by the netgraph test.
Please go ahead!

The motivation behind the check can be understood from the viewpoint of a working system. It is an early dropout for a disconnected system.
But the PR253166 highlights, that this assumption is not valid during the early boot phase. So yes, remove this check.

For the performance aspect, the following code path is involved:

A complete different approach would be to call ether_reassign from if_ethersubr.c

In D32550#734666, @kp wrote:

In part it's to provide symmetry with the existing detach call. The original (Netgate internal) commit message also mentions a desire to avoid netgraph overhead ("This should prevent the netgraph queue to slow down network performance on fast links."), although I've not yet been able to measure any such impact myself. pfSense will actively detach interfaces unless/until it wants to use netgraph on them.

In D32550#734619, @kp wrote:

I may not know enough about netgraph, but I'm not sure how we could put it in the ether namespace. It can't be directed to the ng_ether node, because that doesn't exist at the time we try to (re-)attach.

This proposed message is specific to the ng_ether node. There is no need to clobber the global message space with such a specialized message.

In D31941#721219, @imp wrote:

If char is really 32-bits long, then the uint8_t type must necessarily also be at least 32-bits long. char is the smallest unit you can have in C, absent non-standard extensions (and ignoring the special case of bit fields which aren't a first-class type).

Which architecture causes this problem?

The rules of pf does not replicate exactly the same logic as the ipfw ones.
This might be insignificant how, but causes extra headache, if the test fails in an interesting way ...

okay or the ipfw part

May this cause an unwanted effect on automatic module loading?
So may somebody depend on this dependency in the local setup?

In D31871#719152, @melifaro wrote:

Could you describe the actual problem with ng_vlan? Without the problem statement, it's not exactly clear what can/should be done here.
This diff breaks QinQ processing, so it doesn't look like a path forward.

The current situation is good enough.

Same procedure as last test.

Same game as if_vlan

LGTM

LGTM

The whole netgraph code predates any epoch, so the interactions are interesting.

Because this is an incompatible change to current behavior, it should be mentioned in the RELNOTES or UPDATEING

Good catch.

LGTM

LGTM

Do I understand correctly, that this is the first bpf-option to alter the packet in transit?
Usually I'd solve this problem by adding a simple netgraph network to the outgoing interface.

In D31099#699666, @kp wrote:

My plan was to push the preceding patches (i.e. adding and moving to the new call in libpfctl) first, and this one a week later. That'd give main and stable/X users some time to build a new kernel and userspace before we remove the old call. That achieves having the new kernel support old userspace, with a short transition period (so we don't end up with DIOCGETSTATESNV in a release and we have to support is forever).

Depends on D30959

Good work! But hard to review.