> -----Original Message----- > From: Kyle Evans <kev...@freebsd.org> > Sent: Saturday, May 30, 2020 4:34 AM > To: Wei Hu <w...@freebsd.org> > Cc: src-committers <src-committ...@freebsd.org>; svn-src-all <svn-src- > a...@freebsd.org>; svn-src-head <svn-src-head@freebsd.org> > Subject: Re: svn commit: r361360 - head/sys/dev/hyperv/hvsock > > 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsv > > > > nweb.freebsd.org%2Fchangeset%2Fbase%2F361360&data=02%7C01%7C > weh% > > > > 40microsoft.com%7Cc97d2dddd23b440a8f1708d8040f9563%7C72f988bf86f1 > 41a > > > > f91ab2d7cd011db47%7C1%7C0%7C637263812262686264&sdata=xPJ95 > nVsWPV > > > f8qxlxSyMplJXuaFcbp5bd9FiGgvHhao%3D&reserved=0 > > > > > > 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 */
Thanks Kyle. It makes a lot of sense if it can be rejected as early as possible when it is not on HyperV. I am fine with the changes in the hvsock. The changes made in uipc_domain.c and domain.h may need more eyes to review from VIMAGE perspective. Thanks, Wei _______________________________________________ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
