Add support for kTLS zerocopy sendfile on Linux #18650

nvmmax · 2022-06-24T09:27:12Z

TLS device offload allows to perform zerocopy sendfile transmissions.
FreeBSD provides this feature by default, and Linux 5.19 introduced it
as an opt-in. Zerocopy improves the TX rate significantly, but has a
side effect: if the underlying file is changed while being transmitted,
and a TCP retransmission happens, the receiver may get a TLS record
containing both new and old data, which leads to an authentication
failure and termination of connection. This effect is the reason Linux
makes a copy on sendfile by default.

This commit adds support for TLS zerocopy sendfile on Linux and uses it
by default, aligning OpenSSL's behavior on Linux and FreeBSD. A new
option called KTLSTxNoZerocopySendfile is added to disable the new
behavior on Linux and revert to copying. This option should be used when
the application can't guarantee that the file is not modified, and it's
not desired to break the connection.

The related documentation is also added in this commit. The unit test is
not added, because testing this functionality requires specific hardware
and a non-local peer.

Signed-off-by: Maxim Mikityanskiy maximmi@nvidia.com
Reviewed-by: Tariq Toukan tariqt@nvidia.com
Reviewed-by: Boris Pismenny borisp@nvidia.com

Checklist

documentation is added or updated

openssl-machine · 2022-07-25T00:07:50Z

This PR has the label 'hold: cla required' and is stale: it has not been updated in 30 days. Note that this PR may be automatically closed in the future if no CLA is provided. For CLA help see https://www.openssl.org/policies/cla.html

hlandau · 2022-07-25T11:12:44Z

We need a CLA signed in order to be able to consider this PR:

https://www.openssl.org/policies/cla.html

nvmmax · 2022-07-25T11:17:54Z

Thanks for reminding me, I'm aware of that, but unfortunately it takes time for the authorized people to sign the corporate CLA.

nvmmax · 2022-08-30T10:31:16Z

Can the CI be triggered for my pull request?

mattcaswell · 2022-08-30T13:04:46Z

Can the CI be triggered for my pull request?

Done

gal-pressman · 2022-09-04T05:42:37Z

Hey, can someone please take a look at this patch?

ttoukan · 2022-09-14T10:26:25Z

Hi, what's the status of this one? I see that all checks passed. Is there any blocker?

hlandau

One nit, otherwise looks good.

doc/man3/SSL_CONF_cmd.pod

mattcaswell

Seems like this PR justifies an entry in CHANGES.md

mattcaswell · 2022-09-16T15:36:30Z

doc/man1/openssl-s_server.pod.in

+performance boost when used with kTLS device offload. Note that invalid TLS
+records might be transmitted if the file is modified while being sent, so this
+option can be used to disable zerocopy. This option is only valid when
+B<-sendfile> is specified.


It seems to me that by having this on by default you are introducing a breaking change. Stuff that worked before may no longer work. That's not allowed in a minor release, so it might be better to have it off by default and explicitly ask for it to be on.

OpenSSL already supports zerocopy TLS sendfile on FreeBSD, and it's always active (i.e. there isn't even an option to disable it). The disclaimer that the file shouldn't be modified during sendfile already applies to FreeBSD to the same extent.

Since OpenSSL is a cross-platform library:

The behavior on different platforms should be as close as possible, i.e. if OpenSSL on FreeBSD uses zerocopy by default, OpenSSL on Linux should use it too.

I would assume that applications using a cross-platform library do not rely on implicit platform-specific behaviors, but stick to the strictest limitations among all platforms. As FreeBSD may corrupt TLS records if the application modifies the file during sendfile, applications using OpenSSL shouldn't modify the file if they care about this corruption. That means, it's safe to turn on the similar feature for Linux.

So, it's a breaking change only for the applications that are already broken (ones that modify the file during sendfile and don't want the corruption to happen, but when they are compiled on FreeBSD, they already suffer from it).

Having it off by default on Linux and always on on FreeBSD means introducing platform-specific behaviors exposed to the users of a cross-platform library, who expect it to behave the same way on all platforms.

I would assume that applications using a cross-platform library do not rely on implicit platform-specific behaviors,

I don't think this is a safe assumption. Although OpenSSL is a cross-platform library, not all users of OpenSSL care about its cross-platform capabilities.

