Re: [PATCH v5 02/12] crypto: acomp - Define new interfaces for compress/decompress batching.

[Sergey Senozhatsky <senozhatsky@chromium.org>]

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.