netdev - Re: [PATCH v2 net-next 1/1] hv_sock: implements Hyper-V transport for Virtual Sockets (AF

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <20170824.181931.584865895464286033.davem@davemloft.net>
Date:   Thu, 24 Aug 2017 18:19:31 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     decui@...rosoft.com
Cc:     jhansen@...are.com, stefanha@...hat.com, netdev@...r.kernel.org,
        mkubecek@...e.cz, joe@...ches.com, olaf@...fle.de,
        sthemmin@...rosoft.com, jasowang@...hat.com, kys@...rosoft.com,
        haiyangz@...rosoft.com, dave.scott@...ker.com,
        linux-kernel@...r.kernel.org, apw@...onical.com,
        rolf.neugebauer@...ker.com, gregkh@...uxfoundation.org,
        marcelo.cerri@...onical.com, devel@...uxdriverproject.org,
        vkuznets@...hat.com, georgezhang@...are.com,
        dan.carpenter@...cle.com, acking@...are.com, dtor@...are.com,
        grantr@...are.com, cavery@...hat.com
Subject: Re: [PATCH v2 net-next 1/1] hv_sock: implements Hyper-V transport
 for Virtual Sockets (AF_VSOCK)

From: Dexuan Cui <decui@...rosoft.com>
Date: Wed, 23 Aug 2017 04:52:14 +0000

> +#define VMBUS_PKT_TRAILER	(sizeof(u64))

This is not the packet trailer, it's the size of the packet trailer.
Please make this macro name match more accurately what it is.

> +	/* Have we sent the zero-length packet (FIN)? */
> +	unsigned long fin_sent;

Why does this need to be atomic?  Why can't a smaller simpler
mechanism be used to make sure hvs_shutdown() only performs
hvs_send_data() call once on the channel?

> +static inline bool is_valid_srv_id(const uuid_le *id)
> +{
> +	return !memcmp(&id->b[4], &srv_id_template.b[4], sizeof(uuid_le) - 4);
> +}

Do not use the inline function attribute in *.c code.  Let the
compiler decide.

> +static inline unsigned int get_port_by_srv_id(const uuid_le *svr_id)

Likewise.

> +static inline void hvs_addr_init(struct sockaddr_vm *addr,

Likewise.

> +static inline void hvs_remote_addr_init(struct sockaddr_vm *remote,
> +					struct sockaddr_vm *local)

Likewise.

And so on and so forth, please audit this for your entire patch.

> +	*((u32 *)&h->vm_srv_id) = vsk->local_addr.svm_port;
> +	*((u32 *)&h->host_srv_id) = vsk->remote_addr.svm_port;

There has to be a better way to express this.

And if this is partially initializing vm_srv_id, at a minimum
endianness needs to be taken into account.