As FreeBSD may corrupt TLS records if the application modifies the file during sendfile, applications using OpenSSL shouldn't modify the file if they care about this corruption.

Users on Linux are unlikely to be aware of this if their application does not run on FreeBSD.

That's not allowed in a minor release

What about merging it to the next major release? Is that allowed?

What about merging it to the next major release? Is that allowed?

In principle yes - but in practice there is no branch for it as yet. master is being used for 3.1.

We could choose to take this to OTC to determine whether this is sufficiently breaking to justify requiring that this is "off" by default.

mattcaswell · 2022-09-20T07:51:16Z

OTC Question: Should zero-copy sendfile be the default on Linux? This is already the default on FreeBSD but isn't currently on Linux - so there is an inconsistency. Changing this could theoretically break some Linux users

mattcaswell · 2022-09-20T09:50:17Z

OTC: We should not change the default for Linux in 3.x. We might reconsider for 4.0. This PR would be ok if it was an opt-in runtime configuration.

gal-pressman · 2022-11-06T09:45:44Z

@tmshort thanks for the review!
Is anything else needed to get this merged?

tmshort

Still not a fan of some of the documentation and s_server output.

I will say this, that just because it's done in a certain way in one place, doesn't mean it's necessarily the right thing to do, or the only way to do it.

The documentation is technically incorrect because the options are not really "required" if the option is automatically turned on. If that documentation is given to QA/DevTest, they will write up a bug on it.

In other words, the documentation and the behavior/message don't really match, and I'd like to see one or the other fixed, even the existing code.

tmshort · 2022-11-07T03:52:39Z

doc/man3/SSL_CONF_cmd.pod


+B<KTLSTxZerocopySendfile>: use the zerocopy TX mode of sendfile(), which gives
+a performance boost when used with kTLS hardware offload. Note that invalid TLS
+records might be transmitted if the file is changed while being sent. This


This still hasn't been addressed. I will also note that the phrasing below "This option only makes on Linux", is probably better as "This option only applies to Linux."

tmshort · 2022-11-07T04:00:45Z

In addition, @hlandau would have to approve, and @mattcaswell would have to re-approve, as changes were made after his initial approval.
EDIT: And there's a CI failure, which may or may not be relevant.

nvmmax · 2022-11-09T09:46:13Z

The documentation is technically incorrect because the options are not really "required" if the option is automatically turned on.

I don't seem to get it... Are you looking at the latest version? Where did you see the word "required"?

The documentation says:

This option depends on B<-sendfile>; when used alone, B<-sendfile> is implied,
and a warning is shown.

which matches the code:

    if (use_zc_sendfile && !use_sendfile) {
        BIO_printf(bio_out, "Warning: -zerocopy_sendfile depends on -sendfile, enabling -sendfile now.\n");
        use_sendfile = 1;
    }

What exactly looks "incorrect" to you? If I missed something elsewhere, please point me at it.

And there's a CI failure, which may or may not be relevant.

I don't think the CI failure on macOS is relevant for a Linux-specific change.

t8m · 2022-11-09T10:00:25Z

And there's a CI failure, which may or may not be relevant.

I don't think the CI failure on macOS is relevant for a Linux-specific change.

The MacOS buildbot worker is broken currently. So that failure is not relevant.

slontis · 2022-11-09T10:02:40Z

The MacOS buildbot worker is broken currently. So that failure is not relevant.

Is someone actually responsible for buildbot? It gets a bit annoying when its broken for this long.

include/openssl/bio.h.in

include/internal/bio.h

tmshort · 2022-11-09T13:52:31Z

doc/man1/openssl-s_server.pod.in

+=item B<-zerocopy_sendfile>
+
+If this option is set, SSL_sendfile() will use the zerocopy TX mode, which gives
+a performance boost when used with kTLS hardware offload. Note that invalid


I am not seeing "kTLS" (note capitalization) anywhere else in the documentation, except this PR.

Please change here, and elsewhere, to "Kernel TLS" or "KTLS" (all caps).

t8m · 2022-11-09T14:10:08Z

include/internal/bio.h

 # define BIO_FLAGS_KTLS_TX_CTRL_MSG 0x1000
 # define BIO_FLAGS_KTLS_RX          0x2000
 # define BIO_FLAGS_KTLS_TX          0x4000
