[RFC] add pr_sockaddr_buf and pr_peeraddr_buf
AbandonedPublic
Actions

Authored by mjg on Sep 5 2022, 12:26 PM.

Details

Reviewers

Group Reviewers

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.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

mjg created this revision.Sep 5 2022, 12:26 PM

Herald added subscribers: glebius, melifaro. · View Herald TranscriptSep 5 2022, 12:26 PM

mjg requested review of this revision.Sep 5 2022, 12:26 PM

mjg edited the summary of this revision. (Show Details)

mjg updated this revision to Diff 110144.Sep 5 2022, 12:32 PM

mjg edited the summary of this revision. (Show Details)

Herald added a subscriber: donner. · View Herald TranscriptSep 5 2022, 12:32 PM

mjg updated this revision to Diff 110145.Sep 5 2022, 12:33 PM

mjg edited the summary of this revision. (Show Details)

zlei added a subscriber: zlei.Sep 6 2022, 1:03 AM

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

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.

This revision now requires changes to proceed.Sep 6 2022, 3:56 PM

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

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.

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);
                        }
                }