The coverity checker found this. I don't know how to fix it, so I'll just
report it and hope that someone else can address the issue.
In drivers/md/dm-crypt.c:crypt_convert() we have this code:
...
while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
ctx->idx_out < ctx->bio_out->bi_vcnt) {
crypt_alloc_req(cc, ctx);
atomic_inc(&ctx->pending);
r = crypt_convert_block(cc, ctx, this_cc->req);
switch (r) {
/* async */
case -EBUSY:
wait_for_completion(&ctx->restart);
INIT_COMPLETION(ctx->restart);
/* fall through*/
case -EINPROGRESS:
this_cc->req = NULL;
ctx->sector++;
continue;
...
If we take the first pass through the 'while' loop and hit the
'-EINPROGRESS' case of the switch, then the second time around we'll pass
a NULL 'this_cc->req' to 'crypt_convert_block()'. 'crypt_convert_block()'
passes the pointer to 'ablkcipher_request_set_crypt()' which dereferences
it:
...
static inline void ablkcipher_request_set_crypt(
struct ablkcipher_request *req,
struct scatterlist *src, struct scatterlist *dst,
unsigned int nbytes, void *iv)
{
req->src = src;
...
That's going to go "BOOM" - definately no what we want, so we need a fix
somehow...
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
On 02/06/2011 11:31 PM, Jesper Juhl wrote:
> The coverity checker found this. I don't know how to fix it, so I'll just
> report it and hope that someone else can address the issue.
Hi,
can I see the plain output from the coverity check somewhere?
>
> In drivers/md/dm-crypt.c:crypt_convert() we have this code:
> ...
> while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
> ctx->idx_out < ctx->bio_out->bi_vcnt) {
>
> crypt_alloc_req(cc, ctx);
Here in crypt_alloc_req() you have:
struct crypt_cpu *this_cc = this_crypt_config(cc);
if (!this_cc->req)
this_cc->req = mempool_alloc(cc->req_pool, GFP_NOIO);
>
> atomic_inc(&ctx->pending);
>
> r = crypt_convert_block(cc, ctx, this_cc->req);
this_cc is: struct crypt_cpu *this_cc = this_crypt_config(cc);
and because it is always running on the same CPU,
this_cc->req cannot be NULL here, because it was allocated
in crypt_alloc_req().
It is false positive here.
Milan
On Sun, 6 Feb 2011, Milan Broz wrote:
> On 02/06/2011 11:31 PM, Jesper Juhl wrote:
> > The coverity checker found this. I don't know how to fix it, so I'll just
> > report it and hope that someone else can address the issue.
>
> Hi,
> can I see the plain output from the coverity check somewhere?
>
If you have a coverity scan account it is CID 40766. But since you ask I
assume you do not have an account, so I've also pasted the output from
their web interface here :
Checker: FORWARD_NULL
Function: crypt_convert
File: /linux-2.6/drivers/md/dm-crypt.c
...
768 static int crypt_convert(struct crypt_config *cc,
769 struct convert_context *ctx)
770 {
771 struct crypt_cpu *this_cc = this_crypt_config(cc);
772 int r;
773
774 atomic_set(&ctx->pending, 1);
775
At conditional (1): "ctx->idx_in < ctx->bio_in->bi_vcnt" taking true path
At conditional (2): "ctx->idx_out < ctx->bio_out->bi_vcnt" taking true path
776 while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
777 ctx->idx_out < ctx->bio_out->bi_vcnt) {
778
779 crypt_alloc_req(cc, ctx);
780
781 atomic_inc(&ctx->pending);
782
Event var_deref_model: Passing null variable "this_cc->req" to function "crypt_convert_block", which dereferences it. [details]
783 r = crypt_convert_block(cc, ctx, this_cc->req);
784
785 switch (r) {
786 /* async */
787 case -EBUSY:
788 wait_for_completion(&ctx->restart);
789 INIT_COMPLETION(ctx->restart);
790 /* fall through*/
791 case -EINPROGRESS:
Event assign_zero: Assigning: "this_cc->req" = 0.
792 this_cc->req = NULL;
793 ctx->sector++;
794 continue;
795
796 /* sync */
797 case 0:
798 atomic_dec(&ctx->pending);
799 ctx->sector++;
800 cond_resched();
801 continue;
802
803 /* error */
804 default:
805 atomic_dec(&ctx->pending);
806 return r;
807 }
808 }
809
810 return 0;
811 }
...
The "[details]" link above shows this info:
...
692 static int crypt_convert_block(struct crypt_config *cc,
693 struct convert_context *ctx,
694 struct ablkcipher_request *req)
695 {
696 struct bio_vec *bv_in = bio_iovec_idx(ctx->bio_in, ctx->idx_in);
697 struct bio_vec *bv_out = bio_iovec_idx(ctx->bio_out, ctx->idx_out);
698 struct dm_crypt_request *dmreq;
699 u8 *iv;
700 int r = 0;
701
702 dmreq = dmreq_of_req(cc, req);
703 iv = iv_of_dmreq(cc, dmreq);
704
705 dmreq->iv_sector = ctx->sector;
706 dmreq->ctx = ctx;
707 sg_init_table(&dmreq->sg_in, 1);
708 sg_set_page(&dmreq->sg_in, bv_in->bv_page, 1 << SECTOR_SHIFT,
709 bv_in->bv_offset + ctx->offset_in);
710
711 sg_init_table(&dmreq->sg_out, 1);
712 sg_set_page(&dmreq->sg_out, bv_out->bv_page, 1 << SECTOR_SHIFT,
713 bv_out->bv_offset + ctx->offset_out);
714
715 ctx->offset_in += 1 << SECTOR_SHIFT;
716 if (ctx->offset_in >= bv_in->bv_len) {
717 ctx->offset_in = 0;
718 ctx->idx_in++;
719 }
720
721 ctx->offset_out += 1 << SECTOR_SHIFT;
722 if (ctx->offset_out >= bv_out->bv_len) {
723 ctx->offset_out = 0;
724 ctx->idx_out++;
725 }
726
727 if (cc->iv_gen_ops) {
728 r = cc->iv_gen_ops->generator(cc, iv, dmreq);
729 if (r < 0)
730 return r;
731 }
732
Event deref_parm_in_call: Function "ablkcipher_request_set_crypt" dereferences parameter "req". [details]
733 ablkcipher_request_set_crypt(req, &dmreq->sg_in, &dmreq->sg_out,
734 1 << SECTOR_SHIFT, iv);
735
736 if (bio_data_dir(ctx->bio_in) == WRITE)
737 r = crypto_ablkcipher_encrypt(req);
738 else
739 r = crypto_ablkcipher_decrypt(req);
740
741 if (!r && cc->iv_gen_ops && cc->iv_gen_ops->post)
742 r = cc->iv_gen_ops->post(cc, iv, dmreq);
743
744 return r;
745 }
...
And the second "[details]" link gives this:
...
713 static inline void ablkcipher_request_set_crypt(
714 struct ablkcipher_request *req,
715 struct scatterlist *src, struct scatterlist *dst,
716 unsigned int nbytes, void *iv)
717 {
Event deref_parm: Directly dereferencing parameter "req".
718 req->src = src;
719 req->dst = dst;
720 req->nbytes = nbytes;
721 req->info = iv;
722 }
...
> >
> > In drivers/md/dm-crypt.c:crypt_convert() we have this code:
> > ...
> > while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
> > ctx->idx_out < ctx->bio_out->bi_vcnt) {
> >
> > crypt_alloc_req(cc, ctx);
>
> Here in crypt_alloc_req() you have:
>
> struct crypt_cpu *this_cc = this_crypt_config(cc);
> if (!this_cc->req)
> this_cc->req = mempool_alloc(cc->req_pool, GFP_NOIO);
>
> >
> > atomic_inc(&ctx->pending);
> >
> > r = crypt_convert_block(cc, ctx, this_cc->req);
>
> this_cc is: struct crypt_cpu *this_cc = this_crypt_config(cc);
> and because it is always running on the same CPU,
> this_cc->req cannot be NULL here, because it was allocated
> in crypt_alloc_req().
>
> It is false positive here.
>
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
On 02/10/2011 08:14 PM, Jesper Juhl wrote:
> On Sun, 6 Feb 2011, Milan Broz wrote:
>
> If you have a coverity scan account it is CID 40766. But since you ask I
> assume you do not have an account, so I've also pasted the output from
> their web interface here :
Thanks.
I would say that the checker has problem to follow per cpu pointers...
In this case
static struct crypt_cpu *this_crypt_config(struct crypt_config *cc)
{
return this_cpu_ptr(cc->cpu);
Otherwise it must see that the struct is allocated in
crypt_alloc_req(cc, ctx);
And mempool allocation should never return NULL here.
Milan
On Fri, 11 Feb 2011, Milan Broz wrote:
> On 02/10/2011 08:14 PM, Jesper Juhl wrote:
> > On Sun, 6 Feb 2011, Milan Broz wrote:
> >
> > If you have a coverity scan account it is CID 40766. But since you ask I
> > assume you do not have an account, so I've also pasted the output from
> > their web interface here :
>
> Thanks.
>
> I would say that the checker has problem to follow per cpu pointers...
>
> In this case
> static struct crypt_cpu *this_crypt_config(struct crypt_config *cc)
> {
> return this_cpu_ptr(cc->cpu);
>
> Otherwise it must see that the struct is allocated in
> crypt_alloc_req(cc, ctx);
>
> And mempool allocation should never return NULL here.
>
But, is that really where it says the problem is? That's not how I read
it.
The problem is the second time through the while loop, not the first :
...
776 while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
777 ctx->idx_out < ctx->bio_out->bi_vcnt) {
778
779 crypt_alloc_req(cc, ctx);
780
781 atomic_inc(&ctx->pending);
782
783 r = crypt_convert_block(cc, ctx, this_cc->req);
first time through the loop this is fine, but if we then subsequently hit
the -EINPROGRESS case in the switch below we'll explictly assign NULL to
this_cc->req and the the 'continue' ensures that we do one more trip
around the loop and on that second pass we pass a NULL this_cc->req to
crypt_convert_block()
784
785 switch (r) {
786 /* async */
787 case -EBUSY:
788 wait_for_completion(&ctx->restart);
789 INIT_COMPLETION(ctx->restart);
790 /* fall through*/
791 case -EINPROGRESS:
792 this_cc->req = NULL;
793 ctx->sector++;
794 continue;
...
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
On 02/11/2011 10:26 AM, Jesper Juhl wrote:
> On Fri, 11 Feb 2011, Milan Broz wrote:
> But, is that really where it says the problem is? That's not how I read
> it.
>
> The problem is the second time through the while loop, not the first :
> ...
> 776 while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
> 777 ctx->idx_out < ctx->bio_out->bi_vcnt) {
> 778
> 779 crypt_alloc_req(cc, ctx);
> 780
> 781 atomic_inc(&ctx->pending);
> 782
> 783 r = crypt_convert_block(cc, ctx, this_cc->req);
>
> first time through the loop this is fine, but if we then subsequently hit
> the -EINPROGRESS case in the switch below we'll explictly assign NULL to
> this_cc->req and the the 'continue' ensures that we do one more trip
> around the loop and on that second pass we pass a NULL this_cc->req to
> crypt_convert_block()
>
Sigh. Did you read my first email? It is false positive.
this_cc->req is allocated in crypt_alloc_req(cc, ctx);
this_cc is simple per cpu struct, common in both functions.
The code here tries to simply support both sync and async cryptAPI operation.
In sync, we can reuse this_cc->req immediately (this is common case).
In async mode (returns EBUSY, EINPROGRESS) we must not use it again (because it is
still processing) so we explicitly set it here to NULL and in the NEXT iteration
crypt_alloc_req(cc, ctx) allocates new this_cc->req from pool.
crypt_alloc_req can probably take this_cc as argument directly and not calculate
it again, but compiler will inline and optimise the code anyway.
You can easily test async path, just apply in crypt_ctr and use some crypt mapping
- "%s(%s)", chainmode, cipher);
+ "cryptd(%s(%s-generic))", chainmode, cipher);
To make coverity happy, see patch below.
-
Tidy code to reuse per cpu pointer
Signed-off-by: Milan Broz <[email protected]>
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index ad2a6df..a2312e3 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -748,9 +748,9 @@ static void kcryptd_async_done(struct crypto_async_request *async_req,
int error);
static void crypt_alloc_req(struct crypt_config *cc,
- struct convert_context *ctx)
+ struct convert_context *ctx,
+ struct crypt_cpu *this_cc)
{
- struct crypt_cpu *this_cc = this_crypt_config(cc);
unsigned key_index = ctx->sector & (cc->tfms_count - 1);
if (!this_cc->req)
@@ -776,7 +776,7 @@ static int crypt_convert(struct crypt_config *cc,
while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
ctx->idx_out < ctx->bio_out->bi_vcnt) {
- crypt_alloc_req(cc, ctx);
+ crypt_alloc_req(cc, ctx, this_cc);
atomic_inc(&ctx->pending);
Milan
On Fri, 11 Feb 2011, Milan Broz wrote:
> On 02/11/2011 10:26 AM, Jesper Juhl wrote:
> > On Fri, 11 Feb 2011, Milan Broz wrote:
> > But, is that really where it says the problem is? That's not how I read
> > it.
> >
> > The problem is the second time through the while loop, not the first :
> > ...
> > 776 while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
> > 777 ctx->idx_out < ctx->bio_out->bi_vcnt) {
> > 778
> > 779 crypt_alloc_req(cc, ctx);
> > 780
> > 781 atomic_inc(&ctx->pending);
> > 782
> > 783 r = crypt_convert_block(cc, ctx, this_cc->req);
> >
> > first time through the loop this is fine, but if we then subsequently hit
> > the -EINPROGRESS case in the switch below we'll explictly assign NULL to
> > this_cc->req and the the 'continue' ensures that we do one more trip
> > around the loop and on that second pass we pass a NULL this_cc->req to
> > crypt_convert_block()
> >
> Sigh. Did you read my first email? It is false positive.
>
Read it. Obviously misunderstood it. :-(
> this_cc->req is allocated in crypt_alloc_req(cc, ctx);
>
> this_cc is simple per cpu struct, common in both functions.
>
> The code here tries to simply support both sync and async cryptAPI operation.
>
> In sync, we can reuse this_cc->req immediately (this is common case).
>
> In async mode (returns EBUSY, EINPROGRESS) we must not use it again (because it is
> still processing) so we explicitly set it here to NULL and in the NEXT iteration
> crypt_alloc_req(cc, ctx) allocates new this_cc->req from pool.
>
> crypt_alloc_req can probably take this_cc as argument directly and not calculate
> it again, but compiler will inline and optimise the code anyway.
>
> You can easily test async path, just apply in crypt_ctr and use some crypt mapping
> - "%s(%s)", chainmode, cipher);
> + "cryptd(%s(%s-generic))", chainmode, cipher);
>
> To make coverity happy, see patch below.
Thank you for patient explanation.
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html