if_vxlan(4): Allow netmap_generic to intercept RX packets.

[afedorov]

Summary

Netmap (generic) intercepts the if_input method to handle RX packets. So:

• Call ifp->if_input() instead of netisr_dispatch().
• Add stricter check for incoming packet length.

This change is very useful with bhyve + vale + if_vxlan.

We have been using this patch for several years.

Test Plan

Setup the following configuration:
Host 1: bhyve VM - vale(4) switch - if_vxlan(4)
Host 2: bhyve VM - vale(4) switch - if_vxlan(4)

Before applying the patch, there is no network connection between the VMs.

[donner]

sys/net/if_vxlan.c

2866

So we never get an error indication if the packet could not be transmitted?

[afedorov]

sys/net/if_vxlan.c

2866

Unfortunately yes. But the calling code doesn't handle errors either.

2792         vni = ntohl(vxh->vxlh_vni) >> VXLAN_HDR_VNI_SHIFT;
2793 
2794         /* Adjust to the start of the inner Ethernet frame. */
2795         m_adj_decap(m, offset + sizeof(struct vxlan_header));
2796 
2797         error = vxlan_input(vso, vni, &m, srcsa);
2798         MPASS(error != 0 || m == NULL);
2799 
2800 out:
2801         if (m != NULL)
2802                 m_freem(m);
2803 }

[kib]

sys/net/if_vxlan.c

2774

I would place this check into vxlan_input(), since that is the function that access inner ethernet header.

2867–2868

And I would set error = 0 there.

[np]

I know it's not directly related but the 'if' here is not needed. If you're tightening the len checks in the function then maybe clean this too?

out:

if (m != NULL)
        m_freem(m);

[afedorov]

• Move ethernet packet length check to vxlan_input().

[afedorov]

In D30638#688320, @np wrote:

I know it's not directly related but the 'if' here is not needed. If you're tightening the len checks in the function then maybe clean this too?

out:

if (m != NULL)
        m_freem(m);

Do you mean 'out' label in vxlan_rcv_udp_packet()?

If yes, then I'm not sure, because vxlan_input() can return an error without touching the mbuf.

[vmaffione]

sys/net/if_vxlan.c

2866

However, the calling code may be improved in the future. And additional callers may be added. Why are we removing an error propagation code path?
I think a function should return the appropriate return value irrespective of the caller ignoring that. It's more future proof.

[afedorov]

sys/net/if_vxlan.c

2866

Unfortunately, the if_input() ( by default: ether_input() ) method does not return an error.

void    (*if_input)

There are many drivers that call if_input () directly. Even netmap calls if_input() and doesn't handle the error.

Is this error handling really necessary for if_vxlan?

[donner]

sys/net/if_vxlan.c

2866

Error handling is not necessary at this point and within this review.

[vmaffione]

Sounds good. I had not noticed that *if_input returns void.