Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 31 May 2023 17:56:48 +0200
From:   Simon Horman <simon.horman@corigine.com>
To:     Bobby Eshleman <bobby.eshleman@bytedance.com>
Cc:     Stefan Hajnoczi <stefanha@redhat.com>,
        Stefano Garzarella <sgarzare@redhat.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "K. Y. Srinivasan" <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>,
        Bryan Tan <bryantan@vmware.com>,
        Vishnu Dasa <vdasa@vmware.com>,
        VMware PV-Drivers Reviewers <pv-drivers@vmware.com>,
        kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hyperv@vger.kernel.org
Subject: Re: [PATCH RFC net-next v3 1/8] vsock/dgram: generalize recvmsg and
 drop transport->dgram_dequeue
Message-ID: <ZHduQMZG4an6A+DG@corigine.com>
References: <20230413-b4-vsock-dgram-v3-0-c2414413ef6a@bytedance.com>
 <20230413-b4-vsock-dgram-v3-1-c2414413ef6a@bytedance.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230413-b4-vsock-dgram-v3-1-c2414413ef6a@bytedance.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Wuo7ghWGi+4L4hNbSj6RX+0Bi6yJllK2VbhYVAXjoEz96xiALCQuNtk4bqdI?=
 =?us-ascii?Q?UaTI3Q6rVxzuI1t67FIXbd7t9SqBW+scdwmBhy0WadRoKt/FUsSUvVPW0Le6?=
 =?us-ascii?Q?7GLKbCBxzUvlX7dSNpu7+X+GKkxHBPah7UhR/TUgLMQV2Xdeff363lapLAzO?=
 =?us-ascii?Q?CUiaUJ0iwLlsE0syQry+EU8Mk96u0fmiRn9wcYR4DFuQxWxbV2rLrRIUz5Dl?=
 =?us-ascii?Q?qCPIIGAcKgiJWo35vfrzWny3Zqxxfhs1NAl5vUmYOWkCsV4jLXlIS+k7ZgOW?=
 =?us-ascii?Q?N4UBZDVceESXe58lerussfHUOIpkZ4XUDBxwrZEl2vEjwAgJtm1p/4nJFp1Q?=
 =?us-ascii?Q?Fsi7REMYvCn0YhzPZaupMqBal4jpW3wU/WtLHVBRRXHKJKQ//YCmRrCm/KOS?=
 =?us-ascii?Q?vqQXznXCOyP9NDULz9ItqeelurobN2m54Dc1lXJS5+EPrRTpu16Xxv0SYKV8?=
 =?us-ascii?Q?FXSxARfdSeKwXbReY+K/ro0me1EweSLTF4molOD/VtzHhP8GrVC+DBqdkYQT?=
 =?us-ascii?Q?FTYNDv7JZ29JuNPd8u9AHLICuD9eopwYDNTZY8mbibpOCo0UtzAIW+63fbMk?=
 =?us-ascii?Q?A5x38+7VlFQezRsfBb83PCHYzoISms0YKAlFD6fPN6Z5LAUAR8oI3KirPpyx?=
 =?us-ascii?Q?9PCKcTsXSoRehBOMwxvawQ1E0exwdP+00x7x6qonP6zgfLxCKq8XPy0Ba3H0?=
 =?us-ascii?Q?x0wNb/o7TZpQFnCntELxA+zXjwJ1fa0pKMlpbNgHQdSx/5lneZPlYNlxtKYA?=
 =?us-ascii?Q?KMEHO3eF+Zp6sfrwGDjk0zKeT/NofQtqj6yqP6Cx557ajLxjjALR89sKDNO9?=
 =?us-ascii?Q?8aOOTg46iZiFP1oYoeImj3zgwlxBTIMZ32wkke5YXx+Zru++rY9rLo6hR7yG?=
 =?us-ascii?Q?nVSCr02HwbR5r4SBZ7tVXQx2ySLRTOAt/CcppADpb3SJ8fiQ4lhgPZLQdZT2?=
 =?us-ascii?Q?SDobujmO528k23FsyfdpPhTJ6tFVJVKbVx36KwJku39LnzRxVjsXXoGrLcnR?=
 =?us-ascii?Q?r6cueAFZi2Z075vTnQi8WB2xAXrbIQ5if5y4dXAICqkeEHLCpkbEa/ngwEXD?=
 =?us-ascii?Q?tGVmXKTqkfySIcHJ0UFbNcAAk5OfsqrfavRZocJqrfu5fXSFcsSHUGa7KKw+?=
 =?us-ascii?Q?GbsNfc7Fi00X+OWXYqmcFBacCjTR29b0CVz4gL5MKGNTRmg0BwC2WqhSO9dN?=
 =?us-ascii?Q?iTi4+cJPbYP6im6pY+SxGJS5POi7Vn0yvnoKrBqUrIbFlnU/Af29FOinJIFz?=
 =?us-ascii?Q?gtglC3wOILuWqscUSm2mi/R6Gd1ucH0mkB1Aq/4AtDkb0XEUEiVK5mIWIHAF?=
 =?us-ascii?Q?ZxMU3XSI0PB01vMAcGTL/LdNpNVCCcj+c3Xpa+ItZB8QG0q7L3BpmzjMju/8?=
 =?us-ascii?Q?I+dG8qcRC5c/LGZgKONImmwNignMb+eA1DEUTWWME7B6aTda+8XbPx/6i9BA?=
 =?us-ascii?Q?9Xp51kNctf01ni7KsoqPRuey55RLmMgl1Nxp6HeEyHJ93q3TWx4+vitXW2pc?=
 =?us-ascii?Q?x3LgXr3xyWRqC23tSXnqrU1B0yNFvsmpyjRurPoJeLr0IMPSc7N4WuaPxEQk?=
 =?us-ascii?Q?NgQr0cfYtrZ/xmW5h8qKtiCOyCFXUEoqZgvpRqJ8S7UtDZyCK7PAVqDetEJ5?=
 =?us-ascii?Q?gkIlzKTqjeXjiXlwkHcC+oXsNqK8o6yhmCZA8aPwSetS7dQyokUccys2WVgg?=
 =?us-ascii?Q?PKlKxQ=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 61bf06ef-37df-42bd-be7b-08db61efa917
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2023 15:56:56.7467
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: d6YgPNRBMXEqz9MklyI86fQ+PMXec81+NBqGEF44IRwHF7MrwdyDXRrE0ui/u+QTdI5Xa9fgs5hREnp1w0PEG2/EWY9G00KZ6ohROBkrlyk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR13MB4469
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, May 31, 2023 at 12:35:05AM +0000, Bobby Eshleman wrote:
> This commit drops the transport->dgram_dequeue callback and makes
> vsock_dgram_recvmsg() generic. It also adds additional transport
> callbacks for use by the generic vsock_dgram_recvmsg(), such as for
> parsing skbs for CID/port which vary in format per transport.
> 
> Signed-off-by: Bobby Eshleman <bobby.eshleman@bytedance.com>

