Re: [PATCH v3 2/2] ksmbd: fix named-pipe reply loss with small output buffers

[Namjae Jeon]

> @@ -9294,46 +9307,59 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
>         rpc = ksmbd_session_rpc_get(work, id);
>         if (IS_ERR(rpc)) {
>                 err = PTR_ERR(rpc);
> -               rpc_resp = NULL;
>                 goto out;
>         }
> -       rpc_resp = ksmbd_rpc_read(work->sess, rpc);
> -       if (rpc_resp) {
> -               void *aux_payload_buf;
>
> +       length = le32_to_cpu(req->Length);
> +       if (!length)
> +               goto empty;
> +
> +       if (!rpc->pending) {
> +               rpc_resp = ksmbd_rpc_read(work->sess, rpc);
> +               if (!rpc_resp) {
> +                       err = -EIO;
> +                       goto out_put;
> +               }
>                 if (rpc_resp->flags != KSMBD_RPC_OK) {
> +                       kvfree(rpc_resp);
>                         err = -EINVAL;
>                         goto out_put;
>                 }
> +               rpc->pending = rpc_resp;
> +       }
>
> -               aux_payload_buf =
> -                       kvmalloc(ALIGN(rpc_resp->payload_sz, 8),
> -                                KSMBD_DEFAULT_GFP);
> -               if (!aux_payload_buf) {
> -                       err = -ENOMEM;
> -                       goto out_put;
> -               }
> +       rpc_resp = rpc->pending;
> +       nbytes = min(length, rpc_resp->payload_sz - rpc->pending_offset);
> +       if (!nbytes) {
> +               smb2_pipe_response_done(rpc, 0);
> +               goto empty;
> +       }
>
> -               memcpy(aux_payload_buf, rpc_resp->payload, rpc_resp->payload_sz);
> -               if (rpc_resp->payload_sz & 7)
> -                       memset(aux_payload_buf + rpc_resp->payload_sz, 0,
> -                              ALIGN(rpc_resp->payload_sz, 8) -
> -                              rpc_resp->payload_sz);
> +       aux_payload_buf = kvmalloc(ALIGN(nbytes, 8), KSMBD_DEFAULT_GFP);
> +       if (!aux_payload_buf) {
> +               err = -ENOMEM;
> +               goto out_put;
> +       }
> +       memcpy(aux_payload_buf, rpc_resp->payload + rpc->pending_offset, nbytes);
> +       if (nbytes & 7)
> +               memset(aux_payload_buf + nbytes, 0, ALIGN(nbytes, 8) - nbytes);
>
> -               nbytes = rpc_resp->payload_sz;
> -               err = ksmbd_iov_pin_rsp_read(work, (void *)rsp,
> -                                            offsetof(struct smb2_read_rsp, Buffer),
> -                                            aux_payload_buf, nbytes);
> -               if (err) {
> -                       kvfree(aux_payload_buf);
> -                       goto out_put;
> -               }
> -               kvfree(rpc_resp);
> -       } else {
> -               err = -EIO;
> +       err = ksmbd_iov_pin_rsp_read(work, rsp,
> +                                    offsetof(struct smb2_read_rsp, Buffer),
> +                                    aux_payload_buf, nbytes);
> +       if (err) {
> +               kvfree(aux_payload_buf);
>                 goto out_put;
>         }
> +       smb2_pipe_response_done(rpc, nbytes);

if (rpc->pending)
    rsp->hdr.Status = STATUS_BUFFER_OVERFLOW;
Should we add this check ?

> +       goto done;
>
> +empty:
> +       err = ksmbd_iov_pin_rsp(work, rsp,
> +                               offsetof(struct smb2_read_rsp, Buffer));
> +       if (err)
> +               goto out_put;
> +done:
>         rsp->StructureSize = cpu_to_le16(17);
>         rsp->DataOffset = 80;
>         rsp->Reserved = 0;
> @@ -9349,7 +9375,6 @@ static noinline int smb2_read_pipe(struct ksmbd_work *work)
>         rsp->hdr.Status = smb2_pipe_error_status(err,
>                                                 STATUS_UNEXPECTED_IO_ERROR);
>         smb2_set_err_rsp(work);
> -       kvfree(rpc_resp);
>         return err;
>  }
>
> @@ -9947,7 +9972,7 @@ static noinline int smb2_write_pipe(struct ksmbd_work *work)
>         if (IS_ERR(rpc)) {
>                 err = PTR_ERR(rpc);
>                 rsp->hdr.Status = smb2_pipe_error_status(err,
> -                                                       STATUS_INVALID_HANDLE);
> +                                                        STATUS_INVALID_HANDLE);
>                 smb2_set_err_rsp(work);
>                 return err;
>         }
> @@ -11440,20 +11465,25 @@ static int fsctl_pipe_transceive(struct ksmbd_work *work, u64 id,
>         struct ksmbd_rpc_command *rpc_resp;
>         struct ksmbd_session_rpc *rpc;
>         char *data_buf = (char *)req + le32_to_cpu(req->InputOffset);
> -       unsigned int nbytes;
> -       int err;
> +       unsigned int nbytes = 0;
> +       int ret;
>
>         rpc = ksmbd_session_rpc_get(work, id);
>         if (IS_ERR(rpc)) {
> -               err = PTR_ERR(rpc);
> -               rsp->hdr.Status = smb2_pipe_error_status(err,
> -                                                       STATUS_INVALID_PARAMETER);
> +               ret = PTR_ERR(rpc);
> +               rsp->hdr.Status = smb2_pipe_error_status(ret,
> +                                                STATUS_INVALID_PARAMETER);
>                 smb2_set_err_rsp(work);
>                 return 0;
>         }
>
> -       rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
> -                                  le32_to_cpu(req->InputCount));
> +       /* Write new input without reading past an already buffered reply. */
> +       if (rpc->pending)
> +               rpc_resp = ksmbd_rpc_write(work->sess, rpc, data_buf,
> +                                          le32_to_cpu(req->InputCount));
> +       else
> +               rpc_resp = ksmbd_rpc_ioctl(work->sess, rpc, data_buf,
> +                                          le32_to_cpu(req->InputCount));
This WRITE leaves KSMBD_DCERPC_RETURN_READY set in mountd. After the
cached reply is drained, the next TRANSCEIVE can return the previous
reply without processing its own input.

Thanks.