udp: make in_pcbbind_setup() acquire the hash lock internally

[glebius]

Summary

The function doesn't modify pcb database, only does lookups. Entering the
inpcb SMR section is sufficient.

[markj]

sys/netinet/udp_usrreq.c

1311–1312

I don't think this can be done, some of the callees acquire mutexes (e.g., prison_local_ip4()), and this is not allowed in an SMR section.

[glebius]

sys/netinet/udp_usrreq.c

1311–1312

You are right. I will update to just push the lock inside.

Interesting fact that this was not triggered by any of the tests.

[markj]

sys/netinet/udp_usrreq.c

1311–1312

I think this would happen only when binding within a classic, non-VNET jail. We do not have many tests which exercise this.

[glebius]

This is a bit branching of the initial topic, but I also found that arc4random() does mtx_lock() inside. And we use arc4random() when we select a random local port. So the entire function can't be put into SMR for that reason as well.

Then I looked into arc4random() and found out that it has percpu context for chacha and we basically lock context[curcpu].lock. I'm going to change this revision about in_pcbbind_setup() anyway, but I thought that we probably don't want such a leaf function as arc4random() to grab a mutex. Why can't it internally do sched_pin() or critical_enter() while it is working with percpu context?

[markj]

In D55973#1280957, @glebius wrote:

This is a bit branching of the initial topic, but I also found that arc4random() does mtx_lock() inside. And we use arc4random() when we select a random local port. So the entire function can't be put into SMR for that reason as well.

Then I looked into arc4random() and found out that it has percpu context for chacha and we basically lock context[curcpu].lock. I'm going to change this revision about in_pcbbind_setup() anyway, but I thought that we probably don't want such a leaf function as arc4random() to grab a mutex. Why can't it internally do sched_pin() or critical_enter() while it is working with percpu context?

One reason is that chacha20_randomstir() walks over all state structures, and your proposal would require binding to each CPU in turn, which we should avoid since it can take a long time. sched_pin() does not provide mutual exclusion, it would have to be critical_enter().

[glebius]

I see :( Also, IMHO, such secure random machinery as chacha offers is an overkill when we select one port out of 65k.

[glebius]

Give up on SMR synchronization for the large function and for now just
push the lock down into in_pcb.c.

Move in_pcbbind() down below the internal functions that it calls to avoid
forward declarations.