BIO_dgram support for BIO_sendmmsg/BIO_recvmmsg #18270

hlandau · 2022-05-09T13:33:44Z

Flags like BIO_DONTWAIT ended up getting dropped because Windows can't support it.

There is a new API which must be used to enable local address specification/detection explicitly.

Some previous discussion in #18238.

hlandau · 2022-05-10T06:54:23Z

The errno packing thing is a bit goofy. Could just add an int *err argument.

WSASendMsg requires Vista so needs _WIN32_WINNT to be bumped to 0x0600. Could probe for it dynamically with GetProcAddress and use it when available. Actually this would help mingw compatibility since it's missing headers for this.

hlandau · 2022-05-11T08:36:09Z

It turns out WSASendMsg and WSARecvMsg, which were introduced in Vista, are not exported as normal symbols by Windows. Instead, you have to use them by obtaining function pointers to them, by creating a socket and calling ioctl on it. What.

In any case, this does have the advantage that we probe for this functionality at runtime and will fall back on platforms without it (XP, etc.).

When a datagram is sent from the local machine, Windows doesn't provide source/destination information in the control message received from WSARecvMsg even if IP_PKTINFO is enabled. Instead, Control.len is just set to 0 when WSARecvMsg returns. The API as documented in BIO_sendmmsg(3) has been updated to reflect this OS-specific quirk. In this case we zero the output BIO_ADDR.

BIO_DONTWAIT was dropped due to lack of Windows support.

hlandau · 2022-05-11T11:29:17Z

@mattcaswell I think this is ready for review now.

t8m

Great work!

crypto/bio/bss_dgram.c

doc/man3/BIO_sendmmsg.pod

test/bio_dgram_test.c

t8m · 2022-05-30T13:58:11Z

test/bio_dgram_test.c

+
+    /* First effort should fail due to missing destination address */
+    ret = BIO_sendmmsg(b1, tx_msg, sizeof(BIO_MSG), 1, 0);
+    if (!TEST_int_le((int)ret, -32))


Should you use BIO_IS_ERRNO()?

0x5d8ef03273C8274A1588BAccDC6A13e52aE9Bda4

levitte · 2022-05-31T09:20:50Z

crypto/bio/bss_dgram.c

+# else
+#  define M_METHOD  M_METHOD_NONE
+# endif
+#endif


Not sure I'm too keen on the result preprocessing condition salad... We have done things like this differently before, see for example crypto/threads_*.c

I'm not going to be very pushy on this, though...

crypto/bio/bss_dgram.c

hlandau · 2022-06-01T08:33:05Z

Updated.

t8m · 2022-06-06T11:31:29Z

ping for second review

mattcaswell

I think this functionality justifies a mention in CHANGES.md

doc/man3/BIO_sendmmsg.pod

mattcaswell · 2022-06-06T13:47:02Z

include/internal/e_os.h

@@ -96,7 +96,7 @@
        * might be possible to achieve the goal by /DELAYLOAD-ing .DLLs
        * and check for current OS version instead.
        */
-#    define _WIN32_WINNT 0x0501
+#    define _WIN32_WINNT 0x0600


This is a breaking change. Or at least potentially it is. It explicitly drops support for Windows XP and Windows Server 2003. I'm not aware of anyone actually building OpenSSL 3.x on those platforms, so I would not be surprised if we're broken there anyway - but this explicit dropping of support is not to be taken lightly. It would require an OTC decision IMO.

My understanding is that increasing _WIN32_WINNT allows us to use newer features, but it does not automatically break compatibility. The availability of WSARecvMsg is probed at runtime and there is a fallback if it is not available.

By bumping this there is a risk we accidentially start depending on APIs not supported on XP. I guess maybe we should support a build with version 0x0501, maybe in CI.

My code as stands detects availability of WSARecvMsg-related definitions by whether WSAID_WSARECVMSG is defined. So we could make _WIN32_WINNT configurable and the code would automatically use the best function available, whether it's set to 0x0501 or 0x0600. We could discuss what we want to do about evolving _WIN32_WINNT, though it feels like a separate discussion.

we could make _WIN32_WINNT configurable

I think it should already be configurable, i.e. you can pass -D_WIN32_WINNT=... at configure time. Does your code require _WIN32_WINNT to be 0x0600 before it will work?

The change here changes the default value for _WIN32_WINNT which I don't think we can do without some agreement that it is ok to do so.

As things stand, if we leave _WIN32_WINNT as 0x0501 by default, the WSARecvMsg code is going to be a dead code path. Do we really want to ship code that will never be used unless people compile OpenSSL in an undocumented way?

I suppose we should document it in the NOTES.windows then? And possibly also change the default to 0x0600 if OMC agrees so (supposedly the override with 0x0501 should be kept working).

I raised this at the OMC meeting today, and it led to this proposal:

openssl/general-policies#22

test/bio_dgram_test.c

crypto/bio/bss_dgram.c

hlandau · 2022-06-07T11:28:50Z

