Path: csiph.com!eternal-september.org!feeder.eternal-september.org!reader01.eternal-september.org!.POSTED!not-for-mail From: Keith Thompson Newsgroups: comp.lang.c Subject: Re: "...the value becomes,indeterminate each time the declaration is reached" Date: Fri, 01 Feb 2019 16:41:11 -0800 Organization: None to speak of Lines: 77 Message-ID: References: <5c51d4fb.236542547@news.xmission.com> <20190201132618.6ed57eb670bad5d4400ccbc8@g{oogle}mail.com> <9MY4E.238695$Y32.184604@fx19.am4> <87tvhnnpmn.fsf@bsb.me.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: reader02.eternal-september.org; posting-host="9763a879bb12a7baac07ce2c771655df"; logging-data="30999"; mail-complaints-to="abuse@eternal-september.org"; posting-account="U2FsdGVkX1+y30XwsKS7tRqYd9ATEQ5z" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) Cancel-Lock: sha1:TG8uNVKd8tQel2iUlXQTnZou+0U= sha1:2jE6lz8Mk9+6hYwi18AzDo4rXvY= Xref: csiph.com comp.lang.c:140467 Ben Bacarisse writes: [...] > but then I thought that the example is probably over-simplified. Indeed > it is. The code is actually: > > static int queue_manag(void *data) > { > cpg->eng_st = ENGINE_IDLE; > do { > struct crypto_async_request *async_req = NULL; > struct crypto_async_request *backlog; > > __set_current_state(TASK_INTERRUPTIBLE); > > if (cpg->eng_st == ENGINE_W_DEQUEUE) > dequeue_complete_req(); > > spin_lock_irq(&cpg->lock); > if (cpg->eng_st == ENGINE_IDLE) { > backlog = crypto_get_backlog(&cpg->queue); > async_req = crypto_dequeue_request(&cpg->queue); > if (async_req) { > BUG_ON(cpg->eng_st != ENGINE_IDLE); > cpg->eng_st = ENGINE_BUSY; > } > } > spin_unlock_irq(&cpg->lock); > > if (backlog) { > backlog->complete(backlog, -EINPROGRESS); > backlog = NULL; > } > > if (async_req) { > if (crypto_tfm_alg_type(async_req->tfm) != > CRYPTO_ALG_TYPE_AHASH) { > struct ablkcipher_request *req = > ablkcipher_request_cast(async_req); > mv_start_new_crypt_req(req); > } else { > struct ahash_request *req = > ahash_request_cast(async_req); > mv_start_new_hash_req(req); > } > async_req = NULL; > } > > schedule(); > > } while (!kthread_should_stop()); > return 0; > } > > The real problem is all the shared global state that makes reasoning > about this code almost impossible, but I assume that is usual in this > sort of driver. (If it usual, I want to ask why that is so in 2019, but > that's another topic!) > > cpg->eng_st is set to ENGINE_IDLE so the code /looks/ fine from the > single-threaded point of view. Presumably some other thread can come > along and alter cpg->eng_s. but then I'd imagine all kinds of possible > messes because lots of the code manipulates *cpg in ways that don't > appear to be thread safe. At first glance it look like the call to > dequeue_complete_req should really be locked. cpg->eng_st is set to ENGINE_IDLE before the loop starts, but it can be set to ENGINE_BUSY in the body of the loop, which will cause an uninitialized pointer (backlog) to be accessed on a later iteration. It invokes BUG_ON() before setting cpg->eng_st to ENGINE_BUSY, implying that it's something that shouldn't normally happen, but it still needs to handle it sanely. -- Keith Thompson (The_Other_Keith) kst-u@mib.org Will write code for food. void Void(void) { Void(); } /* The recursive call of the void */