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 12:10:35 -0400
From:   Sinan Kaya <okaya@...eaurora.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Brian King <brking@...ibm.com>,
        "James E.J. Bottomley" <jejb@...ux.vnet.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        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 <linux-scsi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Will Deacon <will.deacon@....com>
Subject: Re: [PATCH] scsi: ipr: fix build on 32-bit architectures

On 6/8/2018 11:47 AM, Arnd Bergmann wrote:
> On Fri, Jun 8, 2018 at 5:27 PM, Sinan Kaya <okaya@...eaurora.org> wrote:
>> +Will,
>>

[snip]

>> 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.
> 
> If the wmb() was not just about the writeq() then I would argue your patch
> description was misleading. We certainly shouldn't replace random writeq()
> calls with writeq_relaxed() just because we can show that the driver has
> a barrier in front of it.
> 
> In particular, the ipr_mask_and_clear_interrupts() function has multiple
> writeq() or writel() calls, and even a readl() and your patch only changes
> one of them, which seems like a rather pointless exercise as the function
> still fully synchronizes the I/O multiple times.

You are right, I only searched wmb() + writel() combinations. Changed only
the places where I found issues. 

We were given a direction to go to eliminating barriers direction as you already
noted. 

I just wanted to highlight the difficulty of wholesale dropping of wmb() without
careful inspection. [1] [2]

We certainly need a better patch that covers all use cases. Your patch is
a step in the good direction. We just need some attention from the maintainer
that we are not actually breaking something.

> 
>> Will Deacon should probably look at why writeq_relaxed is missing on some ARM
>> arches too.
>>
>> Drivers shouldn't worry about write derivatives.
> 
> This driver defines writeq() itself for architectures that don't have it, but
> it doesn't define writeq_relaxed() and doesn't include
> linux/io-64-nonatomic-lo-hi.h
> or linux/io-64-nonatomic-hi-lo.h. It seems that it needs a different behavior
> from all other drivers here, storing the upper 32 bits into the lower
> address and
> the lower 32 bits into the upper address.

I don't think there is a consensus about using these includes in the community.
I bumped into this issue before and came up with an include you pointed.
I didn't get too much enthusiasm from the maintainer.

Why are we pushing the responsibility into the drivers? I'd think that architecture
should take care of this. Is there a portability issue that I'm missing from some
architecture I never heart of? (I work on Little-Endian machines most of the time)

[1] https://patchwork.kernel.org/patch/10301861/
[2] https://www.mail-archive.com/netdev@vger.kernel.org/msg227443.html

> 
>          Arnd
> 


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