linux-kernel - Re: [PATCH v5 01/10] hyperv: Convert Hyper-V status codes to strings

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: <69c868f9-8bac-4bbe-ba56-832ab6a21660@linux.microsoft.com>
Date: Fri, 28 Feb 2025 08:40:10 -0800
From: Roman Kisel <romank@...ux.microsoft.com>
To: Nuno Das Neves <nunodasneves@...ux.microsoft.com>,
 linux-hyperv@...r.kernel.org, x86@...nel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-arch@...r.kernel.org, linux-acpi@...r.kernel.org
Cc: kys@...rosoft.com, haiyangz@...rosoft.com, wei.liu@...nel.org,
 mhklinux@...look.com, decui@...rosoft.com, catalin.marinas@....com,
 will@...nel.org, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, hpa@...or.com, daniel.lezcano@...aro.org,
 joro@...tes.org, robin.murphy@....com, arnd@...db.de,
 jinankjain@...ux.microsoft.com, muminulrussell@...il.com,
 skinsburskii@...ux.microsoft.com, mrathor@...ux.microsoft.com,
 ssengar@...ux.microsoft.com, apais@...ux.microsoft.com,
 Tianyu.Lan@...rosoft.com, stanislav.kinsburskiy@...il.com,
 gregkh@...uxfoundation.org, vkuznets@...hat.com, prapal@...ux.microsoft.com,
 muislam@...rosoft.com, anrayabh@...ux.microsoft.com, rafael@...nel.org,
 lenb@...nel.org, corbet@....net
Subject: Re: [PATCH v5 01/10] hyperv: Convert Hyper-V status codes to strings



On 2/27/2025 4:15 PM, Nuno Das Neves wrote:
> On 2/27/2025 9:02 AM, Roman Kisel wrote:

[...]

> I guess you're implying it's not worth adding such a function for only a
> few places in the code? That is a good point, and a bit of an oversight
> on my part while editing this series. Originally all the hypercall helper
> functions in the driver code (10+ places) used this function as well, but
> I removed those printks_()s as a temporary solution to limit the use of
> printk in the driver code (as opposed to dev_printk() which is preferred).
> 
> I didn't think to remove *this* patch as a result of that change!
> I do want to figure out a good way to add that logging back to the hypercall
> helpers, so I do want to try and get some form of this patch in to aid
> debugging hypercalls - it has been very very useful over time.
> 

Right, I thought that the function looked more as a bring-up aid rather
than a full fledged solution to some problem.

>> The "Unknown" part would make debugging harder actually when something
>> fails. I presume that the mainstream scenarios all work, and it is the
>> edge cases that might fail, and these are likelier to produce "Unknown".
>>
> That is a very good point. Ideally, we could log "Unknown" along with
> the hex code instead of replacing it.
> 
> What do you think about keeping this function, but instead of using it
> directly, introduce a "standard" way for logging hypercall errors which
> can hopefully be used everywhere in the kernel?
> e.g. a simple macro:
> #define hv_hvcall_err(control, status)
> do {
> 	u64 ___status = (status);
> 	pr_err("Hypercall: %#x err: %#x : %s", (control) & 0xFFFF, hv_result(___status), hv_result_to_string(___status));
> } while (0)
> 
> I feel like this is the best of both worlds, and actually makes it even
> easier to do this logging everywhere it is wanted (for me, that includes
> all the /dev/mshv-related hypercalls).
> We could add strings for the HVCALL_ values too, and/or include __func__
> in the macro to aid in finding the context it was used in.
> 

That doesn’t seem to be common in the kernel from what I’ve seen in 
dmesg, although there is certainly a lot of appeal in that approach. 
However, we will have to remember to update the function each time when 
another status code is added not to leave things half-cooked.

Also it is a bit surprising the *kernel* should report that rather than 
the VMM from the user mode. E.g. the kernel does not report all errors 
on file open, file seek, etc. As I understand, the hv status codes are
later mapped to errno in a lossy manner, and errno is what the user mode
receives?

As long as the hex code is logged, I am fine with the change.

>> Folks who actually debug failed hypercalls rarely have issues with
>> looking up the error code, and printing "Unknown" to the log is worse
>> than a hexadecimal. Like even the people who wrote the code got nothing
>> to say about what is going on.
>>
> Yep, totally agree having the hex code available can be valuable in
> unexpected situations.
> 

Appreciate giving my concerns a thorough consideration!

-- 
Thank you,
Roman