uipc_socket: add sogetsockaddr convenience function

[jhb]

Summary

This is part of the preliminaries for upstreaming wg(4).

It is useful to be able to call pr_usrreqs->pru_sockaddr on a struct
socket, so this commit adds a wrapper around it, analogous to the other
types of socket getters.

Original-author: Kyle Evans <[e-mail]>
Signed-off-by: Jason A. Donenfeld <[e-mail]>

[jason_zx2c4.com]

I had previously sent this to freebsd-net, but I'm told phrabricator is a better place for it. Copy and pasting from: https://lists.freebsd.org/pipermail/freebsd-net/2021-April/058173.html :

Hi,

This series of t̵w̵o̵ is a "trial series" to get a hang of sending in
patches to FreeBSD and to very gradually begin the process of
upstreaming parts of if_wg. I'll eventually have some larger crypto
changes to send in (in addition to if_wg itself, in due time), but it
seems wise to start with some very tiny commits first.

Some background: when if_wg needs things that aren't yet in src/, they
get added to "support.h":

https://git.zx2c4.com/wireguard-freebsd/tree/src/support.h

Then, functions that are in main, but aren't in the various releng or
stable branches, get put in "compat.h":

https://git.zx2c4.com/wireguard-freebsd/tree/src/compat.h

The goal is that over time, everything in support.h migrates to
compat.h.

So, this series contains two small preliminary commits. If you feel that
this is *too early*, and would rather that I wait to submit these all at
once alongside if_wg, whenever that is ready for review, that's fine
too, and I can do that. But in case you'd prefer that I start with
smaller commits now, here's a tiny series for your consideration.

Thanks,
Jason

[glebius]

The revision is already outdated, in main there is no pr_usrreqs.

I doubt value of the function if it is used in if_wg only. Normally in the network stack when we are dealing with a socket, we already are set into its vnet context.

[emaste]

I doubt value of the function if it is used in if_wg only

See the rest of the patch stack and in particular D36907; there are a half dozen or so existing instances to be replaced with sogetsockaddr

[glebius]

In D30087#838750, @emaste wrote:

I doubt value of the function if it is used in if_wg only

See the rest of the patch stack and in particular D36907; there are a half dozen or so existing instances to be replaced with sogetsockaddr

I'm sorry, didn't notice that. The function is useful of course.

I have several suggestions:

• Let's have a naming convention that a function that merely is a wrapper around so->so_proto->pr_foo() will be named sofoo(). This one will be sosockaddr() then.
• Let's implement it as inline. That would require moving the declaration into protosw.h, since socketvar.h users may not know protosw.
• Do we need CURVNET_SET/RESTORE? I doubt that! In priniciple it shouldn't. Transforming internal data to sockaddr doesn't require looking at any global structures at all. I reviewed all implementations and didn't find any evidence that VNET context is required.

[jhb]

In D30087#838752, @glebius wrote:

In D30087#838750, @emaste wrote:

I doubt value of the function if it is used in if_wg only

See the rest of the patch stack and in particular D36907; there are a half dozen or so existing instances to be replaced with sogetsockaddr

I'm sorry, didn't notice that. The function is useful of course.

I have several suggestions:

• Let's have a naming convention that a function that merely is a wrapper around so->so_proto->pr_foo() will be named sofoo(). This one will be sosockaddr() then.
• Let's implement it as inline. That would require moving the declaration into protosw.h, since socketvar.h users may not know protosw.
• Do we need CURVNET_SET/RESTORE? I doubt that! In priniciple it shouldn't. Transforming internal data to sockaddr doesn't require looking at any global structures at all. I reviewed all implementations and didn't find any evidence that VNET context is required.

The CURVNET_SET/RESTORE is just mimicking the existing code (see D36907). It could be removed in a followup perhaps, but I think that should be a separate commit vs this one that is just deduplicating existing code. I'm fine with changing the name. Making it an inline might be simpler after VNET is removed? I can try it though.

[glebius]

Once VNET calls are removed (I believe they should) is the function worth existing? None of the other protosw methods have inliners for them.

[glebius]

Quoting 21ca7b57bd9be4aa0b7f4e8d2fb62075319086b6 which initially added CURVNET around pru_sockaddr:

The exact placement of the CURVNET_SET() / CURVNET_RESTORE() macros
was a result of an empirical iterative process, whith an aim to
reduce recursions on CURVNET_SET() to a minimum, while still reducing
the scope of CURVNET_SET() to networking only operations - the
alternative would be calling CURVNET_SET() on each system call entry.
In general, curvnet has to be set in three typicall cases: when
processing socket-related requests from userspace or from within the
kernel; when processing inbound traffic flowing from device drivers
to upper layers of the networking stack, and when executing
timer-driven networking functions.

[emaste]

Do we need CURVNET_SET/RESTORE? I doubt that! In priniciple it shouldn't.

Ok, I think you're probably right. Perhaps the best course of action is to abandon D30087 and D36907, and update D30087 with a local copy of getsockaddr (or just expand it inline in two places). I don't want to hold up the wg import on this though, so I suggest that we either have the CURVNET_SET and _RESTORE in if_wg.c or not, depending on whether it still exists around other pru_sockaddr calls.

[glebius]

Let's just use straight bare so->so_proto->pr_sockaddr call in if_wg. For the rest of calls in the kernel, I will cleanup unneeded CURVNET_SET() myself.