svn commit: r361360 - head/sys/dev/hyperv/hvsock
Wei Hu
weh at microsoft.com
Tue Jun 2 14:29:58 UTC 2020
> -----Original Message-----
> From: Kyle Evans <kevans at freebsd.org>
> Sent: Saturday, May 30, 2020 4:34 AM
> To: Wei Hu <whu at freebsd.org>
> Cc: src-committers <src-committers at freebsd.org>; svn-src-all <svn-src-
> all at freebsd.org>; svn-src-head <svn-src-head at freebsd.org>
> Subject: Re: svn commit: r361360 - head/sys/dev/hyperv/hvsock
>
> On Fri, May 22, 2020 at 10:51 AM Kyle Evans <kevans at freebsd.org> wrote:
> >
> > On Fri, May 22, 2020 at 4:17 AM Wei Hu <whu at 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
More information about the svn-src-head
mailing list