Messages in this thread | | | Date | Thu, 26 Sep 2013 14:48:10 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH 5/7] staging: usbip: Add encryption support to kernel |
| |
On Thu, Sep 26, 2013 at 12:18:34PM +0200, Dominik Paulus wrote: > > I think a return of zero should mean total = -EBADMSG;. In other words > > this check should be "if (ret < 0) {" and we hit the next else if. > > Same below again. > > As we are wrapping kernel_recvmsg here, we wanted to leave the semantics > intact as far as possible. The calling code already checks for the correct > size.
Hm... Ok. Sometimes zero is interpretted as a connection closed and sometimes reading less than expected is considered a TCP error.
> No, currently, the caller (usbip_sendmsg() / usbip_recvmsg() are the > only functions calling usbip_crypt(), which itself is static) ensures > this. > Admittedly, this isn't great design. We added a check for packetsize < > USBIP_AUTHSIZE and an appropiate return here. > > > > + if (encrypt) > > > + ret = crypto_aead_encrypt(req); > > > + else > > > + ret = crypto_aead_decrypt(req); > > > + > > > > Good on you for figuring out what crypto_aead_en/decrypt() returns. > > Where are these functions documented? > > > > > + switch (ret) { > > > + case 0: /* Success */ > > > + break; > > > + case -EINPROGRESS: > > > + case -EBUSY: > > > + wait_for_completion(&result.completion); > > > + break; > > > + default: > > > + aead_request_free(req); > > > + return ret; > > > + } > > > + > > They aren't, actually. Documentation/crypto/api-intro.txt refers to the > regression test module, which uses exactly those return-values in > crypto/testmgr.c.
Well that sucks.
> We noticed that wait_for_completion might not be the best idea, since it could > hang indefinitely, testmgr.c uses wait_for_completion_interruptible. Do we > want 'interruptible' or 'killable' here?
I think you want the interruptible one wait_for_completion_interruptible()
regards, dan carpenter
| |