[PATCH] Add support for using sendfile when openssl support ktls

[Maxim Dounin]

Hello!

On Wed, Apr 10, 2019 at 02:45:52PM +0300, ben ben ishay wrote:

> # HG changeset patch
> # User ben ben ishay <benishay at mellanox.com>
> # Date 1554896607 -10800
> #      Wed Apr 10 14:43:27 2019 +0300
> # Node ID 87938decdb98bf4a06ed18002a15156a5e8fbd67
> # Parent  65074e13f1716e09c28d730586babad7930b7a98
> Add support for using sendfile when openssl support ktls
> 
> when we need to transfer data between file and socket we prefer to use sendfile instead of write because we save the copy to a buffer.
> the use of sendfile is possible in openssl only if it support ktls(the master of openssl support ktls) otherwise there is a copy of the data to userspace for encryption in any case (this paper explain this https://netdevconf.org/1.2/papers/ktls.pdf ).
> the patch  change the flow when the request is to send data over ssl and also the nginx use openssl that support ktls, the new flow using the sendfile function that tcp use for send data (ngx_linux_sendfile_chain).
> the performence with this patch applied was check with apib benchmark(https://github.com/apigee/apib), one machine run nginx and the other machine that connect back to back to the first one run apib with this comand: ./apib -c <num of connection> -d 30 https://<ip address>/<file name to send>.
> the file size was 100K.
> 
> the result display  in this table , each value represnt average throughput in GBps of 10 runs.
> 
> num of connection   | regular nginx  | new nginx
> 	1		5		5.2
> 	2		7.5		8.5
> 	3		7.7		9
> 
> this result prove that this patch increase nginx performance and thus is useful.

Thank you for your patch.  We've helped to develop similar 
functionality by Netflix for in-kernel TLS on FreeBSD (an earlier 
paper is referenced by the ktls.pdf you've linked).  See, for 
example, this post for a high-level description:

https://lists.freebsd.org/pipermail/freebsd-transport/2018-February/000196.html

The most obvious difference one can observe is that the 
application-level code instead uses SSL_sendfile() call as 
provided by the SSL library, and it is library responsibility to 
make sure keys are properly synced with the kernel when 
kernel-level functions are called.

In contrast, in your patch you assume that as long as 
BIO_get_ktls_send() returns true it is safe to use native kernel 
functions.  This looks unsafe, at least without a documentation 
which explicitly states otherwise, as various control messages 
might interfere with direct calls on the socket.  Moreover, quick 
look at the code seems to suggest that this is indeed might be 
unsafe - before writing anything to the socket OpenSSL checks if 
there are any pending control messages, and using sendfile() 
directly won't allow this to happen:

https://github.com/openssl/openssl/commit/6ba76c4f23e4b4ddc27b9e7234c8b9c3bcff5eff#diff-869032903e697780f95495f7e44410b1R127 

As such, the patch doesn't look correct to me (or at least 
OpenSSL's interface needs further clarification).

[...]

> @@ -140,3 +140,12 @@
>      fi
>  
>  fi
> +ngx_feature="OpenSSL library with KTLS"
> +ngx_feature_name="NGX_OPENSSL_KTLS"
> +ngx_feature_run=no
> +ngx_feature_incs="#include \"openssl/bio.h\" "
> +ngx_feature_path=
> +ngx_feature_libs="-lssl -lcrypto $NGX_LIBDL $NGX_LIBPTHREAD"
> +ngx_feature_test="BIO_get_ktls_send(NULL)"
> +. auto/feature
> +

Note that we don't really use configure-time feature tests for 
OpenSSL.  Instead, consider checking appropriate #define, such as 
#ifdef BIO_get_ktls_send.

> diff -r 65074e13f171 -r 87938decdb98 src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c	Tue Mar 26 09:33:57 2019 +0300
> +++ b/src/event/ngx_event_openssl.c	Wed Apr 10 14:43:27 2019 +0300
> @@ -1528,6 +1528,9 @@
>  #endif
>  
>      sc->connection = SSL_new(ssl->ctx);
> +#if (NGX_OPENSSL_KTLS)
> +    sc->ktls = 0;
> +#endif
>  
>      if (sc->connection == NULL) {
>          ngx_ssl_error(NGX_LOG_ALERT, c->log, 0, "SSL_new() failed");
> @@ -1639,6 +1642,12 @@
>          c->recv_chain = ngx_ssl_recv_chain;
>          c->send_chain = ngx_ssl_send_chain;
>  
> +#if (NGX_OPENSSL_KTLS)
> +       if(BIO_get_ktls_send(SSL_get_wbio(c->ssl->connection))){
> +           c->ssl->ktls = 1;
> +           c->send_chain = ngx_linux_sendfile_chain;
> +	}
> +#endif

Note that compiling this will fail on anything but Linux as long 
as BIO_get_ktls_send() is present in the OpenSSL library, as 
ngx_linux_sendfile_chain() is only available on Linux.

[...]

-- 
Maxim Dounin
http://mdounin.ru/