+# define BIO_FLAGS_KTLS_TX_ZEROCOPY_SENDFILE 0x8000


This internal flag needs to be reflected (as comment) in the openssl/bio.h as well. Otherwise we risk reusing the flag value.

Modified the relevant comment.

TLS device offload allows to perform zerocopy sendfile transmissions. FreeBSD provides this feature by default, and Linux 5.19 introduced it as an opt-in. Zerocopy improves the TX rate significantly, but has a side effect: if the underlying file is changed while being transmitted, and a TCP retransmission happens, the receiver may get a TLS record containing both new and old data, which leads to an authentication failure and termination of connection. This effect is the reason Linux makes a copy on sendfile by default. This commit adds support for TLS zerocopy sendfile on Linux disabled by default to avoid any unlikely backward compatibility issues on Linux, although sacrificing consistency in OpenSSL's behavior on Linux and FreeBSD. A new option called KTLSTxZerocopySendfile is added to enable the new zerocopy behavior on Linux. This option should be used when the the application guarantees that the file is not modified during transmission, or it doesn't care about breaking the connection. The related documentation is also added in this commit. The unit test added doesn't test the actual functionality (it would require specific hardware and a non-local peer), but solely checks that it's possible to set the new option flag. Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com> Reviewed-by: Tariq Toukan <tariqt@nvidia.com> Reviewed-by: Boris Pismenny <borisp@nvidia.com>

tmshort

Fine by me

openssl-machine · 2022-11-24T12:00:29Z

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

t8m · 2022-11-24T12:20:24Z

Merged to master branch. Thank you for your contribution.

TLS device offload allows to perform zerocopy sendfile transmissions. FreeBSD provides this feature by default, and Linux 5.19 introduced it as an opt-in. Zerocopy improves the TX rate significantly, but has a side effect: if the underlying file is changed while being transmitted, and a TCP retransmission happens, the receiver may get a TLS record containing both new and old data, which leads to an authentication failure and termination of connection. This effect is the reason Linux makes a copy on sendfile by default. This commit adds support for TLS zerocopy sendfile on Linux disabled by default to avoid any unlikely backward compatibility issues on Linux, although sacrificing consistency in OpenSSL's behavior on Linux and FreeBSD. A new option called KTLSTxZerocopySendfile is added to enable the new zerocopy behavior on Linux. This option should be used when the the application guarantees that the file is not modified during transmission, or it doesn't care about breaking the connection. The related documentation is also added in this commit. The unit test added doesn't test the actual functionality (it would require specific hardware and a non-local peer), but solely checks that it's possible to set the new option flag. Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com> Reviewed-by: Tariq Toukan <tariqt@nvidia.com> Reviewed-by: Boris Pismenny <borisp@nvidia.com> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #18650)

TLS device offload allows to perform zerocopy sendfile transmissions. FreeBSD provides this feature by default, and Linux 5.19 introduced it as an opt-in. Zerocopy improves the TX rate significantly, but has a side effect: if the underlying file is changed while being transmitted, and a TCP retransmission happens, the receiver may get a TLS record containing both new and old data, which leads to an authentication failure and termination of connection. This effect is the reason Linux makes a copy on sendfile by default. This commit adds support for TLS zerocopy sendfile on Linux disabled by default to avoid any unlikely backward compatibility issues on Linux, although sacrificing consistency in OpenSSL's behavior on Linux and FreeBSD. A new option called KTLSTxZerocopySendfile is added to enable the new zerocopy behavior on Linux. This option should be used when the the application guarantees that the file is not modified during transmission, or it doesn't care about breaking the connection. The related documentation is also added in this commit. The unit test added doesn't test the actual functionality (it would require specific hardware and a non-local peer), but solely checks that it's possible to set the new option flag. Signed-off-by: Maxim Mikityanskiy <maximmi@nvidia.com> Reviewed-by: Tariq Toukan <tariqt@nvidia.com> Reviewed-by: Boris Pismenny <borisp@nvidia.com> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Todd Short <todd.short@me.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#18650)

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jun 24, 2022

t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Jun 24, 2022

nvmmax force-pushed the tls-zerocopy-sendfile-linux branch from 8ef18a9 to 7621af3 Compare August 19, 2022 09:33

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Aug 19, 2022