So, currently, the API isn't required to try and send multiple messages. Meaning an implementation that always returns 1 is permissible. My thinking here is that callers need to handle the case where BIO_sendmmsg doesn't send all messages and call it again anyway.

This is consistent with the semantics for sendmmsg(2):

On success, sendmmsg() returns the number of messages sent from msgvec; if this is less than vlen, the caller can retry with a further sendmmsg() call to send the remaining messages.

Based on this, I'm thinking it might actually be better to remove the sendmmsg emulation loop from the sendmsg case, so that it's similar to the sendto case.

So the options are:

If sendmmsg is not available, emulate with a loop (using sendmsg or sendfrom)
Con: Doesn't offer any performance advantage over having the caller do this. But the caller will have to have such a loop anyway (as even sendmmsg(2) might not send all messages), so this duplicates the loop
Con: This will probably need to an unnecessary extra system call. To see why, consider how this would work:
- Call BIO_sendmmsg to send 100 messages
- BIO_sendmmsg calls sendmsg/sendto successfully 50 times, on the 51st time it fails due to the send buffer being full
- BIO_sendmmsg returns 50
- This is less than 100, so caller calls BIO_sendmmsg again with the remaining messages
- BIO_sendmmsg fails immediately on the first call because the send buffer is full; this is a duplication of the 51st call above; returns 0/error code and caller stops
If sendmmsg is not available, always process only one message
Pro: Makes it clear callers must not expect all messages to be processed and that BIO_sendmmsg may need to be called again
Pro: Avoids unnecessary extra system call:
- Caller calls BIO_sendmmsg with 100 messages, returns 1, calls again with 99 messages, etc.
- When BIO_sendmmsg returns 0, caller receives error code and understands it should stop/defer

I am leaning towards the second option, which would have me remove the loop for the sendmsg(2) case. Thoughts?

mattcaswell · 2022-06-07T13:05:45Z

I'd be ok with the second option. As you say the caller still has to be able to handle the case when less than the number of requested messages is sent anyway so it doesn't add much for the BIO to emulate it.

hlandau · 2022-06-07T19:42:13Z

Assorted updates. Now the sendmsg cases etc. send one message. Repeated sendmmsg calls have also been removed.

t8m · 2022-06-08T06:12:17Z

CI is relevant

crypto/bio/bss_dgram.c

t8m · 2022-06-08T06:23:55Z

crypto/bio/bss_dgram.c

-                    return num_done > 0 ? num_done : -1;
-            }
-        }
+    if (num_msg > BIO_MAX_MSGS_PER_CALL)


Should the BIO_MAX_MSGS_PER_CALL be a public define so the callers won't needlessly attempt to batch more messages? And document that BIO_MAX_MSGS_PER_CALL is a recommended limit on the BIO_sendmmsg call.

I don't think we should make any specific promises about the behaviour of this API in terms of the number of messages it handles at this time. There can be other implementations like BIO_dgram_pair and third party implementations which can do what they want.

Maybe a BIO_ctrl to query a suggested batching factor would be something worth considering, rather than a constant.

Maybe a BIO_ctrl to query a suggested batching factor would be something worth considering, rather than a constant.

Yep, that seems to me as something very useful for callers.

I'd be a little wary about specifically documenting this as a "recommended" batching factor -- there are a lot of topics that influence the best batching factor and depending on the NIC and network hardware trying to batch too much at once can reduce performance.
I'd rather see this documented as a limit or cap on the batching factor achievable, ideally with a note that the optimal batching factor will be highly context dependent.

crypto/bio/bss_dgram.c

hlandau · 2022-06-08T06:54:24Z

OSX failure is going to be annoying to figure out, but thinking about it.

… BIO_dgram support for BIO_sendmmsg/BIO_recvmmsg

hlandau · 2022-08-22T09:10:59Z

Updated and rebased, now that the API is merged. Also addressed (I believe) all outstanding comments.

Unsure what's going on with doc-nits. This PR doesn't change the file it's complaining about.

stale

paulidale

Subject to the few nits being fixed.

I've also asked a question about the test case not indicating a reason for failure but I don't think it's a blocker.

crypto/bio/bio_local.h

test/bio_dgram_test.c

crypto/bio/bss_dgram.c

hlandau · 2022-08-26T06:47:10Z

Updated.

crypto/bio/bss_dgram.c

hlandau · 2022-08-26T09:33:00Z

Updated.

mattcaswell

Approved. @paulidale - still ok?

paulidale

Still good.

openssl-machine · 2022-09-01T08:00:30Z

This pull request is ready to merge

paulidale · 2022-09-01T08:03:22Z

Merged, finally!

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #18270)

levitte

Sorry that this comes so late

levitte · 2022-09-04T09:28:57Z

crypto/bio/bss_dgram.c

