From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yx1-f43.google.com (mail-yx1-f43.google.com [74.125.224.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1245E1B0413 for ; Wed, 19 Nov 2025 01:17:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.224.43 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763515061; cv=none; b=DmOTUDoPqrJDGNcvgSksAmZ4LC/v34USNYJ1ES3x2lnbknMbkokGK0YWGhgNNbCDz/l6kb1X7WiKaQ+YVIpS9m4TcYda0d2cdzElNOhsVt1OWaqDQzacryaXNa52gwK4naw74WojlMmMVUqyQqK4p0YIX7/L2s+TYGEixScXJ+s= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763515061; c=relaxed/simple; bh=rT7Doq+l7z7hbFLJv3oOESobuNgHarGO6IKGTw8AtDk=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=sEU3HQJVTIEVV8HGaKFUcSHGpRYuomdCgHcFxJ6bXPh3xxKEBVsMdQdKgGrRocLllAfUpcvKuDOjAPfyVCcqqY489hVrd6dCz/+nwAdaeutDquiMIE9Kd8ObZH8v5/CTnh1zn5ykKss9xW9EPKaut6z49gBYltDlXouHCfEMkT0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=K0Giw+JM; arc=none smtp.client-ip=74.125.224.43 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="K0Giw+JM" Received: by mail-yx1-f43.google.com with SMTP id 956f58d0204a3-63fca769163so5795023d50.2 for ; Tue, 18 Nov 2025 17:17:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1763515058; x=1764119858; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=2sqe8ncDEknCOi4UhxwOn5EBWZJsrkPpNEM6tqbGLh4=; b=K0Giw+JM9QeeH5Z2m9F2bFtocDz/lC2+BjkNUf8VQxjPcn/VeVzKCCaf54jfdAabgc 1nUF+OsFDHkKUJHG+q8VTQ/j1zhvsJtaWXUCH3pkj/KwOW1qo8bfPikvxt6g5cacG60Y YyuM6RsmftBZypBKj9JnI0+1ABHiLLvOTdSNJVuHPilyZqDKz7ZURUTsJ63iRfSR6FM7 QhUpB7g6k98OlPiiY3hORzd3XDmZk/K9XU5ltyNAPGBm6JQrQuPd0RYRwQUpXaDagZin 5piIoqZ51v1AGtPQJXGTa0ZkA8T7nMhbYEjOUtTdno8lUz3q1ipTT329WWiN+jfgoBYk /5NQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1763515058; x=1764119858; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=2sqe8ncDEknCOi4UhxwOn5EBWZJsrkPpNEM6tqbGLh4=; b=mA6zOBlUDtVRei58ZXRWY9xX1arOUxXlCfrd0krtopPa4lQFFwVWjjt9tISgFPdkJz 81N03ESgWku5wkSMsPTh0/f7m+7KvdQCQN9xjQ1jLKcAoZJ8Glp1wEQ8+NfnCTZ5T/kf qcmTzJprpuoEL71Bo071Ua/wbgxmQo35GeNtA0PFOye7H4tnK1DvZdxP0amtoPLQxpnf 0KMP3mdd7cJGUDWglwfqWUctKBF8GT24ci373xUX6xnl4Q/25tPvKZfZqWL12ECTdcLc zlSCVGVWvBBbrUHshk9tUh6NXP8BGbjJCgpk6wU1eBcg5ls8+P42seQaGOadl4ijybky AaEQ== X-Forwarded-Encrypted: i=1; AJvYcCXazmt5fatNK5Kb76OE8+gXSubW1IP6/jEmOVIVF+sv+aQQOlHMxDhnNyRJ3+dnCmlxspw=@vger.kernel.org X-Gm-Message-State: AOJu0YxCNUsZTfOIYnuy0GpAO8Gef+AeelG5svwNpkHTpcCA/jsH0bhx r8kDDn/AFfVnT1zDIPs7OVYcVEVhtxF732HnF1Fqk2xIpvFx1S6B99Zv X-Gm-Gg: ASbGncsKu2xbCVzhpugQTOaqFQduTRT4xffoNk5F4tUNbzh2vbanWfoGQp/ZNaKeCAz /p4q6smYNmwDiCSxIuTcq0zTCbp34bWA4UhowF0NMcjGrUwXoukHiC7txUnJiDiPSRdloV6EFJZ p9mizf/pxPOtIQSvGDbGwdXfrt9E+mhU2UoYaEraRn81IPPfshvTFexUEJyPOij0fbf9Z+TCh5Q mwy6qp4a1ybExywMx3mvirP13WofUs0p515FZ7vu4zfUoU0nZP8CyAeVUcz4GzeqJtr2X7W0DLv B9Yv9FxRma+LkcWAcw5rFj9KLAuKQFayBqawi9sBUWzgGOxxfwPs3f7i1FH6UmqkbDlc+3H/sb5 t3/zDUvD3iu0T4I2pvEleGjXh4bONEgHQnr7nVt+KtPvf5RyF2xOig65slCcMkZvxuJ0Uy6e97q 8OJLs5AO9JGcqIoejGDLNqbR1QZ+C9Ce1rmIQX7Iw9rHp0dOziaYbJI4IX4w== X-Google-Smtp-Source: AGHT+IE+l2e227udRixUaMV1OD0fbSyMRTRhSYspYsqsrhrAm7qQBfwUi7+G0snVl8c4IqgyshR/vA== X-Received: by 2002:a05:690e:240c:b0:63f:ba88:e8f9 with SMTP id 956f58d0204a3-641e75e62f4mr11153561d50.41.1763515058001; Tue, 18 Nov 2025 17:17:38 -0800 (PST) Received: from devvm11784.nha0.facebook.com ([2a03:2880:25ff:54::]) by smtp.gmail.com with ESMTPSA id 956f58d0204a3-6410e9e8f76sm6410956d50.4.2025.11.18.17.17.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 18 Nov 2025 17:17:37 -0800 (PST) Date: Tue, 18 Nov 2025 17:17:35 -0800 From: Bobby Eshleman To: Stefano Garzarella Cc: "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , Stefan Hajnoczi , "Michael S. Tsirkin" , Jason Wang , Eugenio =?iso-8859-1?Q?P=E9rez?= , Xuan Zhuo , "K. Y. Srinivasan" , Haiyang Zhang , Wei Liu , Dexuan Cui , Bryan Tan , Vishnu Dasa , Broadcom internal kernel review list , Shuah Khan , linux-kernel@vger.kernel.org, virtualization@lists.linux.dev, netdev@vger.kernel.org, kvm@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-kselftest@vger.kernel.org, Sargun Dhillon , berrange@redhat.com, Bobby Eshleman Subject: Re: [PATCH net-next v10 03/11] vsock: reject bad VSOCK_NET_MODE_LOCAL configuration for G2H Message-ID: References: <20251117-vsock-vmtest-v10-0-df08f165bf3e@meta.com> <20251117-vsock-vmtest-v10-3-df08f165bf3e@meta.com> Precedence: bulk X-Mailing-List: kvm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Tue, Nov 18, 2025 at 07:10:28PM +0100, Stefano Garzarella wrote: > On Mon, Nov 17, 2025 at 06:00:26PM -0800, Bobby Eshleman wrote: > > From: Bobby Eshleman > > > > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c > > index 2c937a2df83b..c8319cd1c232 100644 > > --- a/drivers/vhost/vsock.c > > +++ b/drivers/vhost/vsock.c > > @@ -64,6 +64,11 @@ static u32 vhost_transport_get_local_cid(void) > > return VHOST_VSOCK_DEFAULT_HOST_CID; > > } > > > > +static bool vhost_transport_supports_local_mode(void) > > +{ > > + return true; > > Should we enable this later, when we really add support, or it doesn't > affect anything if vhost-vsock is not really supporting it in this PR > (thinking about bisection issues). sgtm! > > > +} > > + > > /* Callers that dereference the return value must hold vhost_vsock_mutex or the > > * RCU read lock. > > */ > > @@ -412,6 +417,7 @@ static struct virtio_transport vhost_transport = { > > .module = THIS_MODULE, > > > > .get_local_cid = vhost_transport_get_local_cid, > > + .supports_local_mode = vhost_transport_supports_local_mode, > > > > .init = virtio_transport_do_socket_init, > > .destruct = virtio_transport_destruct, > > diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h > > index 59d97a143204..824d89657d41 100644 > > --- a/include/net/af_vsock.h > > +++ b/include/net/af_vsock.h > > @@ -180,6 +180,11 @@ struct vsock_transport { > > /* Addressing. */ > > u32 (*get_local_cid)(void); > > > > + /* Return true if this transport supports VSOCK_NET_MODE_LOCAL. > > nit: Here I would make it clearer that rather than supporting MODE_LOCAL, > the transport is not compatible with it, etc. > A summary of the excellent description we have in the commit. > sounds good! > > + * Otherwise, return false. > > + */ > > + bool (*supports_local_mode)(void); > > + > > /* Read a single skb */ > > int (*read_skb)(struct vsock_sock *, skb_read_actor_t); > > > > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c > > index 54373ae101c3..7a235bb94437 100644 > > --- a/net/vmw_vsock/af_vsock.c > > +++ b/net/vmw_vsock/af_vsock.c > > @@ -91,6 +91,12 @@ > > * and locked down by a namespace manager. The default is "global". The mode > > * is set per-namespace. > > * > > + * Note: LOCAL mode is only supported when using namespace-aware transports > > + * (vhost-vsock, loopback). If a guest-to-host transport (virtio-vsock, > > + * hyperv-vsock, vmci-vsock) is loaded, attempts to set LOCAL mode will fail > > + * with EOPNOTSUPP, as these transports do not support per-namespace > > + * isolation. > > Okay, maybe this is fine, so if you don't need to resend, feel free to > ignore the previous comment. > > > + * > > * The modes affect the allocation and accessibility of CIDs as follows: > > * > > * - global - access and allocation are all system-wide > > @@ -2765,17 +2771,30 @@ static int vsock_net_mode_string(const struct ctl_table *table, int write, > > if (*lenp >= sizeof(data)) > > return -EINVAL; > > > > - if (!strncmp(data, VSOCK_NET_MODE_STR_GLOBAL, sizeof(data))) > > + ret = 0; > > IIUC `ret` should already be 0 at this point, no? > > > + mutex_lock(&vsock_register_mutex); > > I honestly don't like to mix the parsing, with this new check, so what > about leaving the parsing as before this patch (also without the mutex), > then just (untested): > > mutex_lock(&vsock_register_mutex); > if (mode == VSOCK_NET_MODE_LOCAL && transport_g2h && > transport_g2h->supports_local_mode && > !transport_g2h->supports_local_mode()) { > ret = -EOPNOTSUPP; > goto out; > } > > if (!vsock_net_write_mode(net, mode)) { > ret = -EPERM; > } > out: > mutex_unlock(&vsock_register_mutex); > return ret; > } Makes sense, I can move that around for next rev. > > > + if (!strncmp(data, VSOCK_NET_MODE_STR_GLOBAL, sizeof(data))) { > > mode = VSOCK_NET_MODE_GLOBAL; > > - else if (!strncmp(data, VSOCK_NET_MODE_STR_LOCAL, sizeof(data))) > > + } else if (!strncmp(data, VSOCK_NET_MODE_STR_LOCAL, sizeof(data))) { > > + if (transport_g2h && transport_g2h->supports_local_mode && > > + !transport_g2h->supports_local_mode()) { > > + ret = -EOPNOTSUPP; > > + goto out; > > + } > > mode = VSOCK_NET_MODE_LOCAL; > > - else > > - return -EINVAL; > > + } else { > > + ret = -EINVAL; > > + goto out; > > + } > > > > - if (!vsock_net_write_mode(net, mode)) > > - return -EPERM; > > + if (!vsock_net_write_mode(net, mode)) { > > + ret = -EPERM; > > + goto out; > > + } > > > > - return 0; > > +out: > > + mutex_unlock(&vsock_register_mutex); > > + return ret; > > } > > > > static struct ctl_table vsock_table[] = { > > @@ -2916,6 +2935,7 @@ int vsock_core_register(const struct vsock_transport *t, int features) > > { > > const struct vsock_transport *t_h2g, *t_g2h, *t_dgram, *t_local; > > int err = mutex_lock_interruptible(&vsock_register_mutex); > > + struct net *net; > > > > if (err) > > return err; > > @@ -2938,6 +2958,22 @@ int vsock_core_register(const struct vsock_transport *t, int features) > > err = -EBUSY; > > goto err_busy; > > } > > + > > + /* G2H sockets break in LOCAL mode namespaces because G2H > > + * transports don't support them yet. Block registering new G2H > > + * transports if we already have local mode namespaces on the > > + * system. > > + */ > > + rcu_read_lock(); > > + for_each_net_rcu(net) { > > + if (vsock_net_mode(net) == VSOCK_NET_MODE_LOCAL) { > > + rcu_read_unlock(); > > + err = -EOPNOTSUPP; > > + goto err_busy; > > + } > > + } > > + rcu_read_unlock(); > > + > > t_g2h = t; > > } > > > > diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c > > index 432fcbbd14d4..279f04fcd81a 100644 > > --- a/net/vmw_vsock/hyperv_transport.c > > +++ b/net/vmw_vsock/hyperv_transport.c > > @@ -833,10 +833,16 @@ int hvs_notify_set_rcvlowat(struct vsock_sock *vsk, int val) > > return -EOPNOTSUPP; > > } > > > > +static bool hvs_supports_local_mode(void) > > +{ > > + return false; > > +} > > + > > static struct vsock_transport hvs_transport = { > > .module = THIS_MODULE, > > > > .get_local_cid = hvs_get_local_cid, > > + .supports_local_mode = hvs_supports_local_mode, > > > > .init = hvs_sock_init, > > .destruct = hvs_destruct, > > diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c > > index 5d379ccf3770..e585cb66c6f5 100644 > > --- a/net/vmw_vsock/virtio_transport.c > > +++ b/net/vmw_vsock/virtio_transport.c > > @@ -94,6 +94,18 @@ static u32 virtio_transport_get_local_cid(void) > > return ret; > > } > > > > +static bool virtio_transport_supports_local_mode(void) > > +{ > > + struct virtio_vsock *vsock; > > + > > + rcu_read_lock(); > > + vsock = rcu_dereference(the_virtio_vsock); > > + rcu_read_unlock(); > > + > > + /* Local mode is supported only when no G2H device is present. */ > > + return vsock ? false : true; > > +} > > + > > /* Caller need to hold vsock->tx_lock on vq */ > > static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq, > > struct virtio_vsock *vsock, gfp_t gfp) > > @@ -544,6 +556,7 @@ static struct virtio_transport virtio_transport = { > > .module = THIS_MODULE, > > > > .get_local_cid = virtio_transport_get_local_cid, > > + .supports_local_mode = virtio_transport_supports_local_mode, > > > > .init = virtio_transport_do_socket_init, > > .destruct = virtio_transport_destruct, > > diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c > > index 7eccd6708d66..da7c52ad7b2a 100644 > > --- a/net/vmw_vsock/vmci_transport.c > > +++ b/net/vmw_vsock/vmci_transport.c > > @@ -2033,6 +2033,12 @@ static u32 vmci_transport_get_local_cid(void) > > return vmci_get_context_id(); > > } > > > > +static bool vmci_transport_supports_local_mode(void) > > +{ > > + /* Local mode is supported only when no device is present. */ > > + return vmci_transport_get_local_cid() == VMCI_INVALID_ID; > > IIRC vmci can be registered both as H2G and G2H, so should we filter out > the H2G case? In fact, I'm realizing now that this should probably just be: static bool vmci_transport_supports_local_mode(void) { return false; } ... because even for H2G there is no mechanism for attaching a namespace to a VM (unlike w/ vhost_vsock device open). Does that seem right? Best, Bobby