[RFC] add pr_sockaddr_buf and pr_peeraddr_buf

[mjg]

Summary

At least netgraph calls pr_sockaddr and pr_peeraddr from within epoch (see ng_ksocket_rcvmsg -> NGM_KSOCKET_GETPEERNAME), which immediately panics with invariants due to malloc(..., M_WAITOK) performed there.

I think a second set of ops should be added which accepts passing a preallocated buffer, even something on-stack.

struct sockaddr has a small upper bound size, so this is very feasible.

Illustrative patch below. If network is ok with it, I'll make it operational.

Note that callers only need to allocate

__offsetof(struct sockaddr, sa_data) + SOCK_MAXADDRLEN

bytes. This can be hidden away in struct sockaddr_max or perhaps just a macro indicating the size.

Alternatively a set of routines which take malloc flags can be added, but imo this only adds a failure point (malloc returning NULL) which does not have to exist.

Comments? Note I don't care for specific names, just the mechanism.

[mjg]

can i get some feedback here? the netgraph panic is partially blocking other work

[glebius]

The ng_ksocket is kind of a hack in principle. We shouldn't adopt generic structures to satisfy its demands. The netgraph itself definitely needs additional care wrt the epoch(9) protected network stack. All the epoch(9) related commits I did to netgraph are a very rough approach to what really needs to be done. @afedorov promises to invest some time into that. Once netgraph is updated, this particular problem will be solved.

Getting back to this particular problem. What is your scenario when NGM_KSOCKET_GETPEERNAME is called from the kernel context? Cause if message arrives from netgraph socket the thread is not in epoch, unless it is forced to be queued. If you are hitting the queue case, I can invent something in scope of netgraph to fix that.

[afedorov]

In D36451#828133, @mjg wrote:

can i get some feedback here? the netgraph panic is partially blocking other work

May you show the panic stack trace?

If the patch only solves the Netgraph(4) issue, then I don't like it.

There are many places in netgraph where NGQF_MESG message processing can call functions with M_WAITOK. Moreover, nothing forbids calling the code sections forbidden in epochs. Because ng_ksocket_rcvmsg() is the control path. On the other hand, ng_ksocket_rcvdata() is a hot path that should be called in epoch context.

I think we should generally disable ng_item (NGQF_MESG) processing in an epoch context.

For example, another panic in ng_eiface(4):

panic: epoch_wait_preempt() called in the middle of an epoch section of the same epoch
cpuid = 0
time = 1600268186
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe0154b9e880
vpanic() at vpanic+0x182/frame 0xfffffe0154b9e8d0
panic() at panic+0x43/frame 0xfffffe0154b9e930
epoch_wait_preempt() at epoch_wait_preempt+0x293/frame 0xfffffe0154b9e980
if_detach_internal() at if_detach_internal+0x1ca/frame 0xfffffe0154b9e9e0
if_detach() at if_detach+0x3d/frame 0xfffffe0154b9ea00
ng_eiface_rmnode() at ng_eiface_rmnode+0x55/frame 0xfffffe0154b9ea40
ng_rmnode() at ng_rmnode+0x188/frame 0xfffffe0154b9ea70
ng_mkpeer() at ng_mkpeer+0x7b/frame 0xfffffe0154b9eac0
ng_apply_item() at ng_apply_item+0x547/frame 0xfffffe0154b9eb40
ngthread() at ngthread+0x26e/frame 0xfffffe0154b9ebb0
fork_exit() at fork_exit+0x80/frame 0xfffffe0154b9ebf0
fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe0154b9ebf0

[mjg]

malloc_dbg() at malloc_dbg+0xd4/frame 0xfffffe0046ffed10
malloc() at malloc+0x2d/frame 0xfffffe0046ffed50
in_getsockaddr() at in_getsockaddr+0x6a/frame 0xfffffe0046ffed80
ng_ksocket_rcvmsg() at ng_ksocket_rcvmsg+0x345/frame 0xfffffe0046ffedf0
ng_apply_item() at ng_apply_item+0x3be/frame 0xfffffe0046ffee80
ngthread() at ngthread+0x200/frame 0xfffffe0046ffeef0
fork_exit() at fork_exit+0x80/frame 0xfffffe0046ffef30
fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe0046ffef30

netgraph panic aside, my point is that it is incredibly weird to induce M_WAITOK allocation for things which arguably should be readily available, but I'm not going to argue one way or the other.

[afedorov]

My main objection is that it is bad to extend 'struct protosw' for only one consumer ( ng_kocket(4) ).

As for netgraph(4), a lot of refactoring is required there.

It seems to me that a patch like this one could be a temporary solution:

diff --git a/sys/netgraph/ng_base.c b/sys/netgraph/ng_base.c
index ac9d55a6f703..092231850f18 100644
--- a/sys/netgraph/ng_base.c
+++ b/sys/netgraph/ng_base.c
@@ -3439,7 +3439,19 @@ ngthread(void *arg)
                        } else {
                                NG_QUEUE_UNLOCK(&node->nd_input_queue);
                                NGI_GET_NODE(item, node); /* zaps stored node */
-                               ng_apply_item(node, item, rw);
+
+                               if ((item->el_flags & NGQF_TYPE) == NGQF_MESG) {
+                                       /*
+                                        * NGQF_MESG items should never be processed in
+                                        * NET_EPOCH context. So, temporary exit from EPOCH.
+                                        */
+                                       NET_EPOCH_EXIT(et);
+                                       ng_apply_item(node, item, rw);
+                                       NET_EPOCH_ENTER(et);
+                               } else {
+                                       ng_apply_item(node, item, rw);
+                               }
+
                                NG_NODE_UNREF(node);
                        }
                }

@mjg , may you test it?

[mjg]

thanks for the patch, i supplied a test kernel to the coworker who can reproduce the problem. hopefully he will be able to sort it out today.

[mjg]

@afedorov the patch works fine, no other issues either

[afedorov]

Thanks. I will submit this patch in a separate review where we can discuss the details.

[glebius]

This looks like a "good enough" solution for now. Thanks, Alexander!