-
Notifications
You must be signed in to change notification settings - Fork 111
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.
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
Unix sockets support #1436
Unix sockets support #1436
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1436 +/- ##
==========================================
+ Coverage 80.88% 81.01% +0.13%
==========================================
Files 149 149
Lines 15117 15130 +13
==========================================
+ Hits 12227 12258 +31
+ Misses 2290 2278 -12
+ Partials 600 594 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! Good stuff! LGTM 🥇
bpf/k_unix_sock.h
Outdated
struct sock *sk; | ||
BPF_CORE_READ_INTO(&sk, sock, sk); | ||
|
||
if (!sk) { | ||
return 0; | ||
} | ||
|
||
struct unix_sock *usock; | ||
BPF_CORE_READ_INTO(&usock, sock, sk); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-rhetorical question: why is the double read required? Would something like the following not work?
const struct unix_sock *usock = BPF_CORE_READ(sock, sk);
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks for catching it, it's a left over from some earlier changes.
bpf/k_unix_sock.h
Outdated
|
||
u8 *buf = iovec_memory(); | ||
if (buf) { | ||
copied_len = read_iovec_ctx(iov_ctx, buf, copied_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd not reuse the variable here, I found it a bit confusing - for instance, it begs the question: after a successful the call to read_iovec_ctx, should the new copied len match the older one? If not, why are they expected to be different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can split it in another variable, but iovecs read with a loop and it's limited in how many iterations we do, so we may not read the full buffer.
bpf/k_unix_sock.h
Outdated
unsigned long peer_inode_number = 0; | ||
BPF_CORE_READ_INTO(&inode_number, sock, sk, sk_socket, file, f_inode, i_ino); | ||
|
||
struct unix_sock *usock = unix_sock_from_socket(sock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case, is this equivalent to:
struct unix_sock *usock = unix_sock_from_sk(sk);
?
If so, then that will save us a few extra reads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, another code iteration I guess, we don't need to re-read it.
This PR adds support for running TCP and HTTP connections over unix sockets. These are different than the regular TCP sockets because they run streams across mapped files on disk. The setup is always on the same host, so the context propagation can be done with our black box context propagation.
Couple of things related to this addition:
struct sock
pointers tostruct unix_sock
, which allows us to see the peer socket.TODO: