linux-kernel - Re: [PATCH 2/2] virt: vmgenid: add support for generation counter

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: <YuqUjUBxq6X738t/@kroah.com>
Date:   Wed, 3 Aug 2022 17:30:21 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     bchalios@...zon.es
Cc:     linux-kernel@...r.kernel.org, tytso@....edu, Jason@...c4.com,
        dwmw@...zon.co.uk, graf@...zon.de, xmarcalx@...zon.co.uk
Subject: Re: [PATCH 2/2] virt: vmgenid: add support for generation counter

On Wed, Aug 03, 2022 at 05:21:27PM +0200, bchalios@...zon.es wrote:
> +static const struct file_operations fops = {
> +	.owner = THIS_MODULE,
> +	.open = vmgenid_open,
> +	.read = vmgenid_read,
> +	.mmap = vmgenid_mmap,
> +	.llseek = noop_llseek,

Where is this new user/kernel api being documented?

See, put it in the code please, no one knows to look in a random file in
Documentation/ 


> +};
> +
> +static struct miscdevice vmgenid_misc = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = "vmgenid",
> +	.fops = &fops,
>  };
>  
>  static int parse_vmgenid_address(struct acpi_device *device, acpi_string object_name,
> @@ -57,7 +124,7 @@ static int vmgenid_add(struct acpi_device *device)
>  	phys_addr_t phys_addr;
>  	int ret;
>  
> -	state = devm_kmalloc(&device->dev, sizeof(*state), GFP_KERNEL);
> +	state = devm_kzalloc(&device->dev, sizeof(*state), GFP_KERNEL);
>  	if (!state)
>  		return -ENOMEM;
>  
> @@ -74,6 +141,27 @@ static int vmgenid_add(struct acpi_device *device)
>  
>  	device->driver_data = state;
>  
> +	/* Backwards compatibility. If CTRA is not there we just don't expose
> +	 * the char device
> +	 */
> +	ret = parse_vmgenid_address(device, "CTRA", &state->gen_cntr_addr);
> +	if (ret)
> +		return 0;
> +
> +	state->next_counter = devm_memremap(&device->dev, state->gen_cntr_addr,
> +			sizeof(u32), MEMREMAP_WB);
> +	if (IS_ERR(state->next_counter))
> +		return 0;
> +
> +	memcpy(&state->misc, &vmgenid_misc, sizeof(state->misc));
> +	ret = misc_register(&state->misc);
> +	if (ret) {
> +		devm_memunmap(&device->dev, state->next_counter);
> +		return 0;

Why are you not returning an error?  Why is this ok?

And why call devm_memunmap() directly?  That kind of defeats the purpose
of using devm_memremap(), right?

thanks,

greg k-h