linux-kernel - Re: [PATCH 05/13] x86, dax: replace clear_pmem() with open coded memset + dax

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, 20 Jan 2017 07:33:02 -0800
From:   Dan Williams <dan.j.williams@...el.com>
To:     Jan Kara <jack@...e.cz>
Cc:     Matthew Wilcox <mawilcox@...rosoft.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 05/13] x86, dax: replace clear_pmem() with open coded
 memset + dax_ops->flush

On Fri, Jan 20, 2017 at 2:27 AM, Jan Kara <jack@...e.cz> wrote:
> On Thu 19-01-17 19:50:39, Dan Williams wrote:
>> The clear_pmem() helper simply combines a memset() plus a cache flush.
>> Now that the flush routine is optionally provided by the dax device
>> driver we can avoid unnecessary cache management on dax devices fronting
>> volatile memory.
>>
>> With clear_pmem() gone we can follow on with a patch to make pmem cache
>> management completely defined within the pmem driver.
> ...
>> diff --git a/fs/dax.c b/fs/dax.c
>> index 160024e403f6..8883ce4d391e 100644
>> --- a/fs/dax.c
>> +++ b/fs/dax.c
>> @@ -986,6 +986,7 @@ static bool dax_range_is_aligned(struct block_device *bdev,
>>  int __dax_zero_page_range(struct block_device *bdev, sector_t sector,
>>               unsigned int offset, unsigned int length)
>>  {
>> +     const struct dax_operations *dax_ops = to_dax_ops(bdev);
>>       struct blk_dax_ctl dax = {
>>               .sector         = sector,
>>               .size           = PAGE_SIZE,
>> @@ -999,7 +1000,9 @@ int __dax_zero_page_range(struct block_device *bdev, sector_t sector,
>>       } else {
>>               if (dax_map_atomic(bdev, &dax) < 0)
>>                       return PTR_ERR(dax.addr);
>> -             clear_pmem(dax.addr + offset, length);
>> +             memset(dax.addr + offset, 0, length);
>> +             if (dax_ops->flush)
>> +                     dax_ops->flush(dax.addr + offset, length);
>>               dax_unmap_atomic(bdev, &dax);
>>       }
>>       return 0;
>
> Shouldn't we rather have some callback in dax_ops for clearing memory?
> If we had all accesses to persistent memory inside DAX code wrapped inside
> appropriate device wrappers that can report errors, we can have proper
> error handling for the case we hit MCE, can't we?

That's true. I was thinking along those lines in the badblocks thread
and then missed that I did not give clearing pmem the same treatment.
No objections from me, I'll add ->clear() to dax_ops.