Details

Reviewers

tuexen
rscheff

Group Reviewers

transport

Commits

rS360638: Adjust the fb to have a way to ask the underlying stack

Summary

This is the first step in TCP OOB handling on a per stack basis. We first introduce a
check for OOB (question should it only be PRUS_OOB or do we need to be looking for MSG_OOB)?

If we see the OOB signal, we ask a function if it's an error. This function now
returns nothing but 0 "its ok" but the next step will add a tcp stack specific query so that
a stack can examine the PRUS_flag and say yes or no?

Another question should we also take care of the other PRUS_flags (PRUS_EOF?)....

Test Plan

Write a small test code and make sure that MSG_OOB still works as one would expect. Note I am
not sure it really works from my code examination.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

rrs requested review of this revision.Apr 26 2020, 3:05 PM

rrs created this revision.

rscheff added a subscriber: rscheff.Apr 26 2020, 10:01 PM

rscheff added inline comments.

tcp_usrreq.c
984	the socket MSG_OOB flag is directly mapped into protocol PRU_OOB flag, not? (they also happen to be numeric identical). Conceptually, the protocol itself should be checking the PRUS_* flags, not the MSG_* flags. uipc_socket.c:1499: (flags & MSG_OOB) ? PRUS_OOB : uipc_socket.c:1729: pru_flag = (flags & MSG_OOB) ? PRUS_OOB :

rrs added inline comments.Apr 27 2020, 11:24 AM

tcp_usrreq.c
984	The problem here can be found in uipc_socket.c. Before this function became the pru_send routine we "used" to use sosend_generric() or maybe sosend_stream(), go look at these two functions (in fact look too at sosend_dgram) all of them map MSG_OOB -> PRUS_OOB and MSG_EOF -> PRUS_EOF But now, since this function is what is called that is not happening, since this function does nto have the proper mapping. Yes conceptually the protocol needs to use PRUS_... but someone must do the translation from the socket level to the protocol level. And that used to be done in sosend_xxxx() function which now is no longer called. It most likely should be done here.

rscheff added inline comments.Apr 27 2020, 12:27 PM

tcp_usrreq.c
984	Then I must be missing some context here. Are you saying that you are bypassing the socket interface, and instead are calling tcp_usr_send() directly in some deployments, where the MSG_ flags are used instead of the PRUS_ flags? kern_sendfile.c has vn_sendfile, this however doesn't support OOB either apparently.

remove MSG_ flag check(s) in tcp_usr_send()

tcp_usrreq.c
984	see transport@; I believe you missed that sosend() is calling the pru_sosend() pointer, and not the pru_send() pointer. So the socket API for TCP still runs thought either sosend_generic (default) or sosend_stream (sysctl selectable), both of which do the conversion from MSG_OOB / MSG_EOF in the "correct" layer - and TCP here only has to care about the PRU_ type flags.

This revision now requires changes to proceed.Apr 27 2020, 3:02 PM

rrs added inline comments.Apr 27 2020, 4:23 PM

tcp_usrreq.c
984	Yeah I had missed the so

Now that I know MSG_OOB is out of the picture make the appropriate update.

Now that D24575 is in lets take it a step further and provide the hooks so
we can ask a transport (optionally) if they support various PRU_XXX flags.
If they don't provide a function then they "support all" PRU_XXX flags.

Once this is in and D24576 is completed, I will make a follow on one
to have rack and bbr define the functions that will specify OOB is not supported.

tuexen added inline comments.Apr 27 2020, 5:38 PM

tcp_usrreq.c
993	I think letting `tcp_options_support()` return the error and lateron overwriting it is not optimal. What about letting `tcp_options_support()` return a bool, and replace the above with: if (flags & PRUS_OOB && !tcp_options_support(tp, PRUS_OOB)) { if (control) m_freem(control); if (m && (flags & PRUS_NOTREADY) == 0) m_freem(m); error = EINVAL; goto out; }
1377	Maybe let it return a `bool`? Maybe call it something like `tcp_pru_options_support()` or so? TCP options rings other bells...
1415	If we use a bool, we want to set error to something....

Thanks! I also like Michael's comments;

The stack specific handler tfb_pru_options() could return a bitmasked field for all the PRU_* Flags, where tcp_pru_support_options checks one or more, as specified by flags like return(tcp_pru_support(tp) == flags) or rather return (tcp_pru_support(tp) & flags).

In keeping tradition with the base stack, suggest to use int (uint32_t) for the return value instead of bool.

Returning multiple flags that way may come useful if you want to do an all-or-nothing check, followed up by the return value checked against individual features without having to call it multiple times...

In D24574#541303, @rscheff wrote:

Thanks! I also like Michael's comments;

The stack specific handler tfb_pru_options() could return a bitmasked field for all the PRU_* Flags, where tcp_pru_support_options checks one or more, as specified by flags like return(tcp_pru_support(tp) == flags) or rather return (tcp_pru_support(tp) & flags).

The use case right now is to check for a single flag. I don't see that a lot of stuff is added there.

In keeping tradition with the base stack, suggest to use int (uint32_t) for the return value instead of bool.

I was told on the developer list that new code should be modern and use bool... But personally I don't care much how a bool is encoded.

Returning multiple flags that way may come useful if you want to do an all-or-nothing check, followed up by the return value checked against individual features without having to call it multiple times...

Right now we have

#define PRUS_OOB        0x1
#define PRUS_EOF        0x2
#define PRUS_MORETOCOME 0x4
#define PRUS_NOTREADY   0x8

and I don't see much else coming up to be stack specific.

rrs added inline comments.Apr 28 2020, 12:40 PM

tcp_usrreq.c
993	That was a code cloning error... The error = CONNRESET should not be present. and I prefer not to make it a bool, but to have the stack return 0 = I support it ERRNO = I don't support it and here is the error. The overwrite was just a mistake.
1377	sure
1415	I do not want to use bool here

Update to fix inadvertent overwrite of error and the name of the option function change per Michael's comment

lgtm. Perhaps a comment that the function's return should be zero when option is supported, or a specific error code?

Naively, one may assume non-zero for support, and zero for no support (inverse).

tuexen accepted this revision.Apr 28 2020, 2:00 PM

This revision is now accepted and ready to land.Apr 28 2020, 2:00 PM

Closed by commit rS360638: Adjust the fb to have a way to ask the underlying stack (authored by rrs). · Explain WhyMay 4 2020, 8:20 PM

This revision was automatically updated to reflect the committed changes.

rrs added a commit: rS360638: Adjust the fb to have a way to ask the underlying stack.

Herald added subscribers: melifaro, imp. · View Herald TranscriptMay 4 2020, 8:20 PM

TCP OOB data handling
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 71105

tcp_usrreq.c

tcp_var.h

TCP OOB data handlingClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 71105

tcp_usrreq.c

tcp_var.h

TCP OOB data handling
ClosedPublic
Actions

Revision Contents
Changeset List