netdev - Re: [PATCH net-next 01/14] qed: Add CONFIG_QED

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]

Date:	Tue, 10 May 2016 11:02:09 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Yuval Mintz <Yuval.Mintz@...gic.com>
Cc:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Ariel Elior <Ariel.Elior@...gic.com>
Subject: Re: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV

On Tue, May 10, 2016 at 10:16 AM, Yuval Mintz <Yuval.Mintz@...gic.com> wrote:
>  > From: Yuval Mintz <Yuval.Mintz@...gic.com>
>> Date: Mon, 9 May 2016 16:19:10 +0300
>>
>> > +   /* bitmap indicating which fields hold valid values */
>> > +   aligned_u64 valid_bitmap;
>>
>> There is absolutely no reason to use aligned_u64 here.  That type is for handling
>> a specific issue in user facing APIs, which this is not.
>
> I'm not entirely convinced this is true; If we'll not enforce the alignment
> of this 64-bit field, it's possible there will be differences between 32-bit
> and 64-bit machines versions of this struct.
> You have to recall that this is going to be copied via DMA between PF and VF,
> so they must have the exact same representation of the structure.
>
> [If I'm wrong on the technical part here, please correct me; I vaguely
> seem to recall that this was already discussed on bnx2x's implementation
> of the hw-channel which also uses aligned u64 fields]

I think your change does have an impact, I just don't know if you
really realize what it will get you.  Specifically what using the
aligned_u64 is doing is forcing qed_bulletin_content to be u64 aligned
and introducing two holes in qed_bulletin on 32 bit platforms where
dma_addr_t might be 32 bit, and adding a 4 bytes of padding after
size.

My advice would be to update phys to be a u64 instead of a dma_addr_t.
That way it won't change size depending on 32 or 64 bit architecture.
Then you could align qed_bulletin to 8 bytes so that the size of the
structure remains constant on both 32 and 64 bit systems.

- Alex