TCP OOB data handling

[rrs]

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.

[rscheff]

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]

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]

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.

[rscheff]

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.

[rrs]

tcp_usrreq.c

984

Yeah I had missed the so

[rrs]

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

[rrs]

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]

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

1378

Maybe let it return a bool? Maybe call it something like tcp_pru_options_support() or so? TCP options rings other bells...

1411

If we use a bool, we want to set error to something....

[rscheff]

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...

[tuexen]

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]

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.

1378

sure

1411

I do not want to use bool here

[rrs]

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

[rscheff]

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