UDP Kernel Tunneling ICMP

[rrs]

Summary

Quite some time ago I added the ability to do UDP tunneling easily from within the kernel. The
major thing I did not add is the ability for the tunneled protocol to also obtain the ICMP that
came in on that UDP port. This, as pointed out to me by Michael Tuexen, is an oversight and needs
to be added.

This little revision adds the ability for any tunneled protocol to optionally also collect the ICMP that
comes in from it. For both current users of this feature we default that to NULL, however I am
sure the users will come along and add in the ties to get their ICMP (I know Michael will).

Question, do I need added changes for IPv6?

Test Plan

Verify that adding this does not break anything (done).. Michael Tuexen will come along and
add support for SCTP and test the actual arrival of ICMP.

[rrs]

So turns out we need to do a wild-card lookup here if there is not
a connection (all the tunneled kernel protocols should be wildcard).

IPv6 will require some thought because it *does* do something different.. sigh.

[rrs]

Note I need to put back in that ASSERT I took out.. opps

[rrs]

Michael:

Please test this now for V6 I think I have it right.. and I have also cleaned up a few
space changes so I think its in good shape.

R

[jtl]

Overall, looks good. (Thanks!) See inline for some style nits and queries.

netinet/udp_usrreq.c

796 ↗

(On Diff #15100)

How expensive is it to do a wildcard match? (I honestly don't know.) If it is expensive, it might be worth conditioning this on some global variable that tracks whether this is needed.

I'm thinking of how the kernel will perform in the case where someone floods a device with malicious ICMP messages.

799 ↗

(On Diff #15100)

style(9) nit: inp != NULL

netinet6/udp6_usrreq.c

559 ↗

(On Diff #15100)

Same question about the cost of the wildcard lookup. Also, here, it looks like the lookup is always done instead of only occurring if a local connection is not found.

(We could mimic the v4 behavior by having in6_pcbnotify() return an integer of the number of sockets notified. If that is 0, then we could run this code.)

563 ↗

(On Diff #15100)

style(9) nit: inp != NULL

570 ↗

(On Diff #15100)

Is there a potential race condition here, using an element of up after unlocking it? (It looks like the same thing happens in the v4 code, as well.)

[rrs]

netinet/udp_usrreq.c

796 ↗

(On Diff #15100)

Well consider that is done every time for any inbound UDP packet it better be cheap :-)

799 ↗

(On Diff #15100)

thanks got it.

netinet6/udp6_usrreq.c

559 ↗

(On Diff #15100)

Hmm but you might just send a notify up to a tunneled socket that is ignored that way. The point
here is we *want* anything going to that port going to the tunneled protocol period. Thus if
the wildcard finds it the ICMP belongs to the protocol thats tunneled.

The lookup costs had better be cheap, take a look inside that lookup function it does a *lot* of lookups.

563 ↗

(On Diff #15100)

Thanks got it!

570 ↗

(On Diff #15100)

And in all things having to do with tunneling. This is done on purpose since
we cannot hold the lock when we call into the tunneled protocol. I guess
the real question is do these things register/un-register themselves a lot and
if so do we make sure all other protocol state is cleaned up before un-registering?

The answer to that should be yes the state should be all cleaned up so I don't
think there is an issue. If we really felt it was I suppose in all places we could go
back in make a local variable, copy it out and then deref that,... this does work
by the way in all cases ... its being used (at least the non-icmp path) by SCTP in
a *lot* of pounding that Michael T (and myself occasionally) do to SCTP...

[tuexen]

We need the possibility of getting ICMP feedback for UDP encapsulated SCTP packets. I tested the patch with an ICMP handler for SCTP/UDP and it works. Will test for ICMP6 and report back. The SCTP part will be committed to head soon after this hits head.

netinet/udp_usrreq.c

798 ↗

(On Diff #15100)

Spaces around | are inconsistent.

1777 ↗

(On Diff #15100)

Two spaces between ) and ||.

[tuexen]

netinet6/udp6_usrreq.c

569 ↗

(On Diff #15100)

Trailing whitespace.

575 ↗

(On Diff #15100)

Trailing whitespace.

[tuexen]

I finally tested the IPv6 code. Using the handlers from
https://github.com/sctplab/sctp-idata/commit/088b4d21f750ccc300005df6dd6b2e8187538f9a
allow the processing of ICMP messages over IPv4 and IPv6.
After addressing the above NITs, I would commit it to head.

[rrs]

netinet/udp_usrreq.c

798 ↗

(On Diff #15100)

Got it :-)

1777 ↗

(On Diff #15100)

fixed in next rev

[rrs]

This fixes Michael's nits