linux-kernel - Re: [PATCH v2] blk: improve order of bio handling in generic_make

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87shmkfxkz.fsf@notabene.neil.brown.name>
Date:   Sat, 11 Mar 2017 11:47:56 +1100
From:   NeilBrown <neilb@...e.com>
To:     Lars Ellenberg <lars.ellenberg@...bit.com>
Cc:     Jens Axboe <axboe@...nel.dk>,
        Jack Wang <jinpu.wang@...fitbricks.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Kent Overstreet <kent.overstreet@...il.com>,
        Pavel Machek <pavel@....cz>, Mike Snitzer <snitzer@...hat.com>,
        Mikulas Patocka <mpatocka@...hat.com>,
        linux-raid@...r.kernel.org,
        device-mapper development <dm-devel@...hat.com>,
        linux-block@...r.kernel.org
Subject: Re: [PATCH v2] blk: improve order of bio handling in generic_make_request()

On Fri, Mar 10 2017, Lars Ellenberg wrote:

>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1975,7 +1975,14 @@ generic_make_request_checks(struct bio *bio)
>>   */
>>  blk_qc_t generic_make_request(struct bio *bio)
>>  {
>> -       struct bio_list bio_list_on_stack;
>> +       /*
>> +        * bio_list_on_stack[0] contains bios submitted by the current
>> +        * make_request_fn.
>> +        * bio_list_on_stack[1] contains bios that were submitted before
>> +        * the current make_request_fn, but that haven't been processed
>> +        * yet.
>> +        */
>> +       struct bio_list bio_list_on_stack[2];
>>         blk_qc_t ret = BLK_QC_T_NONE;
>
> May I suggest that, if you intend to assign something that is not a
> plain &(struct bio_list), but a &(struct bio_list[2]),
> you change the task member so it is renamed (current->bio_list vs
> current->bio_lists, plural, is what I did last year).
> Or you will break external modules, silently, and horribly (or,
> rather, they won't notice, but break the kernel).
> Examples of such modules would be DRBD, ZFS, quite possibly others.
>

This is exactly what I didn't in my first draft (bio_list -> bio_lists),
but then I reverted that change because it didn't seem to be worth the
noise.
It isn't much noise, sched.h, bcache/btree.c, md/dm-bufio.c, and
md/raid1.c get minor changes.
But as I'm hoping to get rid of all of those uses, renaming before
removing seemed pointless ... though admittedly that is what I did for
bioset_create().... I wondered about that too.

The example you give later:
	struct bio_list *tmp = current->bio_list;
	current->bio_list = NULL;
	submit_bio()
	current->bio_list = tmp;

won't cause any problem.  Whatever lists the parent generic_make_request
is holding onto will be untouched during the submit_bio() call, and will
be exactly as it expects them when this caller returns.

If some out-of-tree code does anything with ->bio_list that makes sense
with the previous code, then it will still make sense with the new
code. However there will be a few bios that it didn't get too look at.
These will all be bios that were submitted by a device further up the
stack (closer to the filesystem), so they *should* be irrelevant.
I could probably come up with some weird behaviour that might have
worked before but now wouldn't quite work the same way.  But just fixing
bugs can sometimes affect an out-of-tree driver in a strange way because
it was assuming those bugs.

I hope that I'll soon be able to remove punt_bios_to_rescuer and
flush_current_bio_list, after which current->bio_list  can really be
just a list again.  I don't think it is worth changing the name for a
transient situation.

But thanks for the review - it encouraged me to think though the
consequences again and I'm now more confident.
I actually now think that change probably wasn't necessary.  It is
safer though.  It ensures that current functionality isn't removed
without a clear justification.

Thanks,
NeilBrown

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)