...

> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> index b370070194fa..b6a51afb74b8 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -1731,57 +1731,40 @@ static int vmci_transport_dgram_enqueue(
>  	return err - sizeof(*dg);
>  }
>  
> -static int vmci_transport_dgram_dequeue(struct vsock_sock *vsk,
> -					struct msghdr *msg, size_t len,
> -					int flags)
> +int vmci_transport_dgram_get_cid(struct sk_buff *skb, unsigned int *cid)
>  {
> -	int err;
>  	struct vmci_datagram *dg;
> -	size_t payload_len;
> -	struct sk_buff *skb;
>  
> -	if (flags & MSG_OOB || flags & MSG_ERRQUEUE)
> -		return -EOPNOTSUPP;
> +	dg = (struct vmci_datagram *)skb->data;
> +	if (!dg)
> +		return -EINVAL;
>  
> -	/* Retrieve the head sk_buff from the socket's receive queue. */
> -	err = 0;
> -	skb = skb_recv_datagram(&vsk->sk, flags, &err);
> -	if (!skb)
> -		return err;
> +	*cid = dg->src.context;
> +	return 0;
> +}

Hi Bobby,

clang-16 with W=1 seems a bit unhappy about this.

  net/vmw_vsock/vmci_transport.c:1734:5: warning: no previous prototype for function 'vmci_transport_dgram_get_cid' [-Wmissing-prototypes]
  int vmci_transport_dgram_get_cid(struct sk_buff *skb, unsigned int *cid)
      ^
  net/vmw_vsock/vmci_transport.c:1734:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
  int vmci_transport_dgram_get_cid(struct sk_buff *skb, unsigned int *cid)
  ^
  static 
  net/vmw_vsock/vmci_transport.c:1746:5: warning: no previous prototype for function 'vmci_transport_dgram_get_port' [-Wmissing-prototypes]
  int vmci_transport_dgram_get_port(struct sk_buff *skb, unsigned int *port)
      ^
  net/vmw_vsock/vmci_transport.c:1746:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
  int vmci_transport_dgram_get_port(struct sk_buff *skb, unsigned int *port)
  ^
  static 
  net/vmw_vsock/vmci_transport.c:1758:5: warning: no previous prototype for function 'vmci_transport_dgram_get_length' [-Wmissing-prototypes]
  int vmci_transport_dgram_get_length(struct sk_buff *skb, size_t *len)
      ^
  net/vmw_vsock/vmci_transport.c:1758:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
  int vmci_transport_dgram_get_length(struct sk_buff *skb, size_t *len)
  ^

