Re: [PATCH v2] ubusd: convert tx_queue to linked list
On 2021-03-25 21:43, Arnout Vandecappelle wrote:
>
>
> On 25/03/2021 10:48, Felix Fietkau wrote:
>>
>> 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;
>> };
>
> This saves space by open-coding a single-linked list rather than using the
> normal double-linked list. This comes at the cost of iterating over the entire
> list in order to append to it.
>
> It also saves space by dropping the offset member. But for that, we don't
> need
> to go to this extra structure.
>
> Applying those to "optimisations" to struct ubus_msg_buf_list would make that
> one 8 bytes (compared to 16 now). So that's 8 bytes per queued buffer, in
> addition to the 24 bytes of struct ubus_msg_buf and the actual buffer itself,
> which you can expect to be hundreds of bytes.
>
> struct ubus_msg_backlog is 24 bytes (assuming UBUSD_CLIENT_BACKLOG is 4),
> that's 6 bytes per element. So not much gained here. Increasing
> UBUSD_CLIENT_BACKLOG will decrease the overhead for large backlog, but gives a
> much bigger overhead for smaller backlog. So not a clear win.
UBUSD_CLIENT_BACKLOG is currently 32. I'm not so much counting bytes for
the optimization, I'm more interested in reducing the number of memory
allocations. This helps reduce memory allocator overhead and fragmentation.
> Finally, the backlog should normally be empty. It's pretty unusual for a ubus
> message to take more then one write to transfer.
>
> In conclusion:
> - I think your proposal doesn't save much;
> - and it complicates things quite a bit;
> - in addition, the single-linked list potentially poses significant time
> overhead.
With a reasonable upper limit for the maximum number of messages and the
current UBUSD_CLIENT_BACKLOG of 32, the time overhead for walking the
pointers is actually insignificant. If it ever becomes significant, we
can simply add a tail pointer to struct ubus_client and still save space.
I also expect it to be less than the malloc overhead for lots of
single-message list entries.
> If you want to save a few bytes, it does make sense to move the offset back
> to
> struct ubus_client.
>
> If you *really* want to save space, and time as well, it would be better to
> optimise ubusd_handle_lookup. That currently sends a separate, relatively
> small,
> message for every object. The overhead of the struct ubus_msg_buf dwarfs the
> overhead of struct ubus_msg_buf_list by quite a lot, and in addition there's
> overhead on the wire as well. It shouldn't be too hard to change
> ubus_lookup_cb
> to handle a list rather than a single object.
>
> Maybe I should have gone down that path. I hadn't thought of it at the time.
I think that's a good idea.
>>> 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*
[PATCH v3 1/2] ubusd: convert tx_queue to linked list
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.
To maintain the linked list, an additional structure ubus_msg_buf_list
is created. It is not possible to add the linked list to ubus_msg_buf,
because that is shared between clients.
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:
- 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.
Fixes: https://bugs.openwrt.org/index.php?do=details_id=1525
Signed-off-by: Arnout Vandecappelle (Essensium/Mind)
---
v3:
- remove the txq_ofs change
- reduce the diff a bit by adding "ub = ubl->msg;"
v2: workarounds for clang static analysis false positives:
- use list_for_each_safe instead of a while loop;
- hide the free() by moving it to ubusd.c.
---
ubusd.c | 20
ubusd.h | 11 ---
ubusd_main.c | 33 +
ubusd_proto.c | 1 +
4 files changed, 34 insertions(+), 31 deletions(-)
diff --git a/ubusd.c b/ubusd.c
index 5993653..c324c70 100644
--- a/ubusd.c
+++ b/ubusd.c
@@ -133,13 +133,25 @@ ssize_t ubus_msg_writev(int fd, struct ubus_msg_buf *ub,
size_t offset)
return ret;
}
+void ubus_msg_list_free(struct ubus_msg_buf_list *ubl)
+{
+ list_del_init(>list);
+ ubus_msg_free(ubl->msg);
+ free(ubl);
+}
+
static void ubus_msg_enqueue(struct ubus_client *cl, struct ubus_msg_buf *ub)
{
- if (cl->tx_queue[cl->txq_tail])
+ struct ubus_msg_buf_list *ubl;
+
+ ubl = calloc(1, sizeof(struct ubus_msg_buf_list));
+ if (!ubl)
return;
- cl->tx_queue[cl->txq_tail] = ubus_msg_ref(ub);
- cl->txq_tail = (cl->txq_tail + 1) % ARRAY_SIZE(cl->tx_queue);
+ INIT_LIST_HEAD(>list);
+ ubl->msg = ubus_msg_ref(ub);
+
+ list_add_tail(>tx_queue, >list);
}
/* takes the msgbuf reference */
@@ -150,7 +162,7 @@ void ubus_msg_send(struct ubus_client *cl, struct
ubus_msg_buf *ub)
if (ub->hdr.type != UBUS_MSG_MONITOR)
ubusd_monitor_message(cl, ub, true);
- if (!cl->tx_queue[cl->txq_cur]) {
+ if (list_empty(>tx_queue)) {
written = ubus_msg_writev(cl->sock.fd, ub, 0);
if (written < 0)
diff --git a/ubusd.h b/ubusd.h
index 923e43d..f34cba1 100644
--- a/ubusd.h
+++ b/ubusd.h
@@ -23,7 +23,6 @@
#include "ubusmsg.h"
#include "ubusd_acl.h"
-#define UBUSD_CLIENT_BACKLOG 32
#define UBUS_OBJ_HASH_BITS 4
extern struct blob_buf b;
@@ -36,6 +35,11 @@ struct ubus_msg_buf {
int len;
};
+struct ubus_msg_buf_list {
+ struct list_head list;
+ struct ubus_msg_buf *msg;
+};
+
struct ubus_client {
struct ubus_id id;
struct uloop_fd sock;
@@ -48,8 +52,8 @@ struct ubus_client {
struct list_head objects;
- struct ubus_msg_buf *tx_queue[UBUSD_CLIENT_BACKLOG];
- unsigned int txq_cur, txq_tail, txq_ofs;
+ struct list_head tx_queue;
+ unsigned int txq_ofs;
struct ubus_msg_buf *pending_msg;
struct ubus_msg_buf *retmsg;
@@ -72,6 +76,7 @@ struct ubus_msg_buf *ubus_msg_new(void *data, int len, bool
shared);
void ubus_msg_send(struct ubus_client *cl, struct ubus_msg_buf *ub);
ssize_t ubus_msg_writev(int fd, struct ubus_msg_buf *ub, size_t offset);
void ubus_msg_free(struct ubus_msg_buf *ub);
+void ubus_msg_list_free(struct ubus_msg_buf_list *ubl);
struct blob_attr **ubus_parse_msg(struct blob_attr *msg, size_t len);
struct ubus_client *ubusd_proto_new_client(int fd, uloop_fd_handler cb);
diff --git a/ubusd_main.c
[PATCH v3 2/2] ubusd: add per-client tx queue limit
No new message can be enqueued if this brings the total queue length of
that client over UBUS_CLIENT_MAX_TXQ_LEN.
Set UBUS_CLIENT_MAX_TXQ_LEN to UBUS_MAX_MSGLEN, i.e. 1MB. This limit
should be plenty for any practical use case.
Signed-off-by: Arnout Vandecappelle (Essensium/Mind)
---
v3: new patch
I've tested this patch by creating about 40K objects on the bus, which
should be more than enough to reach the 1MB limit. And indeed, if I do
"ubus list | wc", I get 40K lines, but if I do "ubus list | less", it
stops at around 1000 lines (1K per object, is that realistic? Feels a
bit high to me...).
---
ubusd.c | 5 +
ubusd.h | 2 ++
ubusd_main.c | 1 +
3 files changed, 8 insertions(+)
diff --git a/ubusd.c b/ubusd.c
index c324c70..0e1b0c9 100644
--- a/ubusd.c
+++ b/ubusd.c
@@ -144,6 +144,9 @@ static void ubus_msg_enqueue(struct ubus_client *cl, struct
ubus_msg_buf *ub)
{
struct ubus_msg_buf_list *ubl;
+ if (cl->txq_len + ub->len > UBUS_CLIENT_MAX_TXQ_LEN)
+ return;
+
ubl = calloc(1, sizeof(struct ubus_msg_buf_list));
if (!ubl)
return;
@@ -152,6 +155,7 @@ static void ubus_msg_enqueue(struct ubus_client *cl, struct
ubus_msg_buf *ub)
ubl->msg = ubus_msg_ref(ub);
list_add_tail(>tx_queue, >list);
+ cl->txq_len += ub->len;
}
/* takes the msgbuf reference */
@@ -172,6 +176,7 @@ void ubus_msg_send(struct ubus_client *cl, struct
ubus_msg_buf *ub)
return;
cl->txq_ofs = written;
+ cl->txq_len = -written;
/* get an event once we can write to the socket again */
uloop_fd_add(>sock, ULOOP_READ | ULOOP_WRITE |
ULOOP_EDGE_TRIGGER);
diff --git a/ubusd.h b/ubusd.h
index f34cba1..c5d6d2a 100644
--- a/ubusd.h
+++ b/ubusd.h
@@ -24,6 +24,7 @@
#include "ubusd_acl.h"
#define UBUS_OBJ_HASH_BITS 4
+#define UBUS_CLIENT_MAX_TXQ_LENUBUS_MAX_MSGLEN
extern struct blob_buf b;
@@ -54,6 +55,7 @@ struct ubus_client {
struct list_head tx_queue;
unsigned int txq_ofs;
+ unsigned int txq_len;
struct ubus_msg_buf *pending_msg;
struct ubus_msg_buf *retmsg;
diff --git a/ubusd_main.c b/ubusd_main.c
index 3728a42..d298b51 100644
--- a/ubusd_main.c
+++ b/ubusd_main.c
@@ -74,6 +74,7 @@ static void client_cb(struct uloop_fd *sock, unsigned int
events)
}
cl->txq_ofs += written;
+ cl->txq_len -= written;
if (cl->txq_ofs < ub->len + sizeof(ub->hdr))
break;
--
2.30.2
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH] ubusd: convert tx_queue to linked list
On 25/03/2021 10:09, Sergey Zakharchenko wrote: > Hi Arnout, > > JFYI: Much of your patch resembles > https://patchwork.ozlabs.org/project/lede/patch/20180503100622.11168-1-i@qbox.audio/ > which was marked superseded for no apparent reason, but "anonymous > remembers":). It's delegated to John, so maybe he intended to rework and apply it but never got around to it. > As for the limits, one of them could include a timeout (e.g. no > progress in writing to client for over 10s => ubusd closes > connection). Oh yes, just what we need, more complexity :-) I agree though that timeout on Tx would make sense. It doesn't help for DoS attacks, but it does help to protect against a frozen client. Regards, Arnout ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH v2] ubusd: convert tx_queue to linked list
On 25/03/2021 10:48, Felix Fietkau wrote:
>
> 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;
> };
This saves space by open-coding a single-linked list rather than using the
normal double-linked list. This comes at the cost of iterating over the entire
list in order to append to it.
It also saves space by dropping the offset member. But for that, we don't need
to go to this extra structure.
Applying those to "optimisations" to struct ubus_msg_buf_list would make that
one 8 bytes (compared to 16 now). So that's 8 bytes per queued buffer, in
addition to the 24 bytes of struct ubus_msg_buf and the actual buffer itself,
which you can expect to be hundreds of bytes.
struct ubus_msg_backlog is 24 bytes (assuming UBUSD_CLIENT_BACKLOG is 4),
that's 6 bytes per element. So not much gained here. Increasing
UBUSD_CLIENT_BACKLOG will decrease the overhead for large backlog, but gives a
much bigger overhead for smaller backlog. So not a clear win.
Finally, the backlog should normally be empty. It's pretty unusual for a ubus
message to take more then one write to transfer.
In conclusion:
- I think your proposal doesn't save much;
- and it complicates things quite a bit;
- in addition, the single-linked list potentially poses significant time
overhead.
If you want to save a few bytes, it does make sense to move the offset back to
struct ubus_client.
If you *really* want to save space, and time as well, it would be better to
optimise ubusd_handle_lookup. That currently sends a separate, relatively small,
message for every object. The overhead of the struct ubus_msg_buf dwarfs the
overhead of struct ubus_msg_buf_list by quite a lot, and in addition there's
overhead on the wire as well. It shouldn't be too hard to change ubus_lookup_cb
to handle a list rather than a single object.
Maybe I should have gone down that path. I hadn't thought of it at the time.
>
> 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.
Right, ubus_msg_free doesn't actually free it, it decreases the refcount. I
forgot that. Good thing I didn't implement it that way then :-)
>> 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
[PATCH] build,json: 3rd fixup of default_packages
This became a bit of a tragedy, caused by a corner cases which wasn't
put into account during testing. DEFAULT_PACKAGES are defined in
target/linux//Makefile but also in
target/linux///target.mk.
The latter was no longer imported when using DUMP=1, however not using
DUMP=1 while running the Makefile in target/linux// caused duplicate
packages in the list.
As a solution, which should have been used from day 0, `make` runs in
target/linux/ without DUMP=1, resulting in no duplicate packages and all
inclusions from include/target.mk, linux/target//{Makefile,
/target.mk}
While at it, sort the list of default packages.
Signed-off-by: Paul Spooren
---
scripts/json_overview_image_info.py | 23 +--
1 file changed, 5 insertions(+), 18 deletions(-)
diff --git a/scripts/json_overview_image_info.py
b/scripts/json_overview_image_info.py
index cd814a19c0..8dbd24af2d 100755
--- a/scripts/json_overview_image_info.py
+++ b/scripts/json_overview_image_info.py
@@ -33,28 +33,13 @@ for json_file in work_dir.glob("*.json"):
)
if output:
-output["default_packages"] = run(
+default_packages, output["arch_packages"] = run(
[
"make",
"--no-print-directory",
"-C",
-"target/linux/{}".format(output["target"].split("/")[0]),
+"target/linux/",
"val.DEFAULT_PACKAGES",
-"DUMP=1",
-],
-stdout=PIPE,
-stderr=PIPE,
-check=True,
-env=environ.copy().update({"TOPDIR": Path().cwd()}),
-universal_newlines=True,
-).stdout.split()
-
-output["arch_packages"] = run(
-[
-"make",
-"--no-print-directory",
-"-C",
-"target/linux/{}".format(output["target"].split("/")[0]),
"val.ARCH_PACKAGES",
],
stdout=PIPE,
@@ -62,7 +47,9 @@ if output:
check=True,
env=environ.copy().update({"TOPDIR": Path().cwd()}),
universal_newlines=True,
-).stdout.strip()
+).stdout.splitlines()
+
+output["default_packages"] = sorted(default_packages.split())
output_path.write_text(json.dumps(output, sort_keys=True, separators=(",",
":")))
else:
--
2.30.2
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
[PATCH 2/2] kernel: Move CONFIG_USERIO to generic config
The CONFIG_USERIO option is unset in multiple target configurations. On the sunxi target it is activated. Move the kernel configuration option to the generic kernel configuration. Signed-off-by: Hauke Mehrtens --- target/linux/gemini/config-5.4 | 1 - target/linux/generic/config-5.10 | 1 + target/linux/generic/config-5.4 | 1 + target/linux/layerscape/armv8_64b/config-5.4 | 1 - target/linux/malta/config-5.10 | 1 - target/linux/malta/config-5.4| 1 - target/linux/omap/config-5.4 | 1 - target/linux/oxnas/config-5.10 | 1 - target/linux/oxnas/config-5.4| 1 - target/linux/rockchip/armv8/config-5.10 | 1 - target/linux/rockchip/armv8/config-5.4 | 1 - target/linux/tegra/config-5.4| 1 - target/linux/x86/config-5.10 | 1 - target/linux/x86/config-5.4 | 1 - target/linux/zynq/config-5.4 | 1 - 15 files changed, 2 insertions(+), 13 deletions(-) diff --git a/target/linux/gemini/config-5.4 b/target/linux/gemini/config-5.4 index 7cfdb3ccdbd0..c98f66148e0b 100644 --- a/target/linux/gemini/config-5.4 +++ b/target/linux/gemini/config-5.4 @@ -441,7 +441,6 @@ CONFIG_UNCOMPRESS_INCLUDE="debug/uncompress.h" CONFIG_UNINLINE_SPIN_UNLOCK=y CONFIG_UNWINDER_ARM=y CONFIG_USB_SUPPORT=y -# CONFIG_USERIO is not set CONFIG_USER_NS=y CONFIG_USE_OF=y CONFIG_UTS_NS=y diff --git a/target/linux/generic/config-5.10 b/target/linux/generic/config-5.10 index 3bf985dc4c71..457f62b195c0 100644 --- a/target/linux/generic/config-5.10 +++ b/target/linux/generic/config-5.10 @@ -6688,6 +6688,7 @@ CONFIG_USB_VIDEO_CLASS_INPUT_EVDEV=y # CONFIG_USB_ZR364XX is not set # CONFIG_USELIB is not set # CONFIG_USERFAULTFD is not set +# CONFIG_USERIO is not set # CONFIG_USE_OF is not set # CONFIG_UTS_NS is not set # CONFIG_UWB is not set diff --git a/target/linux/generic/config-5.4 b/target/linux/generic/config-5.4 index 1dc97ce74598..baa26221c771 100644 --- a/target/linux/generic/config-5.4 +++ b/target/linux/generic/config-5.4 @@ -6242,6 +6242,7 @@ CONFIG_USB_VIDEO_CLASS_INPUT_EVDEV=y # CONFIG_USB_ZR364XX is not set # CONFIG_USELIB is not set # CONFIG_USERFAULTFD is not set +# CONFIG_USERIO is not set # CONFIG_USE_OF is not set # CONFIG_UTS_NS is not set # CONFIG_UWB is not set diff --git a/target/linux/layerscape/armv8_64b/config-5.4 b/target/linux/layerscape/armv8_64b/config-5.4 index 172a9d43b107..304d40da949f 100644 --- a/target/linux/layerscape/armv8_64b/config-5.4 +++ b/target/linux/layerscape/armv8_64b/config-5.4 @@ -925,7 +925,6 @@ CONFIG_UNINLINE_SPIN_UNLOCK=y CONFIG_UNIX_DIAG=y CONFIG_UNMAP_KERNEL_AT_EL0=y CONFIG_USB_SUPPORT=y -# CONFIG_USERIO is not set CONFIG_USER_NS=y CONFIG_USE_PERCPU_NUMA_NODE_ID=y CONFIG_UTS_NS=y diff --git a/target/linux/malta/config-5.10 b/target/linux/malta/config-5.10 index 65e82eef8c6a..b32052ada2cb 100644 --- a/target/linux/malta/config-5.10 +++ b/target/linux/malta/config-5.10 @@ -275,7 +275,6 @@ CONFIG_TIMER_PROBE=y CONFIG_TREE_RCU=y CONFIG_TREE_SRCU=y CONFIG_USB_SUPPORT=y -# CONFIG_USERIO is not set CONFIG_USE_OF=y # CONFIG_VGA_CONSOLE is not set CONFIG_VM_EVENT_COUNTERS=y diff --git a/target/linux/malta/config-5.4 b/target/linux/malta/config-5.4 index 73f6965d01a9..9bcaae2208d9 100644 --- a/target/linux/malta/config-5.4 +++ b/target/linux/malta/config-5.4 @@ -329,7 +329,6 @@ CONFIG_TIMER_PROBE=y CONFIG_TREE_RCU=y CONFIG_TREE_SRCU=y CONFIG_USB_SUPPORT=y -# CONFIG_USERIO is not set CONFIG_USE_OF=y # CONFIG_VGA_CONSOLE is not set CONFIG_VM_EVENT_COUNTERS=y diff --git a/target/linux/omap/config-5.4 b/target/linux/omap/config-5.4 index aa15fa4ad9b4..3e2cc70d27b6 100644 --- a/target/linux/omap/config-5.4 +++ b/target/linux/omap/config-5.4 @@ -731,7 +731,6 @@ CONFIG_USB_PHY=y CONFIG_USB_SUPPORT=y CONFIG_USB_TI_CPPI41_DMA=y CONFIG_USB_TUSB_OMAP_DMA=y -# CONFIG_USERIO is not set CONFIG_USE_OF=y CONFIG_VFAT_FS=y CONFIG_VFP=y diff --git a/target/linux/oxnas/config-5.10 b/target/linux/oxnas/config-5.10 index 9fc311946954..9feea93b866f 100644 --- a/target/linux/oxnas/config-5.10 +++ b/target/linux/oxnas/config-5.10 @@ -343,7 +343,6 @@ CONFIG_UNCOMPRESS_INCLUDE="debug/uncompress.h" CONFIG_UNWINDER_ARM=y # CONFIG_UNWINDER_FRAME_POINTER is not set CONFIG_USB_SUPPORT=y -# CONFIG_USERIO is not set CONFIG_USE_OF=y CONFIG_VERSATILE_FPGA_IRQ=y CONFIG_VERSATILE_FPGA_IRQ_NR=4 diff --git a/target/linux/oxnas/config-5.4 b/target/linux/oxnas/config-5.4 index 11f35e295a3a..c7ac30089261 100644 --- a/target/linux/oxnas/config-5.4 +++ b/target/linux/oxnas/config-5.4 @@ -340,7 +340,6 @@ CONFIG_UNCOMPRESS_INCLUDE="debug/uncompress.h" CONFIG_UNWINDER_ARM=y # CONFIG_UNWINDER_FRAME_POINTER is not set CONFIG_USB_SUPPORT=y -# CONFIG_USERIO is not set CONFIG_USE_OF=y CONFIG_VERSATILE_FPGA_IRQ=y CONFIG_VERSATILE_FPGA_IRQ_NR=4 diff --git a/target/linux/rockchip/armv8/config-5.10
[PATCH 1/2] kernel: Deactivate CONFIG_VFIO in generic kernel config
Instead of deactivating this in every target config, deactivate it once in the generic kernel config. I was asked for this config option in a x86 64 build in OpenWrt 21.02. Signed-off-by: Hauke Mehrtens --- target/linux/generic/config-5.10| 1 + target/linux/generic/config-5.4 | 1 + target/linux/rockchip/armv8/config-5.10 | 1 - target/linux/rockchip/armv8/config-5.4 | 1 - target/linux/tegra/config-5.4 | 1 - 5 files changed, 2 insertions(+), 3 deletions(-) diff --git a/target/linux/generic/config-5.10 b/target/linux/generic/config-5.10 index cd096b2934cc..3bf985dc4c71 100644 --- a/target/linux/generic/config-5.10 +++ b/target/linux/generic/config-5.10 @@ -6709,6 +6709,7 @@ CONFIG_VDSO=y # CONFIG_VF610_ADC is not set # CONFIG_VF610_DAC is not set # CONFIG_VFAT_FS is not set +# CONFIG_VFIO is not set # CONFIG_VGASTATE is not set # CONFIG_VGA_ARB is not set # CONFIG_VGA_SWITCHEROO is not set diff --git a/target/linux/generic/config-5.4 b/target/linux/generic/config-5.4 index 0f6d3d7791d7..1dc97ce74598 100644 --- a/target/linux/generic/config-5.4 +++ b/target/linux/generic/config-5.4 @@ -6259,6 +6259,7 @@ CONFIG_VDSO=y # CONFIG_VF610_ADC is not set # CONFIG_VF610_DAC is not set # CONFIG_VFAT_FS is not set +# CONFIG_VFIO is not set # CONFIG_VGASTATE is not set # CONFIG_VGA_ARB is not set # CONFIG_VGA_SWITCHEROO is not set diff --git a/target/linux/rockchip/armv8/config-5.10 b/target/linux/rockchip/armv8/config-5.10 index 3a8b534bd57d..fedbeb5c4e54 100644 --- a/target/linux/rockchip/armv8/config-5.10 +++ b/target/linux/rockchip/armv8/config-5.10 @@ -645,7 +645,6 @@ CONFIG_USB_ULPI_VIEWPORT=y CONFIG_USB_XHCI_HCD=y CONFIG_USB_XHCI_PLATFORM=y # CONFIG_USERIO is not set -# CONFIG_VFIO is not set # CONFIG_VIRTIO_MENU is not set CONFIG_VMAP_STACK=y CONFIG_VM_EVENT_COUNTERS=y diff --git a/target/linux/rockchip/armv8/config-5.4 b/target/linux/rockchip/armv8/config-5.4 index 53adaf7760e0..a1663b19988f 100644 --- a/target/linux/rockchip/armv8/config-5.4 +++ b/target/linux/rockchip/armv8/config-5.4 @@ -617,7 +617,6 @@ CONFIG_USB_ULPI_VIEWPORT=y CONFIG_USB_XHCI_HCD=y CONFIG_USB_XHCI_PLATFORM=y # CONFIG_USERIO is not set -# CONFIG_VFIO is not set # CONFIG_VIRTIO_MENU is not set CONFIG_VMAP_STACK=y CONFIG_VM_EVENT_COUNTERS=y diff --git a/target/linux/tegra/config-5.4 b/target/linux/tegra/config-5.4 index 00fc11a69bae..c5e37f8d3bb0 100644 --- a/target/linux/tegra/config-5.4 +++ b/target/linux/tegra/config-5.4 @@ -553,7 +553,6 @@ CONFIG_USB_ULPI=y CONFIG_USB_ULPI_VIEWPORT=y # CONFIG_USERIO is not set CONFIG_USE_OF=y -# CONFIG_VFIO is not set CONFIG_VFP=y CONFIG_VFPv3=y CONFIG_WATCHDOG_CORE=y -- 2.30.2 ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH v2] ubusd: convert tx_queue to linked list
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
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH] ubusd: convert tx_queue to linked list
Hi Arnout, JFYI: Much of your patch resembles https://patchwork.ozlabs.org/project/lede/patch/20180503100622.11168-1-i@qbox.audio/ which was marked superseded for no apparent reason, but "anonymous remembers":). As for the limits, one of them could include a timeout (e.g. no progress in writing to client for over 10s => ubusd closes connection). Best regards, -- DoubleF ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH] ubusd: convert tx_queue to linked list
On 24/03/2021 09:31, Petr Štetiar wrote: > Arnout Vandecappelle (Essensium/Mind) [2021-03-23 16:23:25]: > > Hi, > >> Note that this infinite tx_queue opens the door to a DoS attack. > > memory is not infinite, so I think, that some sensible upper limit should be > kept. It might be much higher by default since it's dynamic now, but still > should be capped and lifted if needed. I agree, however: - the limit should be a global one (not per client); - it should take into account Rx, Tx and monitor buffers; - it's not exactly simple to implement (or I don't see a simple way to implement it); - I don't have time to work on this; - it's anyway a rather theoretical issue which already exists anyway, as explained in the commit message; - the fix is fixing an actual use case that is not theoretical at all (as in, we have a device that doesn't boot due to this issue). Regards, Arnout ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH] bcm4908: implement basic sysupgrade support
On 24.03.2021 19:10, Adrian Schmutzler wrote:
The usual nitpicks below ...
Thanks for review!
+ [ "$magic" = "GT-AC5300" ] && {
+ echo "asus"
+ return
+ }
+
+ echo "unknown"
+}
One could modify this towards a case with default "unknown" (and drop the
return again) ...
In the future this function will need to check for various variables so a
single case (switch) won't be an option.
I've fixed all other pointed out places!
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
