On Tue, Feb 10, 2015 at 06:01:32PM -0800, Linus Torvalds wrote:
> On Tue, Feb 10, 2015 at 5:45 PM, Al Viro <v...@zeniv.linux.org.uk> wrote:
> >
> > Could you check if
> >
> > diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> > index eb78fe8..5b11d64 100644
> > --- a/crypto/af_alg.c
> > +++ b/crypto/af_alg.c
> > @@ -348,7 +348,7 @@ int af_alg_make_sg(struct af_alg_sgl *sgl, struct
> > iov_iter *iter, int len)
> > if (n < 0)
> > return n;
> >
> > - npages = PAGE_ALIGN(off + n);
> > + npages = DIV_ROUND_UP(off + n, PAGE_SIZE);
> > if (WARN_ON(npages == 0))
> > return -EINVAL;
> >
> > on top of what went into Dave's tree is enough to fix what you are seeing?
>
> So quite frankly, considering that we already know that 'used' was
> also calculated wrong (it stays at zero) and that the one-liner above
> is not sufficient on its own, by now I'd like somebody with an actual
> test-case to check this thing out., and send me a proper tested patch.
> That somebody preferably being you.
>
> Are there no tests for that crypto interface?
I hoped LTP would have them, but it doesn't ;-/ The best I'd been able to
find had been in libkcapi, modulo bunch of
let result=($result + 1)
(in a bash script; replacing with result=$(($result+1)) has dealt with that)
and algif-aead module it supplies and wants to test.
It reproduces the oopsen just fine and patch below seems to fix things.
> Also, I'm unhappy that you made these unrelated and broken changes at
> all. Both wrt 'used' and this 'npages' thing, the original code wasn't
> wrong, and the pointless changes to correct code not only broke
> things, but had nothing to do with the iovec conversion that was the
> stated reason for the commit.
I've fucked up on those, no arguments, but they *were* very much related
to the conversion in question - original code in skcipher_recvmsg()
was a loop over iovec segments, with inner loop over each segment.
With iov_iter conversion we need a flat loop, and I'd screwed up while
massaging the code into that form. And af_alg_make_sg() change had been
a consequence of the same - we used to feed it the next subset of the
range covered by ->msg_iter.iov[...] in the inner loop there.
npages thing is _not_ a replacement of (similar) line you've picked out
of that diff - it replaced
npages = err;
after get_user_pages_fast(). Calling conventions of iov_iter_get_pages()
differ - it returns offset of the beginning within the first page (via
*start) and amount of data it picked from iterator (as return value).
> Grr. I really hate it when I find bugs during the merge window. My
> test environment is *not* odd. If _I_ end up being the one finding the
> bugs, that means that things must have gotten basically no testing at
> all.
Out of curiosity - what has triggered that oops on your userland? I certainly
_did_ the "allmodconfig and see if it boots" on those; in my usual test
image (jessie-amd64 in KVM) nothing has stepped into that one. Again,
I'm not trying to excuse the fuckup - just wondering what to add to tests...
The patch below appears to fix the problems on the reproducer I'd
been able to find.
Signed-off-by: Al Viro <v...@zeniv.linux.org.uk>
---
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index eb78fe8..5b11d64 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -348,7 +348,7 @@ int af_alg_make_sg(struct af_alg_sgl *sgl, struct iov_iter
*iter, int len)
if (n < 0)
return n;
- npages = PAGE_ALIGN(off + n);
+ npages = DIV_ROUND_UP(off + n, PAGE_SIZE);
if (WARN_ON(npages == 0))
return -EINVAL;
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 37110fd..0eb31a6 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -427,11 +427,11 @@ static int skcipher_recvmsg(struct kiocb *unused, struct
socket *sock,
struct skcipher_sg_list *sgl;
struct scatterlist *sg;
int err = -EAGAIN;
- int used;
long copied = 0;
lock_sock(sk);
while (iov_iter_count(&msg->msg_iter)) {
+ int used;
sgl = list_first_entry(&ctx->tsgl,
struct skcipher_sg_list, list);
sg = sgl->sg;
@@ -439,14 +439,13 @@ static int skcipher_recvmsg(struct kiocb *unused, struct
socket *sock,
while (!sg->length)
sg++;
- used = ctx->used;
- if (!used) {
+ if (!ctx->used) {
err = skcipher_wait_for_data(sk, flags);
if (err)
goto unlock;
}
- used = min_t(unsigned long, used,
iov_iter_count(&msg->msg_iter));
+ used = min_t(unsigned long, ctx->used,
iov_iter_count(&msg->msg_iter));
used = af_alg_make_sg(&ctx->rsgl, &msg->msg_iter, used);
err = used;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
