Re: [PATCH] crypto: algif_aead - stop recvmsg looping after a completed request

[cyper@tutanota.com]

> The point of this loop is to wait for a new sendmsg on the socket
> which is supposed to set ctx->init to true again. So are you saying
> that even a new sendmsg cannot get out of this wait?

Thanks -- you're right about the mechanism, and my changelog was
imprecise. Let me correct it with what actually happens.

A new sendmsg *does* get out of that particular wait: it sets
ctx->init = true and af_alg_data_wakeup() wakes the sleeper, so
af_alg_wait_for_data() returns and _aead_recvmsg() processes the new
request. What it does *not* do is make the recvmsg() return: the
"while (msg_data_left(msg))" loop only stops once the output buffer is
full, so after processing the new request it just loops back and blocks
again in af_alg_wait_for_data() for the *next* one.

So the blocking read() as a whole never completes until the caller sends
enough separate requests to fill the entire output buffer (or a signal
arrives). I confirmed this on a live socket with stock gcm(aes), one
initial request (48B PT -> 64B result) and a 256-byte read buffer, using
a helper thread that sends additional full requests:

  extra sendmsgs | read() returns   | elapsed
  ---------------+------------------+------------------------------
   0             | 64 (watchdog)    | 4.00s  hung
   1             | 128 (watchdog)   | 4.00s  hung  (advanced, re-blocked)
   3 (fills 256) | 256              | 1.80s  returned on its own

i.e. one extra sendmsg advanced the result 64 -> 128 and then blocked
again; the read() only returned by itself once four requests had filled
the 256-byte buffer exactly. (The "elapsed 4.00s" rows returned only
because a 4s alarm interrupted the sleep, which makes the loop bail out
returning the bytes accumulated so far -- that is also why a signal or
strace made my original repro "work".)

So my one-line summary "hangs forever" was wrong; the accurate statement
is: a blocking recvmsg() into a buffer larger than the data the caller
intends to send does not return -- it waits to fill the rest of the
buffer from further requests that a one-shot caller has no reason to
send (it did not set MSG_MORE).

Why I still think this is worth fixing rather than "size your buffer":
it breaks the poll()/read() contract. After a non-MSG_MORE request is
consumed, af_alg_poll() reports EPOLLIN:

        if (!ctx->more || ctx->used)
                mask |= EPOLLIN | EPOLLRDNORM;

(!ctx->more is true), so a poll()-driven caller is told the socket is
readable, but the subsequent blocking read() then sleeps in
af_alg_wait_for_data() instead of returning the data poll() promised.
Same test:

        sendmsg = 48
        poll rc=1 revents=0x1 POLLIN=1
        read=64 elapsed=4.00s
        -> poll() said POLLIN but read() blocked

That is what libkcapi/OpenSSL/cryptsetup avoid today only by always
sizing the RX buffer exactly to the expected output; anything larger
trips it.

On the fix itself: stopping the loop once a non-MSG_MORE request is fully
consumed (ctx->more == 0 && ctx->used == 0) turns that case into a normal
short read, which is what poll() already advertises. It deliberately does
not change:

  - MSG_MORE streaming: ctx->more != 0 -> no break, keeps waiting for the
    rest of the message;
  - partial / AIO output: _*_recvmsg() leaves ctx->used > 0 -> no break;
  - the -EIOCBQUEUED / -EBADMSG paths: handled by the existing err <= 0
    branch before the new check (the check is only reached after
    "ret += err", i.e. after a pass that made forward progress).

The only behaviour it removes is coalescing *multiple independent*
non-MSG_MORE requests into a single oversized read(). That case is not
something a caller can reach or rely on deterministically:

  - A single thread cannot even set it up: after one non-MSG_MORE request
    the next sendmsg hits the "ctx->init && !ctx->more" gate with
    ctx->used != 0 and fails with -EINVAL (verified). Coalescing is only
    reachable if a *second* context sends further requests while the first
    is blocked inside read() draining ctx->used.

  - How many requests get coalesced then depends purely on scheduling (how
    much of the buffer is filled before the next send lands), so the
    result length is nondeterministic -- not an API contract anything can
    depend on.

  - For AEAD it is meaningless anyway: each request has its own tag and
    independent result, so splitting them across separate reads loses
    nothing.

With the fix that pattern returns a short read (the first request's
output) and the caller reads again -- which is exactly what poll() already
tells it to do. No single-threaded, MSG_MORE, or poll()/nonblock-driven
caller sees any change.

If you'd prefer, I can respin with the changelog rewritten around the
poll()/read() inconsistency (which is the concrete, indefensible part),
or take a different approach if you had one in mind. Happy to add a
selftest as well.

Reproducers (single-threaded hang, the poll test, and the multi-thread
sendmsg test above) are available if useful.

Thanks,
Qiguang