On Fri, May 22, 2020 at 10:51 AM Kyle Evans <kev...@freebsd.org> wrote:
>
> On Fri, May 22, 2020 at 4:17 AM Wei Hu <w...@freebsd.org> wrote:
> >
> > Author: whu
> > Date: Fri May 22 09:17:07 2020
> > New Revision: 361360
> > URL: https://svnweb.freebsd.org/changeset/base/361360
> >
> > Log:
> > Socket AF_HYPERV should return failure when it is not running on HyperV
> >
> > [... snip ...]
> > @@ -354,6 +354,9 @@ hvs_trans_attach(struct socket *so, int proto, struct
> > {
> > struct hvs_pcb *pcb = so2hvspcb(so);
> >
> > + if (vm_guest != VM_GUEST_HV)
> > + return (ESOCKTNOSUPPORT);
> > +
> > HVSOCK_DBG(HVSOCK_DBG_VERBOSE,
> > "%s: HyperV Socket hvs_trans_attach called\n", __func__);
> >
>
> This one may be OK, but none of these other methods should be able to
> be invoked if the attach failed. See this comment at around
> ^/sys/kern/uipc_socket.c#50:
>
> 50 * pru_detach() disassociates protocol layer state from an attached socket,
> 51 * and will be called exactly once for sockets in which pru_attach() has
> 52 * been successfully called. If pru_attach() returned an error,
> 53 * pru_detach() will not be called. Socket layer private.
>
> That said, I wonder if VNET_DOMAIN_SET provides the correct semantics
> here at all. You can't fail domain_init, but I don't think there's any
> value in this domain actually getting registered on non-HyperV guests,
> a fact which presumably won't change at runtime.
>
I'm considering the patch below, which is almost certainly going to be
mangled by my mail client, for solving this style of problem. It gives
the domain a chance to probe the system and opt out, for cases like
hvsock where leaving the domain around and fully-initialized when
there's no way it can work is both wasteful and a bit of an accident
waiting to happen -- IMO it's much less error prone and more
comforting if we just reject it early on, since we can. The vm_guest
detection and hyperv's false-positive aversion stuff in hyperv_init
should run much earlier than this.
diff --git a/sys/dev/hyperv/hvsock/hv_sock.c b/sys/dev/hyperv/hvsock/hv_sock.c
index d212c2d8c2d..d3bc1ab0f2c 100644
--- a/sys/dev/hyperv/hvsock/hv_sock.c
+++ b/sys/dev/hyperv/hvsock/hv_sock.c
@@ -74,6 +74,8 @@ SYSCTL_INT(_net_hvsock, OID_AUTO, hvs_dbg_level,
CTLFLAG_RWTUN, &hvs_dbg_level,
MALLOC_DEFINE(M_HVSOCK, "hyperv_socket", "hyperv socket control structures");
+static int hvs_dom_probe(void);
+
/* The MTU is 16KB per host side's design */
#define HVSOCK_MTU_SIZE (1024 * 16)
#define HVSOCK_SEND_BUF_SZ (PAGE_SIZE - sizeof(struct vmpipe_proto_header))
@@ -124,6 +126,7 @@ static struct protosw hv_socket_protosw[] = {
static struct domain hv_socket_domain = {
.dom_family = AF_HYPERV,
.dom_name = "hyperv",
+ .dom_probe = hvs_dom_probe,
.dom_protosw = hv_socket_protosw,
.dom_protoswNPROTOSW = &hv_socket_protosw[nitems(hv_socket_protosw)]
};
@@ -322,6 +325,16 @@ hvs_trans_unlock(void)
sx_xunlock(&hvs_trans_socks_sx);
}
+static int
+hvs_dom_probe(void)
+{
+
+ /* Don't even give us a chance to attach on non-HyperV. */
+ if (vm_guest != VM_GUEST_HV)
+ return (ENXIO);
+ return (0);
+}
+
void
hvs_trans_init(void)
{
@@ -329,9 +342,6 @@ hvs_trans_init(void)
if (!IS_DEFAULT_VNET(curvnet))
return;
- if (vm_guest != VM_GUEST_HV)
- return;
-
HVSOCK_DBG(HVSOCK_DBG_VERBOSE,
"%s: HyperV Socket hvs_trans_init called\n", __func__);
@@ -354,9 +364,6 @@ hvs_trans_attach(struct socket *so, int proto,
struct thread *td)
{
struct hvs_pcb *pcb = so2hvspcb(so);
- if (vm_guest != VM_GUEST_HV)
- return (ESOCKTNOSUPPORT);
-
HVSOCK_DBG(HVSOCK_DBG_VERBOSE,
"%s: HyperV Socket hvs_trans_attach called\n", __func__);
@@ -383,9 +390,6 @@ hvs_trans_detach(struct socket *so)
{
struct hvs_pcb *pcb;
- if (vm_guest != VM_GUEST_HV)
- return;
-
HVSOCK_DBG(HVSOCK_DBG_VERBOSE,
"%s: HyperV Socket hvs_trans_detach called\n", __func__);
@@ -595,9 +599,6 @@ hvs_trans_disconnect(struct socket *so)
{
struct hvs_pcb *pcb;
- if (vm_guest != VM_GUEST_HV)
- return (ESOCKTNOSUPPORT);
-
HVSOCK_DBG(HVSOCK_DBG_VERBOSE,
"%s: HyperV Socket hvs_trans_disconnect called\n", __func__);
@@ -925,9 +926,6 @@ hvs_trans_close(struct socket *so)
{
struct hvs_pcb *pcb;
- if (vm_guest != VM_GUEST_HV)
- return;
-
HVSOCK_DBG(HVSOCK_DBG_VERBOSE,
"%s: HyperV Socket hvs_trans_close called\n", __func__);
@@ -969,9 +967,6 @@ hvs_trans_abort(struct socket *so)
{
struct hvs_pcb *pcb = so2hvspcb(so);
- if (vm_guest != VM_GUEST_HV)
- return;
-
HVSOCK_DBG(HVSOCK_DBG_VERBOSE,
"%s: HyperV Socket hvs_trans_abort called\n", __func__);
diff --git a/sys/kern/uipc_domain.c b/sys/kern/uipc_domain.c
index 60e30eb1ae0..63217566ba9 100644
--- a/sys/kern/uipc_domain.c
+++ b/sys/kern/uipc_domain.c
@@ -170,9 +170,13 @@ protosw_init(struct protosw *pr)
void
domain_init(void *arg)
{
- struct domain *dp = arg;
+ struct domain_set_args *dsa = arg;
+ struct domain *dp;
struct protosw *pr;
+ if (!dsa->dsa_supported)
+ return;
+ dp = dsa->dsa_dp;
if (dp->dom_init)
(*dp->dom_init)();
for (pr = dp->dom_protosw; pr < dp->dom_protoswNPROTOSW; pr++)
@@ -198,8 +202,12 @@ vnet_domain_init(void *arg)
void
vnet_domain_uninit(void *arg)
{
- struct domain *dp = arg;
+ struct domain_set_args *dsa = arg;
+ struct domain *dp;
+ if (!dsa->dsa_supported)
+ return;
+ dp = dsa->dsa_dp;
if (dp->dom_destroy)
(*dp->dom_destroy)();
}
@@ -213,9 +221,14 @@ vnet_domain_uninit(void *arg)
void
domain_add(void *data)
{
+ struct domain_set_args *dsa;
struct domain *dp;
- dp = (struct domain *)data;
+ dsa = data;
+ dp = dsa->dsa_dp;
+ if (dp->dom_probe != NULL && (*dp->dom_probe)() != 0)
+ return;
+ dsa->dsa_supported = 1;
mtx_lock(&dom_mtx);
dp->dom_next = domains;
domains = dp;
diff --git a/sys/sys/domain.h b/sys/sys/domain.h
index e83dd0f4afc..11211f072c2 100644
--- a/sys/sys/domain.h
+++ b/sys/sys/domain.h
@@ -49,6 +49,7 @@ struct socket;
struct domain {
int dom_family; /* AF_xxx */
char *dom_name;
+ int (*dom_probe)(void); /* check for support (optional) */
void (*dom_init) /* initialize domain data structures */
(void);
void (*dom_destroy) /* cleanup structures / state */
@@ -79,20 +80,36 @@ void vnet_domain_init(void *);
void vnet_domain_uninit(void *);
#endif
+struct domain_set_args {
+ struct domain *dsa_dp;
+ int dsa_supported;
+};
+
#define DOMAIN_SET(name)
\
+ static struct domain_set_args name ## domain_set_args = { \
+ .dsa_dp = & name ## domain, \
+ }; \
SYSINIT(domain_add_ ## name, SI_SUB_PROTO_DOMAIN, \
- SI_ORDER_FIRST, domain_add, & name ## domain); \
+ SI_ORDER_FIRST, domain_add, &name ## domain_set_args); \
SYSINIT(domain_init_ ## name, SI_SUB_PROTO_DOMAIN, \
- SI_ORDER_SECOND, domain_init, & name ## domain);
+ SI_ORDER_SECOND, domain_init, &name ## domain_set_args);
#ifdef VIMAGE
+/*
+ * There's no such thing as per-vnet domains, so domain_set_args don't really
+ * need to be virtualized at the moment. Further consideration would be needed
+ * for such a concept to work at all.
+ */
#define VNET_DOMAIN_SET(name)
\
+ static struct domain_set_args name ## domain_set_args = { \
+ .dsa_dp = & name ## domain, \
+ }; \
SYSINIT(domain_add_ ## name, SI_SUB_PROTO_DOMAIN, \
- SI_ORDER_FIRST, domain_add, & name ## domain); \
+ SI_ORDER_FIRST, domain_add, &name ## domain_set_args); \
VNET_SYSINIT(vnet_domain_init_ ## name, SI_SUB_PROTO_DOMAIN, \
- SI_ORDER_SECOND, vnet_domain_init, & name ## domain); \
+ SI_ORDER_SECOND, vnet_domain_init, &name ## domain_set_args); \
VNET_SYSUNINIT(vnet_domain_uninit_ ## name, \
SI_SUB_PROTO_DOMAIN, SI_ORDER_SECOND, vnet_domain_uninit, \
- & name ## domain)
+ &name ## domain_set_args);
#else /* !VIMAGE */
#define VNET_DOMAIN_SET(name) DOMAIN_SET(name)
#endif /* VIMAGE */
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
