On Mon, Oct 31, 2022 at 9:37 PM Khazhy Kumykov <khazhy@xxxxxxxxxxxx> wrote: > > I'm investigating a NULL deref crash in bfq_add_bfqq_busy(), wherein > bfqq->woken_list_node is hashed, but bfqq->waker_bfqq is NULL - which > seems inconsistent per my reading of the code. > > Wherein I see bfq_allow_bio_merge() both accesses and modifies > accesses bfqd->bio_bfqq without bfqd->lock, which strikes me as odd. > The call there though to bfq_setup_cooperator and bfq_merge_bfqqs() > seem wrong to me. In particular, the call to bfq_merge_bfqqs() I am > suspecting can cause the inconsistency seen above, since it's the only > place I've found that modifies bfqq->waker_bfqq without bfqd->lock. > > But I'm curious in general - what's special about bio_bfqq? Should we > grab bfqd->lock when touching it? e.g. bfq_request_merge() also > accesses bio_bfqq without grabbing the lock, where-in we traverse > bfqq->sort_list - that strikes me as odd as well, but I'm not fully > familiar with the locking conventions here. But it feels like, > especially since we can merge bfqqs, so bio_bfqq is shared - this > lockless access seems wrong. Something on this front, since it does look like in *some* paths we do call blk_mq_sched_allow_merge()/bfq_allow_bio_merge() with the lock held, and some paths we do not - e.g. blk_mq_sched_try_merge gets called directly by the schedulers (and bfq calls it under the lock). However, blk_attempt_bio_merge also calls blk_mq_sched_allow_merge(), and it's called by blk-mq directly on the submission path (blk_bio_list_merge <- blk_mq_sched_bio_merge <- blk_mq_attempt_bio_merge <- blk_mq_get_new_requests <- blk_mq_submit_bio), and so we'll call bfq_allow_bio_merge without bfqd->lock held in this path only. I can see also for bfq_request_merge(), it gets called under bfqd->lock, since the only path to ->request_merge() is through blk_mq_sched_try_merge() - which is called by the schedulers. If I'm understanding this correctly, and the functions are intended to be called under the locks, perhaps it'd be appropriate to add some lockdep_held annotations? Khazhy
