Skip to content

Fix tls python offload with libxlio #349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Fix tls python offload with libxlio #349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6872781
issue: 4436329 Add getsockopt SOL_SOCKET SO_TYPE
BasharRadya May 7, 2025
a5891cd
issue: 4436329 Load libssl symbols from specific handle
BasharRadya May 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/core/sock/sockinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,29 @@ int sockinfo::getsockopt(int __level, int __optname, void *__optval, socklen_t *
errno = EINVAL;
}
break;
case SO_TYPE:
if (*__optlen != sizeof(int)) {
errno = EINVAL;
si_logdbg("(SO_TYPE) invalid length");
break;
}
switch (m_protocol) {
case PROTO_TCP:
*(int *)__optval = SOCK_STREAM;
ret = 0;
si_logdbg("(SO_TYPE) value: %d", SOCK_STREAM);
break;
case PROTO_UDP:
*(int *)__optval = SOCK_DGRAM;
ret = 0;
si_logdbg("(SO_TYPE) value: %d", SOCK_DGRAM);
break;
default:
errno = EINVAL;
si_logdbg("(SO_TYPE) invalid protocol");
break;
}
break;
case SO_MAX_PACING_RATE:
if (*__optlen == sizeof(struct xlio_rate_limit_t)) {
*(struct xlio_rate_limit_t *)__optval = m_so_ratelimit;
Expand Down
32 changes: 30 additions & 2 deletions src/core/sock/sockinfo_ulp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,34 @@ template <typename T> static void dlsym_default(T &ptr, const char *name)
dlsym_handle(ptr, name, RTLD_DEFAULT);
}

#define XLIO_TLS_API_FIND(__name) dlsym_default(s_tls_api.__name, #__name);
static void *openssl_handle = nullptr;

void xlio_tls_api_setup()
#define XLIO_TLS_API_FIND(__name) dlsym_handle(s_tls_api.__name, #__name, openssl_handle);

inline bool check_openssl_loaded()
{
openssl_handle = dlopen("libssl.so.3", RTLD_NOW | RTLD_GLOBAL);
Copy link
Collaborator

@AlexanderGrissik AlexanderGrissik May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the library can change in the future. at least it should be some changeable define somewhere

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;
}
}
Comment on lines +123 to +133
Copy link
Member

@pasis pasis May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


inline void xlio_tls_api_setup()
{
if (openssl_handle) {
Copy link
Collaborator

@AlexanderGrissik AlexanderGrissik May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@pasis pasis May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, the application can be statically linked.

// OpenSSL symbols is already loaded
return;
}

if (!check_openssl_loaded()) {
vlog_printf(VLOG_DEBUG, "OpenSSL library not found or failed to load");
return;
}

XLIO_TLS_API_FIND(EVP_CIPHER_CTX_new);
XLIO_TLS_API_FIND(EVP_CIPHER_CTX_free);
XLIO_TLS_API_FIND(EVP_CIPHER_CTX_reset);
Expand All @@ -132,6 +156,7 @@ void xlio_tls_api_setup()
XLIO_TLS_API_FIND(EVP_EncryptInit_ex);
XLIO_TLS_API_FIND(EVP_EncryptUpdate);
XLIO_TLS_API_FIND(EVP_EncryptFinal_ex);

if (s_tls_api.EVP_CIPHER_CTX_new && s_tls_api.EVP_CIPHER_CTX_free &&
s_tls_api.EVP_CIPHER_CTX_reset && s_tls_api.EVP_aes_128_gcm && s_tls_api.EVP_aes_256_gcm &&
s_tls_api.EVP_DecryptInit_ex && s_tls_api.EVP_DecryptUpdate &&
Expand Down Expand Up @@ -508,6 +533,9 @@ int sockinfo_tcp_ops_tls::setsockopt(int __level, int __optname, const void *__o
return -1;
}
} else {
#ifdef DEFINED_UTLS
Copy link
Collaborator

@AlexanderGrissik AlexanderGrissik May 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this ifdef this whole code is under this ifdef

Copy link
Collaborator

@AlexanderGrissik AlexanderGrissik May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we put it just before if (unlikely(!g_tls_api)) ?

xlio_tls_api_setup();
Copy link
Member

@pasis pasis May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

#endif /* DEFINED_UTLS */
/* RX offload checks. */
if (unlikely(!m_p_sock->is_utls_supported(UTLS_MODE_RX))) {
si_ulp_logdbg("TLS_RX is not supported.");
Expand Down