linux-kernel - Re: [PATCH] virt: Add vboxguest driver for Virtual Box Guest integration

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: <20171003100449.GA5491@infradead.org>
Date:   Tue, 3 Oct 2017 03:04:49 -0700
From:   Christoph Hellwig <hch@...radead.org>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Michael Thayer <michael.thayer@...cle.com>,
        "Knut St . Osmundsen" <knut.osmundsen@...cle.com>,
        Larry Finger <Larry.Finger@...inger.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] virt: Add vboxguest driver for Virtual Box Guest
 integration

Looks like you forgot to CC previous revierers.

> +#define CHECK_IOCTL_IN(req)						     \
> +do {									     \
> +	if ((req)->Hdr.cbIn != (sizeof((req)->Hdr) + sizeof((req)->u.In)) || \
> +	    (req)->Hdr.cbOut != sizeof((req)->Hdr))			     \
> +		return -EINVAL;						     \
> +} while (0)

It seems like you ignored the comments on the last version.

Get rid of the weird struct capilization.

Make these things functions instead of macros.

> +	VMMDevReqHypervisorInfo *req;
> +	void *guest_mappings[GUEST_MAPPINGS_TRIES];

Fix your structure names.

> +	for (i = 0; i < (size >> PAGE_SHIFT); i++)

Remove pointless inner braces.

> +#ifdef CONFIG_X86_64
> +	req1->guestInfo.osType = VBOXOSTYPE_Linux26_x64;
> +#else
> +	req1->guestInfo.osType = VBOXOSTYPE_Linux26;
> +#endif

Hardcoding architecvtures is almost always a bug.  If you think it
isn't here it needs a big fat comment explaining that rationale.

> +	req->guestStatus.status = active ? VBoxGuestFacilityStatus_Active :
> +					   VBoxGuestFacilityStatus_Inactive;

Please use if/else for readability.

> +/** @name Memory Ballooning
> + * @{
> + */

Please get rid of this whacky comment style.

> +
> +/**
> + * Inflate the balloon by one chunk.
> + *
> + * The caller owns the balloon mutex.
> + *
> + * @returns 0 or negative errno value.
> + * @param   gdev	The Guest extension device.
> + * @param   chunk_idx	Index of the chunk.
> + */

An this one isn't proper kerneldoc either.

> +	pages = kmalloc(sizeof(*pages) * VMMDEV_MEMORY_BALLOON_CHUNK_PAGES,

> +/**
> + * Callback for heartbeat timer.
> + */

/**

as the comment start is reserved for kerneldoc comments, this one
isn't a kerneldoc comment, so please remove it.

> +static void vbg_heartbeat_timer(unsigned long data)
> +{
> +	struct vbg_dev *gdev = (struct vbg_dev *)data;
> +
> +	vbg_req_perform(gdev, gdev->guest_heartbeat_req);
> +	mod_timer(&gdev->heartbeat_timer,
> +		  msecs_to_jiffies(gdev->heartbeat_interval_ms));
> +}

Please use timer_setup and from_timer instead of the data argument.
(this is new in -rc3).

> +	list_add(&session->list_node, &gdev->session_list);
> +	spin_unlock_irqrestore(&gdev->session_spinlock, flags);

Session_list isn't used anywhere.  And if it was it would probably
buggy due to the lack of reference counting.

> +
> +	*session_ret = session;
> +
> +	return 0;

Just return the session or an ERR_PTR.

> +		guest_status = (const VMMDevReportGuestStatus *)req;

struct pointer asts are a ba idea - this probably should use
container_of magic or a more generic strucure with unioned elements.

> +int vbg_status_code_to_errno(int rc)
> +{
> +	if (rc >= 0)
> +		return 0;
> +
> +	switch (rc) {
> +	case VERR_ACCESS_DENIED:                    return -EPERM;

Please never put code in the same line as the switch statement.

Also in general code like this is better done by looping over a lookup
table.

Anyway, this was a super shallow review that everyone could do.

The actual ioctls and pv hardware interfaces look even worse, but
such a messy giant blob they aren't reviewable.  This patch needs
a major cleanup pass first, and then a split into reviewable chunks.