linux-kernel - Re: [RFC] weird semantics of SG_DXFER_TO_FROM_DEV in BLK_DEV

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] [day] [month] [year] [list]

Message-ID: <20160408191935.GB25498@ZenIV.linux.org.uk>
Date:	Fri, 8 Apr 2016 20:19:37 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Christoph Hellwig <hch@...radead.org>
Cc:	Jens Axboe <axboe@...com>, linux-kernel@...r.kernel.org,
	linux-block@...r.kernel.org
Subject: Re: [RFC] weird semantics of SG_DXFER_TO_FROM_DEV in BLK_DEV_SKD
 (drivers/block/skd*)

On Thu, Apr 07, 2016 at 08:55:33AM -0700, Christoph Hellwig wrote:
> On Tue, Apr 05, 2016 at 12:45:08AM +0100, Al Viro wrote:
> > AFAICS, what we need there is simply
> > 	nr_pages = iov_iter_npages(iter);
> > 	alignment = iov_iter_alignment(iter);
> > 	if (alignment & (queue_dma_alignment(q) | q->dma_pad_mask))
> > 		copy = true;
> > and I really wonder if we care about special-casing the situation when the
> > ends are not aligned to queue_virt_boundary(q).  If we don't, we might as
> > well add queue_virt_boundary(q) to the mask we are checking.  If we do,
> > it's not hard to add a variant that would calculate both the alignment and
> > alignment for internal boundaries...
> 
> I suspect this is the right thing to do.  Care to send a patch to Jens?

OK...  The interesting part is the calling conventions - we could either
return a pair (minimal alignment, minimal alignment of inner boundaries) or
do something like iov_iter_aligned(iter, all, inner).  The thing is,
iov_iter_alignment() has a couple of users that do not fit well into the
latter model, the worst one being in do_blockdev_direct_IO().

OTOH, the cost of walking the array of iovecs the second time is going to
be trivial - it's already in cache, so something like

unsigned long iov_iter_gap_alignment(struct iov_iter *i)
{
	unsigned long res = 0;
	size_t size = i->i_count;
	iterate_all_kinds(i, size, v,
		(res |= (!res ? 0 : (unsigned long)v.iov_base) |
			(size != v.iov_len ? size : 0), 0),
		(res |= (!res ? 0 : (unsigned long)v.bv_offset) |
			(size != v.bv_len ? size : 0)),
		(res |= (!res ? 0 : (unsigned long)v.iov_base) |
			(size != v.iov_len ? size : 0))
	);
	return res;
}

in addition to iov_iter_alignment() might be the least painful approach...

I still wonder what would be the legitimate cause for stronger alignment
requirements on the gaps than on the ends, though - could you explain why
virt_boundary_mask is there in the first place?  Do we really have drivers
that can take weakly aligned single segment, but can't take the same
alignment between the segments?