On 1/31/25 11:48 PM, John Ousterhout wrote:
On Thu, Jan 30, 2025 at 1:39 AM Paolo Abeni [off-list ref] wrote:
quoted
On 1/30/25 1:41 AM, John Ousterhout wrote:
quoted
On Fri, Jan 24, 2025 at 12:31 AM Paolo Abeni [off-list ref] wrote:
quoted
OoO will cause additional allocation? this feels like DoS prone.
quoted
+ }
+ rpc->msgin.recv_end = end;
+ goto keep;
+ }
+
+ /* Must now check to see if the packet fills in part or all of
+ * an existing gap.
+ */
+ list_for_each_entry_safe(gap, dummy, &rpc->msgin.gaps, links) {
Linear search for OoO has proven to be subject to serious dos issue. You
should instead use a (rb-)tree to handle OoO packets.
I have been assuming that DoS won't be a major issue for Homa because
it's intended for use only in datacenters (if there are antagonistic
parties, they will be isolated from each other by networking
hardware). Is this a bad assumption?
I think assuming that the peer will always behave is dangerous. The peer
could be buggy or compromised, transient network condition may arise.
Even un-malicious users tend to do the most crazy and unexpected things
given enough time.
Also the disclaimer "please don't use this in on an internet facing
host" sounds quite bad for a networking protocol ;)
I don't see why this disclaimer would be needed: as long as the Homa
hosts are inside the firewall, the firewall will prevent any external
Homa packets from reaching them. And, if your host is compromised, DoS
is the least of your worries.
It seems to me that this is mostly about helping people debug. I agree
that that could be useful. However, it's hard for me to imagine this
particular situation (lots of gaps in the packet stream) happening by
accident. Applications won't be generating Homa packets themselves,
they will be using Homa, and Homa won't generate lots of gaps. There
are other kinds of bad application behavior that are much more likely
to occur, such as an app that gets into an infinite loop sending
requests without ever receiving responses.
And, an rb-tree will add complexity and slow down the common case
(trees have better O(...) behavior than lists but worse constant
factors).
My point is that a service (and the host running it) is supposed to
survive any kind of unexpected conditions, as they will happen for sure
in any internet-facing host, and will likely happen also in the most
controlled environment due to bugs or unexpected events somewhere(else).
quoted
quoted
quoted
Also it looks like there is no memory accounting at all, and SO_RCVBUF
setting are just ignored.
Homa doesn't yet have comprehensive memory accounting, but there is a
limit on buffer space for incoming messages. Instead of SO_RCVBUF,
applications control the amount of receive buffer space by controlling
the size of the buffer pool they provide to Homa with the
SO_HOMA_RCVBUF socket option.
Ignoring SO_RCVBUF (and net.core.rmem_* sysctls) is both unexpected and
dangerous (a single application may consume unbounded amount of system
memory). Also what about the TX side? I don't see any limit at all there.
An application cannot consume unbounded system memory on the RX side
(in fact it consumes almost none). When packets arrive, their data is
immediately transferred to a buffer region in user memory provided by
the application (using the facilities in homa_pool.c). Skb's are
occupied only long enough to make this transfer, and it happens even
if there is no pending recv* kernel call. The size of the buffer
region is limited by the application, and the application must provide
a region via SO_HOMA_RCVBUF.
I don't see where/how the SO_HOMA_RCVBUF max value is somehow bounded?!?
It looks like the user-space could pick an arbitrary large value for it.
Given this, there's no need for SO_RCVBUF
(and I don't see why a different limit would be specified via
SO_RCVBUF than the one already provided via SO_HOMA_RCVBUF).
I agree that this is different from TCP, but Homa is different from TCP in
lots of ways.
There is currently no accounting or control on the TX side. I agree
that this needs to be implemented at some point, but if possible I'd
prefer to defer this until more of Homa has been upstreamed. For
example, this current patch doesn't include any sysctl support, which
would be needed as part of accounting/control (the support is part of
the GitHub repo, it's just not in this patch series).
SO_RCVBUF and SO_SNDBUF are expected to apply to any kind of socket,
see man 7 sockets. Exceptions should be at least documented, but we need
some way to limit memory usage in both directions.
Fine tuning controls and sysctls could land later, but the basic
constraints should IMHO be there from the beginning.
quoted
quoted
quoted
quoted
+/**
+ * homa_dispatch_pkts() - Top-level function that processes a batch of packets,
+ * all related to the same RPC.
+ * @skb: First packet in the batch, linked through skb->next.
+ * @homa: Overall information about the Homa transport.
+ */
+void homa_dispatch_pkts(struct sk_buff *skb, struct homa *homa)
I see I haven't mentioned the following so far, but you should move the
struct homa to a pernet subsystem.
Sorry for my ignorance, but I'm not familiar with the concept of "a
pernet subsystem". What's the best way for me to learn more about
this?
Have a look at register_pernet_subsys(), struct pernet_operations and
some basic usage example, i.e. in net/8021q/vlan.c.
register_pernet_subsys() allow registering/allocating a per network
namespace structure of specified size (pernet_operations.size) that that
subsystem can use according to its own need fetching it from the netns
via the `id` obtained at registration time.
I will take a look.
quoted
quoted
quoted
quoted
+ /* Find and lock the RPC if we haven't already done so. */
+ if (!rpc) {
+ if (!homa_is_client(id)) {
+ /* We are the server for this RPC. */
+ if (h->common.type == DATA) {
+ int created;
+
+ /* Create a new RPC if one doesn't
+ * already exist.
+ */
+ rpc = homa_rpc_new_server(hsk, &saddr,
+ h, &created);
It looks like a buggy or malicious client could force server RPC
allocation to any _client_ ?!?
I'm not sure what you mean by "force server RPC allocation to any
_client_"; can you give a bit more detail?
AFAICS the home protocol uses only the `id` provided by the sender to
discriminate the incoming packet as a client requests and thus
allocating resources (RPC new server) on the receiver.
Suppose an host creates a client socket, and a port is assigned to it.
A malicious or buggy peer starts sending an (unlimited amount of)
uncompleted homa RPC request to it.
AFAICS the host A will allocate server RPCs in response to such incoming
packets, which is unexpected to me.
Now I see what you're getting at. Homa sockets are symmetric: any
socket can be used for both the client and server sides of RPCs. Thus
it's possible to send requests even to sockets that haven't been
"bound". I think of this as a feature, not a bug (it can potentially
reduce the need to allocate "known" port numbers). At the same time, I
see your point that some applications might not expect to receive
requests. Would you like a mechanism to disable this? For example,
sockets could be configured by default to reject incoming requests;
invoking the "bind" system call would enable incoming requests (I
would also add a setsockopt mechanism for enabling requests even on
"unbound" sockets).
I think that an explicit setsockopt() to enable incoming requests should
be fine.
quoted
Additionally AFAICS each RPC is identified only by dport/id and both
port and id allocation is sequential it looks like it's quite easy to
spoof/inject data in a different RPC - even "by mistake". I guess this
is a protocol limit.
On the server side an RPC is identified by <client address, dport,
id>, but on the client side only by <dport, id> (the sender address
isn't needed to lookup the correct RPC). However, it would be easy to
check incoming packets to make sure that the sender address matches
the sender in the RPC. I will do that.
I somewhat missed the src address matching for the server side. I think
it would be good if the lookup could be symmetric for both the client
and the server.
quoted
quoted
The new methods also provide a consistent and simple solution to
several other problems that had been solved in an ad hoc way.
It would be even better if a function never had to release a lock
internally, but so far I haven't figured out how to do that. If you
have ideas I'd like to hear them.
In some cases it could be possible to move the unlock in the caller,
eventually breaking the relevant function in smaller helpers.
quoted
quoted
quoted
+
+ if (id != 0) {
+ if ((atomic_read(&rpc->flags) & RPC_PKTS_READY) || rpc->error)
+ goto claim_rpc;
+ rpc->interest = interest;
+ interest->reg_rpc = rpc;
+ homa_rpc_unlock(rpc);
With the current schema you should release the hsh socket lock before
releasing the rpc one.
Normally I would agree, but that won't work here: the RPC is no longer
of interest, so it needs to be unlocked,
Is that unlock strictly necessary (would cause a deadlock if omitted) or
just an optimization?
That RPC definitely needs to be unlocked (another RPC gets locked
later in the function).
Side note: if you use per RPC lock, and you know that the later one is a
_different_ RPC, there will be no need for unlocking (and LOCKDEP will
be happy with a "_nested" annotation).
It would be possible to defer its unlocking
until after the socket is unlocked, but that would be awkward: that
RPC is never used again, and there would need to be an extra variable
to remember the RPC for later unlocking; the later unlocking would
drop in "out of the blue" (readers would wonder "why is this RPC being
unlocked here?"). And it would keep an RPC locked unnecessarily.
I guess it's a matter of taste and personal preferences. IMHO
inconsistent unlock chain is harder to follow. I also think a comment is
due in either cases.
Cheers,
Paolo