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  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:	Mon, 24 Nov 2014 10:03:15 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Jason Wang <jasowang@...hat.com>
Cc:	Ben Hutchings <ben@...adent.org.uk>,
	David Miller <davem@...emloft.net>,
	torvalds@...ux-foundation.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, target-devel@...r.kernel.org,
	nab@...ux-iscsi.org, hch@...radead.org
Subject: Re: [PATCH 07/17] new helpers: skb_copy_datagram_from_iter() and
 zerocopy_sg_from_iter()

On Mon, Nov 24, 2014 at 01:34:30PM +0800, Jason Wang wrote:
> >> +		copied = iov_iter_get_pages(from, pages, ~0U, MAX_SKB_FRAGS, &start);

> > Why is this condition needed, given we told iov_iter_get_pages() to
> > limit to MAX_SKB_FRAGS pages?
> 
> We don't want to send truncated packets and there's no other way to put
> those pages since it was not in the frag array.

No, his point is that it could never happen.  It could, actually - what's
confusing here (and that's inherited from zerocopy_from_iovec()) is
that 'i' is a lousy name for that variable.  It's actually "how many fragments
have we already put there?" and it is not reset when we go into the next
iteration of outer loop.

FWIW, I've just renamed it into 'frag', put
	if (frag == MAX_SKB_FRAGS)
		return -EMSGSIZE;
*before* iov_iter_get_pages(), passing MAX_SKB_FRAGS - frag as the
limit on number of pages in that call.  Voila - logics with put_page()
disappears and the inner loop is less obfuscated.

There was another bug in that function - iov_iter_get_pages() does *not*
advance the iterator; the caller needs to do iov_iter_advance() itself.
Also fixed...
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists