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

Authored by afedorov on Jun 4 2021, 4:02 PM.

Details

Reviewers

vmaffione
jhb
kib
np
donner

Group Reviewers

network

Commits

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

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.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

afedorov created this revision.Jun 4 2021, 4:02 PM

Herald added subscribers: melifaro, ae. · View Herald TranscriptJun 4 2021, 4:02 PM

afedorov requested review of this revision.Jun 4 2021, 4:02 PM

donner accepted this revision as: donner.Jun 4 2021, 4:07 PM

donner added a subscriber: donner.

donner added inline comments.

sys/net/if_vxlan.c
2866	So we never get an error indication if the packet could not be transmitted?

This revision is now accepted and ready to land.Jun 4 2021, 4:07 PM

afedorov added inline comments.Jun 4 2021, 4:31 PM

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 accepted this revision.Jun 4 2021, 6:03 PM

kib added inline comments.

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.

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);

Move ethernet packet length check to vxlan_input().

This revision now requires review to proceed.Jun 5 2021, 9:10 AM

afedorov marked 3 inline comments as done.Jun 5 2021, 9:11 AM

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 added a subscriber: allanjude.Jun 6 2021, 9:07 AM

vmaffione requested changes to this revision.Jun 6 2021, 4:16 PM

vmaffione added inline comments.

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.

This revision now requires changes to proceed.Jun 6 2021, 4:16 PM

afedorov added inline comments.Jun 15 2021, 2:24 PM

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?