linux-kernel - RE: [PATCH] Fixing warning cast removes address space '_

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: <SN6PR2101MB16937C421EA9CDF373835360D7A3A@SN6PR2101MB1693.namprd21.prod.outlook.com>
Date:   Sat, 28 Oct 2023 20:10:51 +0000
From:   "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To:     Abhinav Singh <singhabhinav9051571833@...il.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "hpa@...or.com" <hpa@...or.com>,
        Nischala Yelchuri <Nischala.Yelchuri@...rosoft.com>
CC:     "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-kernel-mentees@...ts.linuxfoundation.org" 
        <linux-kernel-mentees@...ts.linuxfoundation.org>
Subject: RE: [PATCH] Fixing warning cast removes address space '__iomem' of
 expression

From: Abhinav Singh <singhabhinav9051571833@...il.com> Sent: Tuesday, October 24, 2023 4:29 AM
> 

Subject lines usually have a prefix to indicate the area of the kernel
the patch is for.   We're not always super consistent with the prefixes, 
but you can look at the commit log for a file to see what is
typically used.  In this case, the prefix is usually "x86/hyperv:"

>
> This patch fixes sparse complaining about the removal of __iomem address
> space when casting the return value of this function ioremap_cache(...)
> from `void __ioremap*` to `void*`.

Should avoid wording like "this patch" in commit messages.  See
the commit message guidelines in the "Describe your changes"
section of Documentation/process/submitting-patches.rst.  A
better approach is to just state the problem:  "Sparse complains
about the removal .....".  Then describe the fix.  Also avoid 
pronouns like "I" or "you".

> 
> I think there are two way of fixing it, first one is changing the
> datatype of variable `ghcb_va` from `void*` to `void __iomem*` .
> Second way of fixing it is using the memremap(...) which is
> done in this patch.
> 
> Signed-off-by: Abhinav Singh <singhabhinav9051571833@...il.com>
> ---
>  arch/x86/hyperv/hv_init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 21556ad87f4b..c14161add274 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -70,7 +70,7 @@ static int hyperv_init_ghcb(void)
> 
>  	/* Mask out vTOM bit. ioremap_cache() maps decrypted */

This comment mentions ioremap_cache().  Since you are changing
to use memremap() instead, the comment should be updated to
match.

>             ghcb_gpa &= ~ms_hyperv.shared_gpa_boundary;
> -           ghcb_va = (void *)ioremap_cache(ghcb_gpa, HV_HYP_PAGE_SIZE);
> +          ghcb_va = memremap(ghcb_gpa, HV_HYP_PAGE_SIZE, MEMREMAP_WB);

As noted in the comment, ioremap_cache() provides a mapping that
accesses the memory as decrypted.  To be equivalent, the call to
memremap() should include the MEMREMAP_DEC flag so that it
also is assured of producing a decrypted mapping.

Also, corresponding to the current ioremap_cache() call here,
there's an iounmap() call in hv_cpu_die().   To maintain proper
pairing, that iounmap() call should be changed to memunmap().

It turns out there are other occurrences of this same pattern in
Hyper-V specific code in the Linux kernel.  See hv_synic_enable_regs(),
for example.   Did "sparse" flag the same problem in those
occurrences?  It turns out that Nischala Yelchuri at Microsoft is
concurrently working on fixing this occurrence as well as the
others we know about in Hyper-V specific code.

Michael

> 
> --
> 2.39.2