RE: svn commit: r361360 - head/sys/dev/hyperv/hvsock
> -Original Message-
> From: Kyle Evans
> Sent: Saturday, May 30, 2020 4:34 AM
> To: Wei Hu
> Cc: src-committers ; svn-src-all a...@freebsd.org>; svn-src-head
> Subject: Re: svn commit: r361360 - head/sys/dev/hyperv/hvsock
>
> On Fri, May 22, 2020 at 10:51 AM Kyle Evans wrote:
> >
> > On Fri, May 22, 2020 at 4:17 AM Wei Hu 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%7Cc97d223b440a8f1708d8040f9563%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_
Re: svn commit: r361360 - head/sys/dev/hyperv/hvsock
On Fri, May 22, 2020 at 10:51 AM Kyle Evans wrote:
>
> On Fri, May 22, 2020 at 4:17 AM Wei Hu 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", __
Re: svn commit: r361360 - head/sys/dev/hyperv/hvsock
On Fri, May 22, 2020 at 4:17 AM Wei Hu 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 have some concerns about how hvsock works out with vnets, but I'll
drop those on the initial commit thread.
Thanks,
Kyle Evans
___
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"
