linux-kernel - Re: [PATCH v5 4/5] cramfs: add mmap support

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]

Message-ID: <20171006070038.GA29142@infradead.org>
Date:   Fri, 6 Oct 2017 00:00:38 -0700
From:   Christoph Hellwig <hch@...radead.org>
To:     Nicolas Pitre <nicolas.pitre@...aro.org>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Christoph Hellwig <hch@...radead.org>,
        linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
        linux-embedded@...r.kernel.org, linux-kernel@...r.kernel.org,
        Chris Brandt <Chris.Brandt@...esas.com>
Subject: Re: [PATCH v5 4/5] cramfs: add mmap support

> +	/* Don't map the last page if it contains some other data */
> +	if (unlikely(pgoff + pages == max_pages)) {
> +		unsigned int partial = offset_in_page(inode->i_size);
> +		if (partial) {
> +			char *data = sbi->linear_virt_addr + offset;
> +			data += (max_pages - 1) * PAGE_SIZE + partial;
> +			if (memchr_inv(data, 0, PAGE_SIZE - partial) != NULL) {
> +				pr_debug("mmap: %s: last page is shared\n",
> +					 file_dentry(file)->d_name.name);
> +				pages--;
> +			}
> +		}
> +	}

Why is pgoff + pages == max_pages marked unlikely?  Mapping the whole
file seems like a perfectly normal and likely case to me..

Also if this was my code I'd really prefer to move this into a helper:

static bool cramfs_mmap_last_page_is_shared(struct inode *inode, int offset)
{
	unsigned int partial = offset_in_page(inode->i_size);
	char *data = CRAMFS_SB(inode->i_sb)->linear_virt_addr + offset +
			(inode->i_size & PAGE_MASK);

	return memchr_inv(data + partial, 0, PAGE_SIZE - partial);
}

	if (pgoff + pages == max_pages && offset_in_page(inode->i_size)	&&
	    cramfs_mmap_last_page_is_shared(inode, offset))
		pages--;

as that's much more readable and the function name provides a good
documentation of what is going on.

> +	if (pages != vma_pages(vma)) {

here is how I would turn this around:

	if (!pages)
		goto done;

	if (pages == vma_pages(vma)) {
		remap_pfn_range();
		goto done;
	}

	...
	for (i = 0; i < pages; i++) {
		...
		vm_insert_mixed();
		nr_mapped++;
	}


done:
	pr_debug("mapped %d out ouf %d\n", ..);
	if (pages != vma_pages(vma))
		vma->vm_ops = &generic_file_vm_ops;
	return 0;
}

In fact we probably could just set the vm_ops unconditionally, they
just wouldn't be called, but that might be more confusing then helpful.