linux-kernel - Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

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]

Date:   Tue, 19 Dec 2017 01:23:16 -0700
From:   "Jan Beulich" <JBeulich@...e.com>
To:     "Boris Ostrovsky" <boris.ostrovsky@...cle.com>
Cc:     <christian.koenig@....com>, <helgaas@...nel.org>,
        <xen-devel@...ts.xen.org>, "Juergen Gross" <jgross@...e.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host
 memory as UNUSABLE

>>> On 18.12.17 at 23:22, <boris.ostrovsky@...cle.com> wrote:
> +void __init arch_xen_balloon_init(struct resource *hostmem_resource)
> +{
> +	struct xen_memory_map memmap;
> +	int rc;
> +	unsigned int i, last_guest_ram;
> +	phys_addr_t max_addr = max_pfn << PAGE_SHIFT;

PFN_PHYS() as right now you still have an issue on 32-bit.

> +	struct e820_table *xen_e820_table;
> +	struct e820_entry *entry;

const?

> +	struct resource *res;
> +
> +	if (!xen_initial_domain())
> +		return;
> +
> +	xen_e820_table = kzalloc(sizeof(*xen_e820_table), GFP_KERNEL);

Wouldn't kmalloc() suffice here?

> +	if (!xen_e820_table)
> +		return;

Not saying "out of memory" here is certainly fine, but shouldn't
there nevertheless be a warning, as failure to go through the
rest of the function will impact overall functionality?

> +	memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);

Is it really reasonable to have a static upper bound here? As we
know especially EFI systems can come with a pretty scattered
(pseudo) E820 table. Even if (iirc) this has a static upper bound
right now in the hypervisor too, it would be nice if the kernel
didn't need further changes once the hypervisor is being made
more flexible.

> +	/* Mark non-RAM regions as not available. */
> +	for (; i < memmap.nr_entries; i++) {
> +		entry = &xen_e820_table->entries[i];
> +
> +		if (entry->type == E820_TYPE_RAM)
> +			continue;

I can't seem to match up this with ...

> +		if (entry->addr >= hostmem_resource->end)
> +			break;
> +
> +		res = kzalloc(sizeof(*res), GFP_KERNEL);
> +		if (!res)
> +			goto out;
> +
> +		res->name = "Host memory";

... this. Do you mean != instead (with the comment ahead of the
loop also clarified, saying something like "host RAM regions which
aren't RAM for us")? And perhaps better "Host RAM"?

> +		rc = insert_resource(hostmem_resource, res);
> +		if (rc) {
> +			pr_warn("%s: Can't insert [%llx - %llx] (%d)\n",

[%llx,%llx) ? Plus won't "ll" cause issues with 32-bit non-PAE builds?
(Same issues somewhere further down.)

> +				__func__, res->start, res->end, rc);
> +			kfree(res);
> +			goto  out;

Perhaps better not to bail out of the loop here (at least if rc is
not -ENOMEM)?

Jan