Fix tls python offload with libxlio #349
Conversation
|
bot:retest |
Added getsockopt SO_TYPE option in SOL_SOCKET Signed-off-by: Bashar Abdelgafer <[email protected]>
Load libssl symbols from speicific handle. Signed-off-by: Bashar Abdelgafer <[email protected]>
BasharRadya
force-pushed
the
fix_tls_python
branch
from
9776aa9 to
a5891cd
Compare
|
bot:retest |
|
Hi @pasis , can you review? please give priority its for customer |
| @@ -508,6 +533,9 @@ int sockinfo_tcp_ops_tls::setsockopt(int __level, int __optname, const void *__o | |||
| return -1; | |||
| } | |||
| } else { | |||
| #ifdef DEFINED_UTLS | |||
There was a problem hiding this comment.
No need for this ifdef this whole code is under this ifdef
There was a problem hiding this comment.
should we put it just before if (unlikely(!g_tls_api)) ?
|
|
||
| inline bool check_openssl_loaded() | ||
| { | ||
| openssl_handle = dlopen("libssl.so.3", RTLD_NOW | RTLD_GLOBAL); |
There was a problem hiding this comment.
The name of the library can change in the future. at least it should be some changeable define somewhere
| { | ||
| if (openssl_handle) { |
There was a problem hiding this comment.
I suggest trying the symbols first the old way without openssl_handle as this is a more generic way and does not require loading a specific lib file. And only if it fails then to fallback to dlopen
There was a problem hiding this comment.
Agree, the application can be statically linked.
|
The changes are OK to be provided as temporary patch |
| inline bool check_openssl_loaded() | ||
| { | ||
| openssl_handle = dlopen("libssl.so.3", RTLD_NOW | RTLD_GLOBAL); | ||
| if (!openssl_handle) { | ||
| vlog_printf(VLOG_DEBUG, "Failed to dlopen libssl.so.3: %s", dlerror()); | ||
| return false; | ||
| } else { | ||
| vlog_printf(VLOG_DEBUG, "Successfully loaded libssl.so.3"); | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider using dl_iterate_phdr(). It's not standardized and appeared in glibc-2.2.4. I'm not sure you can obtain the handle object to use in dlsym() (don't see a way to obtain the handle from dladdr() and address from the iterator), but at least you can find the libssl exact name (path?). Note that not only the lib name can change over time, but also libssl can be loaded from non-standard paths.
| { | ||
| if (openssl_handle) { |
There was a problem hiding this comment.
Agree, the application can be statically linked.
| @@ -508,6 +533,9 @@ int sockinfo_tcp_ops_tls::setsockopt(int __level, int __optname, const void *__o | |||
| return -1; | |||
| } | |||
| } else { | |||
| #ifdef DEFINED_UTLS | |||
| xlio_tls_api_setup(); | |||
There was a problem hiding this comment.
Current implementation is not thread safe. Such a runtime initialization needs to be protected. consider some kind of thread safe singleton assuming that a pointer assignment is an atomic operation on the target platforms (no need to lock the check for openssl API initialization)
|
bot:retest |
1 similar comment
|
bot:retest |
Description
Please provide a summary of the change.
What
Subject: what this PR is doing in one line.
Why ?
Justification for the PR. If there is existing issue/bug please reference.
How ?
It is optional but for complex PRs please provide information about the design,
architecture, approach, etc.
Change type
What kind of change does this PR introduce?
Check list