linux-kernel - Re: [Patch V2 1/4] iommu/vt-d: replace *hdr with hdr[0] in struct dmar_drhd

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] [day] [month] [year] [list]

Message-ID: <20160321223113.GA16222@vultr.guest>
Date:	Mon, 21 Mar 2016 22:31:13 +0000
From:	Wei Yang <richard.weiyang@...il.com>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Wei Yang <richard.weiyang@...il.com>, joro@...tes.org,
	jiang.liu@...ux.intel.com, iommu@...ts.linux-foundation.org,
	linux-kernel@...r.kernel.org
Subject: Re: [Patch V2 1/4] iommu/vt-d: replace *hdr with hdr[0] in struct
 dmar_drhd_unit

On Mon, Mar 21, 2016 at 05:53:15PM +0100, Thomas Gleixner wrote:
>On Mon, 21 Mar 2016, Wei Yang wrote:
>> On Sun, Mar 20, 2016 at 04:42:29PM +0100, Thomas Gleixner wrote:
>> >On Sun, 20 Mar 2016, Wei Yang wrote:
>> >
>> >> hdr in struct dmar_drhd_unit is used to point the DMAR hardware unit copied
>> >> at the end of struct dmar_drhd_unit. One zero-sized array may be more
>> >> elegant for this purpose.
>> >
>> >You forget to tell why. 
>> > 
>> 
>> One possible benefit is to save some space.
>> 
>> Before commit 6b1972493a84 "iommu/vt-d: Implement DMAR unit hotplug framework",
>> dmaru->hdr just points to the memory region of DMA remapping hardware
>> definition. In this case, it would have no difference to where we put hdr.
>> 
>> After this commit, DMA remapping hardware definition is copied and
>> attach to the end of dmaru structure. By replacing a pointer with a zero-sized
>> array, that would save some space for this structure.
>
>Sure and exactly that explanation should be in the changelog. Not some
>handwaving "may be more elegant". We don't care about elegance, we care about
>correctness.
> 

Hey, Thomas

Nice to see your comment again~

You are right, I would add this in the changelog.

>> >> This patch replace *hdr with hdr[0] in struct dmar_drhd_unit.
>> >> 
>> >> Besides this, this patch includes other two changes:
>> >> 1. remove unnecessary type cast in dmar_table_detect()
>> >
>> >Again. Why is it not necessary?
>> >
>> 
>> The second parameter's type of function acpi_get_table_with_size() is "struct
>> acpi_table_header **", and type of dmar_tbl is "struct acpi_table_header *".
>> 
>> So without the type cast, the type of parameter and the function definition
>> matches.
>
>That's the information which a changelog wants to have, because otherwise a
>reviewer is forced to lookup the prototypes ....
>
>So a simple:
>
>   "Remove redundant type case to same type"
>
>would have told me what you are doing.
> 

Agree, thanks for your suggestion, this really helps reviewers.

>> >> 2. type cast from acpi_dmar_header to acpi_dmar_hardware_unit directly
>> >
>> >Don't even think about doing that. container_of() is there for a reason.
>> >
>> >Your change works today, because the embedded structure is the first one in
>> >the containing structure. If the containing structure gets reordered later,
>> >the whole thing will explode in hard to debug ways.
>> >
>> >Even if such a reordering is unlikely in that ACPI case, we just don't do
>> >that. It's bad and sloppy coding style. The generated code is the same.
>> >
>> 
>> Yes, I agree with you that make this change should be very careful, while I
>> think the original usage of container_of() is not necessary.
>
>It's not necessary, but it is correct. Removing it is just putting assumptions
>into the code, which is never a good idea.
>
>> Literally, it converts "struct acpi_dmar_header" to "struct
>> acpi_dmar_hardware_unit", because the first one is an element "header" of the
>> second one. While let's look at how the dmaru->hdr is initialized in
>> dmar_parse_one_drhd(), we copy the memory of "struct acpi_dmar_hardware_unit"
>> to a region where dmaru->hdr points to. So the code itself implies "struct
>> acpi_dmar_header" is the first element of "struct acpi_dmar_hardware_unit".
>u link
>Which is wrong to begin with. Assumption of first elements are crap.
>
>> Otherwise, we can't do this memcpy in dmar_parse_one_drhd().
> 
>> BTW, I am thinking changing the type of dmaru->hdr from "struct
>> acpi_dmar_header *" to "struct acpi_dmar_hardware_unit *". By doing so the
>> code would be more self explain, and it doesn't need to cast back and forth.
>
>Yes, that makes sense.
>

Ah, I am happy that you like this idea :-)

Let me format V3. Have a good night:)

>Thanks,
>
>	tglx

-- 
Wei Yang
Help you, Help me