linux-kernel - Re: [RESEND PATCH v2] devres: Really align data field to unsigned long long

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:   Mon, 9 Jul 2018 07:17:11 +0000
From:   Alexey Brodkin <Alexey.Brodkin@...opsys.com>
To:     "greg@...ah.com" <greg@...ah.com>
CC:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "geert@...ux-m68k.org" <geert@...ux-m68k.org>,
        "linux-snps-arc@...ts.infradead.org" 
        <linux-snps-arc@...ts.infradead.org>
Subject: Re: [RESEND PATCH v2] devres: Really align data field to unsigned
 long long

Hi Greg,

On Mon, 2018-07-09 at 09:06 +0200, greg@...ah.com wrote:
> On Mon, Jul 09, 2018 at 06:46:50AM +0000, Alexey Brodkin wrote:
> > Hi Greg,
> > 
> > On Mon, 2018-07-09 at 07:48 +0200, Greg KH wrote:
> > > On Mon, Jul 09, 2018 at 07:44:44AM +0300, Alexey Brodkin wrote:
> > > > Depending on ABI "long long" type of a particular 32-bit CPU
> > > > might be aligned by either word (32-bits) or double word (64-bits).
> > > > Make sure "data" is really 64-bit aligned for any 32-bit CPU.
> > > > 
> > > > At least for 32-bit ARC cores ABI requires "long long" types
> > > > to be aligned by normal 32-bit word. This makes "data" field aligned to
> > > > 12 bytes. Which is still OK as long as we use 32-bit data only.
> > > > 
> > > > But once we want to use native atomic64_t type (i.e. when we use special
> > > > instructions LLOCKD/SCONDD for accessing 64-bit data) we easily hit
> > > > misaligned access exception.
> > > 
> > > So is this something you hit today?  If not, why is this needed for
> > > stable kernels?
> > 
> > Indeed we hit that problem recently when Etnaviv driver was switched to
> > DRM GPU scheduler, see
> > commit e93b6deeb45a ("drm/etnaviv: hook up DRM GPU scheduler").
> > The most important part of DRM GPU scheduler is "job_id_count" member of
> > "drm_gpu_scheduler" structure of type "atomic64_t". This structure is put
> > in a buffer allocated by devm_kzalloc() and if "job_id_count" is not 64-bit
> > aligned atomic instruction fails with an exception.
> > 
> > As for stable requirements - mentioned commit was a part of 4.17 kernel
> > which broke GPU driver for one of our HSDK board so I guess back-porting
> > to 4.17 is a no-brainer.
> 
> Ok, so 4.17 is as far back as you need?  Please try to be specific when
> asking for stable backports.

Well in that particular case I really wanted to get this fix back-ported
starting from v4.8 so I guess that's what I need to add in Cc tag, right?
----------------------------->8---------------------
Cc: <stable@...r.kernel.org> # 4.8+
----------------------------->8---------------------

> > > > That's because even on CPUs capable of non-aligned data access LL/SC
> > > > instructions require strict alignment.
> > > 
> > > Are you going to hit this code with all types of structures?
> > 
> > If there're other cases which lead to 4-byte aligned "atomic64_t" variables
> > there will be a problem as well but it's quite hard to predict those cases.
> > That said if we manage to reproduce more similar issues there will be more
> > patches with fixes :)
> > 
> > > What happens when you do have an unaligned access?
> > 
> > Atomic instructions are a bit special as compared to normal loads and stores.
> > Even if normal loads and stores may deal with unaligned data atomic instructions
> > still require data to be aligned because it's hard to manage atomic value that
> > spans through multiple cache lines or even MMU pages. And hardware just
> > raises an alignment fault exception.
> > 
> > And that's not something special for ARC, I guess all CPUs are the same in
> > that regard, see here's an extract from ARM(r) Architecture Reference
> > Manual ARMv7-A and ARMv7-R edition: https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2017_12_5_440&d=DwIBAg&c=DPL6_X_6JkXFx7AXWq
> > B0tg&r=lqdeeSSEes0GFDDl656eViXO7breS55ytWkhpk5R81I&m=eT1OUXQEt6zlA0bABwdI7sFr7Hys3aHzoCXTDAkM6xY&s=-3n_Xurm4D2TIC-_G_GIvGj9P_3unmq839oGATLE5W0&e=
> > From "Table A3-1 Alignment requirements of load/store instructions"
> > it's seen that LDREXD, STREXD instructions will cause alignment fault
> > even if SCTLR.A=0 (strict alignment fault checking disabled) for non
> > double-word-aligned data.
> 
> Thanks for the better explaination, that helps out a lot.  Can you redo
> the patch with all of that information so that others do not have the
> same questions as I did?

Sure will do it soonish.

-Alexey