linux-kernel - Re: [PATCH v3 05/15] libnvdimm, pmem: fix size trim in pmem_direct

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151103193223.GD23366@linux.intel.com>
Date:	Tue, 3 Nov 2015 12:32:23 -0700
From:	Ross Zwisler <ross.zwisler@...ux.intel.com>
To:	Dan Williams <dan.j.williams@...el.com>
Cc:	axboe@...com, jack@...e.cz, linux-nvdimm@...ts.01.org,
	david@...morbit.com, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org, ross.zwisler@...ux.intel.com, hch@....de
Subject: Re: [PATCH v3 05/15] libnvdimm, pmem: fix size trim in
 pmem_direct_access()

On Sun, Nov 01, 2015 at 11:30:10PM -0500, Dan Williams wrote:
> This masking prevents access to the end of the device via dax_do_io(),
> and is unnecessary as arch_add_memory() would have rejected an unaligned
> allocation.
> 
> Cc: <stable@...r.kernel.org>
> Cc: Ross Zwisler <ross.zwisler@...ux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> ---
>  drivers/nvdimm/pmem.c |   17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index e46988fbdee5..93472953e231 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -100,26 +100,15 @@ static int pmem_rw_page(struct block_device *bdev, sector_t sector,
>  }
>  
>  static long pmem_direct_access(struct block_device *bdev, sector_t sector,
> -		      void __pmem **kaddr, unsigned long *pfn)
> +		      void __pmem **kaddr, pfn_t *pfn)

It seems kind of weird to change only this instance of direct_access() to have
the last argument as a pfn_t instead of an unsigned long?   If pfn_t is more
descriptive (I think it is) should we update the definition in struct
block_device_operations and all the other implementors of direct_access as
well?  If that's touching too much, let's do them all together later, but
let's not change one now and have them be inconsistent.

>  {
>  	struct pmem_device *pmem = bdev->bd_disk->private_data;
>  	resource_size_t offset = sector * 512 + pmem->data_offset;
> -	resource_size_t size;
>  
> -	if (pmem->data_offset) {
> -		/*
> -		 * Limit the direct_access() size to what is covered by
> -		 * the memmap
> -		 */
> -		size = (pmem->size - offset) & ~ND_PFN_MASK;
> -	} else
> -		size = pmem->size - offset;
> -
> -	/* FIXME convert DAX to comprehend that this mapping has a lifetime */
>  	*kaddr = pmem->virt_addr + offset;
> -	*pfn = (pmem->phys_addr + offset) >> PAGE_SHIFT;
> +	*pfn = __phys_to_pfn(pmem->phys_addr + offset, pmem->pfn_flags);

__phys_to_pfn() only takes a single argument (the paddr) in v4.3,
jens/for-4.4/integrity and in nvdimm/libnvdimm-for-next.  Is this second
argument of pfn_flags actually correct?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/