netdev - RE: [PATCH v14 net-next 1/1] hv_sock: introduce Hyper-V Sockets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 5 Jul 2016 09:31:51 +0000
From:	Dexuan Cui <decui@...rosoft.com>
To:	Joe Perches <joe@...ches.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"olaf@...fle.de" <olaf@...fle.de>,
	"apw@...onical.com" <apw@...onical.com>,
	"jasowang@...hat.com" <jasowang@...hat.com>,
	Vitaly Kuznetsov <vkuznets@...hat.com>,
	Cathy Avery <cavery@...hat.com>,
	KY Srinivasan <kys@...rosoft.com>
CC:	Haiyang Zhang <haiyangz@...rosoft.com>,
	Rolf Neugebauer <rolf.neugebauer@...ker.com>
Subject: RE: [PATCH v14 net-next 1/1] hv_sock: introduce Hyper-V Sockets

> From: Joe Perches [mailto:joe@...ches.com]
> 
> > +#define sk_to_hvsock(__sk)   ((struct hvsock_sock *)(__sk))
> > +#define hvsock_to_sk(__hvsk) ((struct sock *)(__hvsk))
> 
> Might as well be static inlines
Hi Joe,
Thank you for the suggestions (again)! :-)

I'll change them to static inlines.

> > +/* We send at most 4KB payload per VMBus packet. */
> > +struct hvsock_send_buf {
> > +	struct vmpipe_proto_header hdr;
> > +	u8 buf[PAGE_SIZE];
> 
> PAGE_SIZE might not be the right define here if
> the comment is to be believed.

I'll change to something like this:

+#define HVSOCK_MAX_SND_SIZE_BY_VM (1024 * 4)
 struct hvsock_send_buf {
        struct vmpipe_proto_header hdr;
-       u8 buf[PAGE_SIZE];
+       u8 buf[HVSOCK_MAX_SND_SIZE_BY_VM];
 };

 > > diff --git a/include/uapi/linux/hyperv.h b/include/uapi/linux/hyperv.h
> []
> > @@ -396,4 +397,27 @@ struct hv_kvp_ip_msg {
> >  	struct hv_kvp_ipaddr_value      kvp_ip_val;
> >  } __attribute__((packed));
> >
> > +/* This is the address fromat of Hyper-V Sockets.
> 
> format
I suppose you meant I should change 
/* This is ...
to 
/*
  * This is ...
I'll fix this.

> > diff --git a/net/hv_sock/af_hvsock.c b/net/hv_sock/af_hvsock.c
> []
> > @@ -0,0 +1,1519 @@
> > +/*
> > + * Hyper-V Sockets -- a socket-based communication channel between the
> > + * Hyper-V host and the virtual machines running on it.
> > + *
> > + * Copyright(c) 2016, Microsoft Corporation. All rights reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met
> .....
> Is this license GPL compatible?
Yes. At the end of the file, there is a line
+MODULE_LICENSE("Dual BSD/GPL");

> > +static struct proto hvsock_proto = {
> > +	.name = "HV_SOCK",
> > +	.owner = THIS_MODULE,
> > +	.obj_size = sizeof(struct hvsock_sock),
> > +};
> 
> const?
No. In hvsock_create(), hvsock_proto is passed to sk_alloc(), which requires
a non-const argument. 

> > +static int hvsock_recvmsg_wait(struct sock *sk, struct msghdr *msg,
> > +			       size_t len, int flags)
> > +{
> []
> > +				if (ret != 0 || payload_len >
> > +						sizeof(hvsk->recv->buf)) {
> 
> This could look nicer as
> 
> 				if (ret != 0 ||
> 				    payload_len > sizeof(hvsk->recv->buf)) {
I'll fix this.

Thanks,
-- Dexuan