I see similar warnings for net/vmw_vsock/af_vsock.c in patch 4/8.

> +
> +int vmci_transport_dgram_get_port(struct sk_buff *skb, unsigned int *port)
> +{
> +	struct vmci_datagram *dg;
>  
>  	dg = (struct vmci_datagram *)skb->data;
>  	if (!dg)
> -		/* err is 0, meaning we read zero bytes. */
> -		goto out;
> -
> -	payload_len = dg->payload_size;
> -	/* Ensure the sk_buff matches the payload size claimed in the packet. */
> -	if (payload_len != skb->len - sizeof(*dg)) {
> -		err = -EINVAL;
> -		goto out;
> -	}
> +		return -EINVAL;
>  
> -	if (payload_len > len) {
> -		payload_len = len;
> -		msg->msg_flags |= MSG_TRUNC;
> -	}
> +	*port = dg->src.resource;
> +	return 0;
> +}
>  
> -	/* Place the datagram payload in the user's iovec. */
> -	err = skb_copy_datagram_msg(skb, sizeof(*dg), msg, payload_len);
> -	if (err)
> -		goto out;
> +int vmci_transport_dgram_get_length(struct sk_buff *skb, size_t *len)
> +{
> +	struct vmci_datagram *dg;
>  
> -	if (msg->msg_name) {
> -		/* Provide the address of the sender. */
> -		DECLARE_SOCKADDR(struct sockaddr_vm *, vm_addr, msg->msg_name);
> -		vsock_addr_init(vm_addr, dg->src.context, dg->src.resource);
> -		msg->msg_namelen = sizeof(*vm_addr);
> -	}
> -	err = payload_len;
> +	dg = (struct vmci_datagram *)skb->data;
> +	if (!dg)
> +		return -EINVAL;
>  
> -out:
> -	skb_free_datagram(&vsk->sk, skb);
> -	return err;
> +	*len = dg->payload_size;
> +	return 0;
>  }
>  
>  static bool vmci_transport_dgram_allow(u32 cid, u32 port)

...