+
+        /* See comment above. */
+        if (local->s_in.sin_port != 0
+            && data->local_addr.s_in.sin_port != local->s_in.sin_port) {


I just discovered, through a build on FreeBSD, that this is untested code. The error I got is this:

crypto/bio/bss_dgram.c:1165:16: error: use of undeclared identifier 'data' && data->local_addr.s_in.sin_port != local->s_in.sin_port) { ^

Through this, I see the following declaration sprinkled into all different variants of code in this function:

CMSGHDR_TYPE *cmsg; bio_dgram_data *data = b->ptr;

Since those are common for all variants, it would probably be safer if they were declared once and for all at the beginning of this function.

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#18270)

hlandau added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending triaged: feature The issue/pr requests/adds a feature labels May 9, 2022

hlandau requested review from t8m and mattcaswell May 9, 2022 13:33

hlandau self-assigned this May 9, 2022

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label May 9, 2022

hlandau mentioned this pull request May 13, 2022

Add dgram API discussion #18210

Closed

faramir-dev force-pushed the master branch from 36c269c to 3c0e8bc Compare May 24, 2022 04:50

t8m requested changes May 30, 2022

View reviewed changes

levitte reviewed May 31, 2022

View reviewed changes

kaduk reviewed Jun 1, 2022

View reviewed changes

crypto/bio/bss_dgram.c Outdated Show resolved Hide resolved

t8m previously approved these changes Jun 1, 2022

View reviewed changes

t8m removed the approval: otc review pending label Jun 1, 2022

t8m closed this Jun 6, 2022

t8m reopened this Jun 6, 2022

mattcaswell requested changes Jun 6, 2022

View reviewed changes

t8m reviewed Jun 8, 2022

View reviewed changes

mattcaswell mentioned this pull request Jun 8, 2022

Stop supporting Windows XP and Windows Server 2003 from OpenSSL 3.1 openssl/general-policies#22

Open

hlandau added 4 commits August 22, 2022 10:04

fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup!…

1282192

… BIO_dgram support for BIO_sendmmsg/BIO_recvmmsg

fixup! BIO_dgram support for BIO_sendmmsg/BIO_recvmmsg

abc72b9

fixup! BIO_dgram support for BIO_sendmmsg/BIO_recvmmsg

398ee10

fixup! BIO_dgram support for BIO_sendmmsg/BIO_recvmmsg

f00b39b

hlandau force-pushed the dgram-mmsg2 branch from 12f439b to f00b39b Compare August 22, 2022 09:11

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Aug 22, 2022

paulidale requested a review from t8m August 26, 2022 01:05

paulidale approved these changes Aug 26, 2022

View reviewed changes

crypto/bio/bio_local.h Outdated Show resolved Hide resolved

test/bio_dgram_test.c Outdated Show resolved Hide resolved

crypto/bio/bss_dgram.c Outdated Show resolved Hide resolved

crypto/bio/bss_dgram.c Show resolved Hide resolved

paulidale added the approval: review pending This pull request needs review by a committer label Aug 26, 2022

fixup! BIO_dgram support for BIO_sendmmsg/BIO_recvmmsg

88af43e

paulidale approved these changes Aug 26, 2022

View reviewed changes

mattcaswell requested changes Aug 26, 2022

View reviewed changes

crypto/bio/bss_dgram.c Outdated Show resolved Hide resolved

crypto/bio/bss_dgram.c Outdated Show resolved Hide resolved

fixup! BIO_dgram support for BIO_sendmmsg/BIO_recvmmsg

28d9a90

mattcaswell approved these changes Aug 26, 2022

View reviewed changes

paulidale approved these changes Aug 31, 2022

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Aug 31, 2022

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Sep 1, 2022

paulidale closed this Sep 1, 2022

openssl-machine pushed a commit that referenced this pull request Sep 1, 2022

BIO_dgram support for BIO_sendmmsg/BIO_recvmmsg

664e096

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #18270)

levitte reviewed Sep 4, 2022

View reviewed changes

sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022

BIO_dgram support for BIO_sendmmsg/BIO_recvmmsg

365b1f7

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#18270)

beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022

BIO_dgram support for BIO_sendmmsg/BIO_recvmmsg

5fcf5bb

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#18270)

BIO_dgram support for BIO_sendmmsg/BIO_recvmmsg #18270

BIO_dgram support for BIO_sendmmsg/BIO_recvmmsg #18270

Conversation

hlandau commented May 9, 2022

hlandau commented May 10, 2022

hlandau commented May 11, 2022 • edited Loading

hlandau commented May 11, 2022

t8m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levitte May 31, 2022 • edited Loading

Choose a reason for hiding this comment

hlandau commented Jun 1, 2022

t8m commented Jun 6, 2022

mattcaswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlandau commented Jun 7, 2022

mattcaswell commented Jun 7, 2022

hlandau commented Jun 7, 2022

t8m commented Jun 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlandau commented Jun 8, 2022

hlandau commented Aug 22, 2022

paulidale left a comment

Choose a reason for hiding this comment

hlandau commented Aug 26, 2022

hlandau commented Aug 26, 2022

mattcaswell left a comment

Choose a reason for hiding this comment

paulidale left a comment

Choose a reason for hiding this comment

openssl-machine commented Sep 1, 2022

paulidale commented Sep 1, 2022

levitte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlandau commented May 11, 2022 •

edited

Loading

levitte May 31, 2022 •

edited

Loading