Page MenuHomeFreeBSD

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

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

Details

Reviewers
vmaffione
jhb
kib
np
donner
Group Reviewers
network
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

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

donner added a subscriber: donner.
donner added inline comments.
sys/net/if_vxlan.c
2864

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

This revision is now accepted and ready to land.Fri, Jun 4, 4:07 PM
sys/net/if_vxlan.c
2864

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 added inline comments.
sys/net/if_vxlan.c
2772

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

2865–2866

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.Sat, Jun 5, 9:10 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 inline comments.
sys/net/if_vxlan.c
2864

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.Sun, Jun 6, 4:16 PM
sys/net/if_vxlan.c
2864

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?

sys/net/if_vxlan.c
2864

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

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

This revision is now accepted and ready to land.Wed, Jun 16, 9:28 PM