[PATCH v2] ubusd: convert tx_queue to linked list

[Felix Fietkau]

On 2021-03-24 13:27, Arnout Vandecappelle (Essensium/Mind) wrote:
> ubusd maintains a per-client tx_queue containing references to message
> buffers that have not been sent yet (due to the socket blocking). This
> is a fixed-size, 64-element queue.
> 
> When more than 64 elements are queued, subsequent elements are simply
> dropped. Thus, a client that is waiting for those messages will block
> indefinitely. In particular, this happens when more than +- 250 objects
> are registered on the bus and either "ubus list" or "ubus wait_for" is
> called. The responses to these requests consist of a message buffer per
> object. Since in practice, ubusd will not yield between the sends of
> these message buffers, the client has no time to process them and
> eventually the output socket blocks. After 64 more objects, the rest is
> dropped, including the final message that indicates termination. Thus,
> the client waits indefinitely for the termination message.
> 
> To solve this, turn the tx_queue into a variable-sized linked list
> instead of a fixed-size queue.
In order to reduce memory allocation overhead, I would propose the
following:

struct ubus_msg_backlog {
	struct ubus_msg_backlog *next;
	struct ubus_msg_buf *msg[UBUSD_CLIENT_BACKLOG];
	int tail;
};

To struct ubus_client add these:

	struct ubus_msg_backlog *backlog;
	int backlog_head;

After sending messages from tx_queue, you pull in messages from
cl->backlog, incrementing backlog_head.
Once cl->backlog_head == backlog->tail, you set cl->backlog to
backlog->next and free backlog.

> To maintain the linked list, an additional structure ubus_msg_buf_list
> is created. We could also have added the linked list to the ubus_msg_buf
> struct itself, but it is not relevant in the many other uses of the
> ubus_msg_buf struct so it would just complicate things.
Adding the linked list to ubus_msg_buf doesn't work, because a single
message can be queued for multiple receivers. This mistake was already
made by previous attempts at introducing a linked list for messages.

> The txq_off variable was used to keep track of which part of the current
> message was already written, in case only a partial write was possible.
> We take this opportunity to also move that variable under the
> ubus_msg_buf_list structure (and rename it to "offset", and change its
> type to size_t). This makes it clearer that it is related to that
> particular queued message and not to the queue as a whole.
> 
> Note that this infinite tx_queue opens the door to a DoS attack. You can
> open a client and a server connection, then send messages from the
> client to the server without ever reading anything on the server side.
> This will eventually lead to an out-of-memory. However, such a DoS
> already existed anyway, it just requires opening multiple server
> connections and filling up the fixed-size queue on each one. To protect
> against such DoS attacks, we'd need to:
I don't fully agree with your reasoning regarding the potential for DoS
attacks. It's true that the potential for *intentional* DoS attacks
exists in the current code already. What I'm worried about with this
patch is that you're adding extra potential for *accidental* DoS attacks
(i.e. excessive ubusd memory use for hanging processes receiving events).

> - keep a global maximum queue size that applies to all rx and tx queues
>   together;
> - stop reading from any connection when the maximum is reached;
> - close any connection when it hasn't become writeable after some
>   timeout.
I think we should have both a local and a global maximum queue size.
Other than that, I agree with the above.

- Felix