Page MenuHomeFreeBSD

uipc_socket: add sogetsockaddr convenience function
AbandonedPublic

Authored by jhb on May 3 2021, 8:21 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Feb 5, 4:50 PM
Unknown Object (File)
Dec 16 2022, 2:29 PM
Unknown Object (File)
Dec 13 2022, 2:38 PM
Unknown Object (File)
Dec 7 2022, 7:24 AM

Details

Reviewers
kevans
emaste
glebius
jason_zx2c4.com
Group Reviewers
network
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 <kevans@FreeBSD.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

jason_zx2c4.com created this revision.

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

This revision is now accepted and ready to land.Oct 8 2022, 12:52 AM
glebius added a subscriber: 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.

This revision now requires changes to proceed.Oct 10 2022, 11:51 PM

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 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.

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.

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

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.

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.

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.

jhb abandoned this revision.
jhb added a reviewer: jason_zx2c4.com.