nvmmax force-pushed the tls-zerocopy-sendfile-linux branch from 7621af3 to d0747da Compare August 22, 2022 15:22

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

nvmmax force-pushed the tls-zerocopy-sendfile-linux branch from d0747da to d76d6ef Compare August 23, 2022 09:40

t8m added approval: review pending This pull request needs review by a committer approval: otc review pending labels Sep 14, 2022

hlandau suggested changes Sep 16, 2022

View reviewed changes

doc/man3/SSL_CONF_cmd.pod Outdated Show resolved Hide resolved

nvmmax force-pushed the tls-zerocopy-sendfile-linux branch from d76d6ef to 2f0bbf8 Compare September 16, 2022 14:44

mattcaswell requested changes Sep 16, 2022

View reviewed changes

mattcaswell added the hold: discussion The community needs to establish a consensus how to move forward with the issue or PR label Sep 20, 2022

This comment was marked as abuse.

Sign in to view

mattcaswell removed the hold: discussion The community needs to establish a consensus how to move forward with the issue or PR label Sep 20, 2022

nvmmax force-pushed the tls-zerocopy-sendfile-linux branch 3 times, most recently from 263d8d1 to b085b93 Compare September 20, 2022 13:37

nvmmax requested review from hlandau and removed request for hlandau September 23, 2022 13:21

nvmmax force-pushed the tls-zerocopy-sendfile-linux branch from d694ff2 to 41a2698 Compare October 15, 2022 12:01

nvmmax force-pushed the tls-zerocopy-sendfile-linux branch from 41a2698 to d8e095e Compare October 25, 2022 15:48

tmshort requested changes Nov 7, 2022

View reviewed changes

nvmmax force-pushed the tls-zerocopy-sendfile-linux branch from d8e095e to 1809693 Compare November 9, 2022 09:46

t8m requested changes Nov 9, 2022

View reviewed changes

include/openssl/bio.h.in Outdated Show resolved Hide resolved

include/internal/bio.h Outdated Show resolved Hide resolved

nvmmax force-pushed the tls-zerocopy-sendfile-linux branch from 1809693 to c406f68 Compare November 9, 2022 10:31

tmshort reviewed Nov 9, 2022

View reviewed changes

t8m reviewed Nov 9, 2022

View reviewed changes

nvmmax force-pushed the tls-zerocopy-sendfile-linux branch from c406f68 to 8c8b3e3 Compare November 23, 2022 11:06

nvmmax requested review from t8m and tmshort and removed request for tmshort November 23, 2022 11:07

t8m approved these changes Nov 23, 2022

View reviewed changes

t8m added the tests: present The PR has suitable tests present label Nov 23, 2022

t8m requested a review from mattcaswell November 23, 2022 11:20

mattcaswell approved these changes Nov 23, 2022

View reviewed changes

mattcaswell 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 Nov 23, 2022

tmshort approved these changes Nov 23, 2022

View reviewed changes

t8m closed this Nov 24, 2022

Uh oh!

Add support for kTLS zerocopy sendfile on Linux #18650

Add support for kTLS zerocopy sendfile on Linux #18650

Conversation

nvmmax commented Jun 24, 2022

Checklist

Uh oh!

openssl-machine commented Jul 25, 2022

Uh oh!

hlandau commented Jul 25, 2022

Uh oh!

nvmmax commented Jul 25, 2022

Uh oh!

nvmmax commented Aug 30, 2022

Uh oh!

mattcaswell commented Aug 30, 2022

Uh oh!

gal-pressman commented Sep 4, 2022

Uh oh!

ttoukan commented Sep 14, 2022

Uh oh!

hlandau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as abuse.

mattcaswell commented Sep 20, 2022

Uh oh!

gal-pressman commented Nov 6, 2022

Uh oh!

tmshort left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmshort commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvmmax commented Nov 9, 2022

Uh oh!

t8m commented Nov 9, 2022

Uh oh!

slontis commented Nov 9, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmshort left a comment

Choose a reason for hiding this comment

Uh oh!

openssl-machine commented Nov 24, 2022

Uh oh!

t8m commented Nov 24, 2022

Uh oh!

Reviewers

Assignees

Labels

mattcaswell commented Sep 20, 2022 •

edited

Loading

tmshort commented Nov 7, 2022 •

edited

Loading