linux-kernel - Re: [PATCH] scsi: ipr: fix build on 32-bit architectures

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:   Fri, 8 Jun 2018 11:27:15 -0400
From:   Sinan Kaya <okaya@...eaurora.org>
To:     Arnd Bergmann <arnd@...db.de>, Brian King <brking@...ibm.com>,
        "James E.J. Bottomley" <jejb@...ux.vnet.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>
Cc:     Kees Cook <keescook@...omium.org>, Hannes Reinecke <hare@...e.com>,
        Souptick Joarder <jrdr.linux@...il.com>,
        Wen Xiong <wenxiong@...ux.vnet.ibm.com>,
        Bart Van Assche <bart.vanassche@....com>,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Will Deacon <will.deacon@....com>
Subject: Re: [PATCH] scsi: ipr: fix build on 32-bit architectures

+Will,

On 6/8/2018 10:46 AM, Arnd Bergmann wrote:
> Replacing writeq() with writeq_relaxed() doesn't work on many architectures,
> as that variant is not available in general:
> 
> net/Makefile:24: CC cannot link executables. Skipping bpfilter.
> drivers/scsi/ipr.c: In function 'ipr_mask_and_clear_interrupts':
> drivers/scsi/ipr.c:767:3: error: implicit declaration of function 'writeq_relaxed'; did you mean 'writew_relaxed'? [-Werror=implicit-function-declaration]
>    writeq_relaxed(~0, ioa_cfg->regs.set_interrupt_mask_reg);
>    ^~~~~~~~~~~~~~
>    writew_relaxed
> 
> The other issue here is that the patch eliminated the wrong barrier.
> As per a long discussion that followed Sinan's original patch submission,
> the conclusion was that drivers should generally assume that the barrier
> implied by writel() is sufficient for ordering DMA, so this reverts his
> change and instead removes the extraneous wmb() before it, which is no
> longer needed on any architecture now.
> 
> Fixes: 0109a4f2e02d ("scsi: ipr: Eliminate duplicate barriers on weakly-ordered archs")
> Signed-off-by: Arnd Bergmann <arnd@...db.de>

This looks good on paper however we need an input from the driver maintainer
because some drivers like Intel NIC are using write barriers in place of
a SMP barrier + write barrier combination as an optimizatin.

Removing the barrier itself can actually break the driver if SMP barrier is
actually needed instead.

So, it is difficult to judge how this barrier has been used without an
expert opinion.

Changing 

wmb() + writel()

to 

wmb() + writel_relaxed()

is safer than dropping the wmb() altogether.

Will Deacon should probably look at why writeq_relaxed is missing on some ARM
arches too.

Drivers shouldn't worry about write derivatives.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.