linux-kernel - Re: [GIT PULL] first round of SCSI updates for the 5.14+ merge window

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <18fc05ae-d167-9eee-de16-55223705819d@kernel.dk>
Date:   Thu, 2 Sep 2021 17:32:18 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     James Bottomley <James.Bottomley@...senPartnership.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-scsi <linux-scsi@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Christoph Hellwig <hch@...radead.org>
Subject: Re: [GIT PULL] first round of SCSI updates for the 5.14+ merge window

On 9/2/21 5:23 PM, James Bottomley wrote:
> On Thu, 2021-09-02 at 15:38 -0700, Linus Torvalds wrote:
>> On Thu, Sep 2, 2021 at 9:50 AM James Bottomley
>> <James.Bottomley@...senpartnership.com> wrote:
>>> We also picked up a non trivial conflict with the already upstream
>>> block tree in st.c
>>
>> Hmm. Resolving that conflict, I just reacted to how the st.c code
>> passes in a NULL gendisk to scsi_ioctl() and then on to
>> blk_execute_rq().
>>
>> Just checking that was fine, and I notice how *many* places do that.
>>
>> Should the blk_execute_rq() function even take that "struct gendisk
>> *bd_disk" argument at all?
>>
>> Maybe the right thing to do would be for the people who care to just
>> set rq->rq_disk before starting the request..
>>
>> But I guess it's traditional, and nobody cares.
> 
> It's certainly traditional, but Christoph has been caring a lot about
> cleaning up our gendisks recently, so he might be interested in seeing
> if he can fix it ...

It's trivially doable, it's more a question of whether it's a good idea
or not... At least when passed in you have to make a conscious decision
on it being NULL. And just like Linus did, it's something you'll notice
and see what other callers are doing.

As I said, I don't feel that strongly about it. Hopefully most cases
that would forget to set it would trigger a NULL defer when they test
their code, and the core does initialize ->rq_disk to NULL. It's less
risky now than earlier, where the request alloc path was less
streamlined.

-- 
Jens Axboe