On (25/02/23 11:38), Herbert Xu wrote:
> On Sun, Feb 23, 2025 at 12:12:47PM +0900, Sergey Senozhatsky wrote:
> >
> > > > It also seems that there is no common way of reporting dst_but overflow.
> > > > Some algos return -ENOSPC immediately, some don't return anything at all,
> > > > and deflate does it's own thing - there are these places where they see
> > > > they are out of out space but they Z_OK it
> > > >
> > > > if (s->pending != 0) {
> > > > flush_pending(strm);
> > > > if (strm->avail_out == 0) {
> > > > /* Since avail_out is 0, deflate will be called again with
> > > > * more output space, but possibly with both pending and
> > > > * avail_in equal to zero. There won't be anything to do,
> > > > * but this is not an error situation so make sure we
> > > > * return OK instead of BUF_ERROR at next call of deflate:
> > > > */
> > > > s->last_flush = -1;
> > > > return Z_OK;
> > > > }
> > > > }
> > >
> > > Z_OK is actually an error, see crypto/deflate.c:
> >
> > I saw Z_STREAM_END, but deflate states "this is not an error" and
> > there are more places like this.
>
> That would be a serious bug in deflate. Where did you see it
> return Z_STREAM_END in case of an overrun or error?
Oh, sorry for the confusion, I was talking about Z_OK for overruns.
> > So it will ENOSPC all errors, not sure how good that is. We also
> > have lz4/lz4hc that return the number of bytes "(((char *)op) - dest)"
> > if successful and 0 otherwise. So any error is 0. dst_buf overrun
> > is also 0, impossible to tell the difference, again not sure if we
> > can just ENOSPC.
>
> I'm talking about the Crypto API calling convention. Individual
> compression libraries obviously have vastly different calling
> conventions.
>
> In the Crypto API, lz4 will return -EINVAL:
>
> int out_len = LZ4_compress_default(src, dst,
> slen, *dlen, ctx);
>
> if (!out_len)
> return -EINVAL;
Right, so you said that for deflate it could be
ret = zlib_deflate(stream, Z_FINISH);
if (ret != Z_STREAM_END) {
ret = -ENOSPC; // and not -EINVAL
goto out;
}
if I understood it correctly. Which would make it: return 0 on success
or -ENOSPC otherwise. So if crypto API wants consistency and return -ENOSPC
for buffer overruns, then for lz4/lz4hc it also becomes binary: either 0 or
-ENOSCP. Current -EINVAL return looks better to me, both for deflate and
for lz4/lz4hc. -ENOSPC is an actionable error code, a user can double the
dst_out size and retry compression etc., while in reality it could be some
SW/HW issue that is misreported as -ENOSPC.
So re-iterating Barry's points:
> My point is:
> 1. All drivers must be capable of handling dst_buf overflow.
Not the case.
> 2. All drivers must return a consistent and dedicated error code for
> dst_buf overflow.
Not the case.
