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: <19cec6f0-e176-4bcc-95a0-9d6eb0261ed1@gmail.com>
Date:   Sun, 29 Oct 2023 02:56:17 +0530
From:   Abhinav Singh <singhabhinav9051571833@...il.com>
To:     "Michael Kelley (LINUX)" <mikelley@...rosoft.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

On 10/29/23 01:40, Michael Kelley (LINUX) wrote:
> 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?

The particular warning msg for this case is like this "warning: cast 
removes address space '__iomem' of expression". I only saw these warning 
one time inside the arch/x86/ directory.

>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.

So should I continue or not with this patch?

> 
> Michael
> 
>>
>> --
>> 2.39.2
> 

Thanks for taking out the time for reviewing this and giving the 
suggestions.

Thank You,
Abhinav Singh