Re: [PATCH] block: bio-integrity: fix potential null-ptr-deref in bio_integrity_free

Re: [PATCH] block: bio-integrity: fix potential null-ptr-deref in bio_integrity_free

[Ming Lei]

On Thu, Jun 06, 2024 at 07:34:14PM +0800, yebin wrote:
> 
> 
> On 2024/6/6 14:44, Christoph Hellwig wrote:
> > What kernel is this on?  As of Linux 6.9 we are now always freezing
> v4.18
> > the queue while updating the logical_block_size in the nvme driver,
> > so there should be no inflight I/O while it is changing.
> > 
> The root cause of the problem is that there is no concurrency protection
> between
> issuing DIO checks in __ blkdev direct IO simple() and updating logical
> block sizes ,
> resulting in the block layer being able to see DIOs that are not aligned
> with logical
> blocks.

Yeah, that is one area queue freezing can't cover logical block size
change, but I'd suggest to put the logical bs check into submit_bio() or
slow path of __bio_queue_enter() at least.

BTW, Yi has one reproducer, and slab is corrupted just like this report
when running 'nvme format' & IO on partitions.

I am not sure if this kind of change can avoid the issue completely, anyway
Yi and I can test it and see if the kind of change works.

My concern is that nvme format is started without draining IO, and
IO can be submitted to hardware when nvme FW is handling formatting.
I am not sure if nvme FW can deal with this situation correctly.
Ewan suggested to run 'nvme format' with exclusive nvme disk open, which
needs nvme-cli change.



Thanks,
Ming

Re: [PATCH] block: bio-integrity: fix potential null-ptr-deref in bio_integrity_free

[Christoph Hellwig]

On Thu, Jun 06, 2024 at 10:30:06PM +0800, Ming Lei wrote:
> Yeah, that is one area queue freezing can't cover logical block size
> change, but I'd suggest to put the logical bs check into submit_bio() or
> slow path of __bio_queue_enter() at least.

We really need an alignment check in submit_bio anyway, so doing it
under the freeze protection would also help with this.

> My concern is that nvme format is started without draining IO, and
> IO can be submitted to hardware when nvme FW is handling formatting.
> I am not sure if nvme FW can deal with this situation correctly.
> Ewan suggested to run 'nvme format' with exclusive nvme disk open, which
> needs nvme-cli change.

.. and doesn't protect against someone using a different tool anyway.

That beeing said, nvme_passthru_start actually freezes all queues
based on the commands supported an affects log, and
nvme_init_known_nvm_effects should force this even for controllers
not supporting the log or reporting bogus information.  So in general
the queue should be frozen during the actual format.

Re: [PATCH] block: bio-integrity: fix potential null-ptr-deref in bio_integrity_free

[Keith Busch]

On Thu, Jun 06, 2024 at 07:52:51AM -0700, Christoph Hellwig wrote:
> On Thu, Jun 06, 2024 at 10:30:06PM +0800, Ming Lei wrote:
> > Yeah, that is one area queue freezing can't cover logical block size
> > change, but I'd suggest to put the logical bs check into submit_bio() or
> > slow path of __bio_queue_enter() at least.
> 
> We really need an alignment check in submit_bio anyway, so doing it
> under the freeze protection would also help with this.
> 
> > My concern is that nvme format is started without draining IO, and
> > IO can be submitted to hardware when nvme FW is handling formatting.
> > I am not sure if nvme FW can deal with this situation correctly.
> > Ewan suggested to run 'nvme format' with exclusive nvme disk open, which
> > needs nvme-cli change.
> 
> .. and doesn't protect against someone using a different tool anyway.

It also doesn't work if format is sent from a different host. If you
have a shared namespace, anyone could change the format without
coordinating with anyone else.

That's just an unfixable gap with the current spec. The driver tries to
react to the Namespace Notice AEN for this, but that can be too late or
not even enabled.

> That beeing said, nvme_passthru_start actually freezes all queues
> based on the commands supported an affects log, and
> nvme_init_known_nvm_effects should force this even for controllers
> not supporting the log or reporting bogus information.  So in general
> the queue should be frozen during the actual format.

Just for single host consideration, the our current nvme freeze does
ensure IO is drained before dispatching the format, and should work for
most format changes.

Re: [PATCH] block: bio-integrity: fix potential null-ptr-deref in bio_integrity_free

[Ming Lei]

On Thu, Jun 06, 2024 at 07:52:51AM -0700, Christoph Hellwig wrote:
> On Thu, Jun 06, 2024 at 10:30:06PM +0800, Ming Lei wrote:
> > Yeah, that is one area queue freezing can't cover logical block size
> > change, but I'd suggest to put the logical bs check into submit_bio() or
> > slow path of __bio_queue_enter() at least.
> 
> We really need an alignment check in submit_bio anyway, so doing it
> under the freeze protection would also help with this.
> 
> > My concern is that nvme format is started without draining IO, and
> > IO can be submitted to hardware when nvme FW is handling formatting.
> > I am not sure if nvme FW can deal with this situation correctly.
> > Ewan suggested to run 'nvme format' with exclusive nvme disk open, which
> > needs nvme-cli change.
> 
> .. and doesn't protect against someone using a different tool anyway.
> 
> That beeing said, nvme_passthru_start actually freezes all queues
> based on the commands supported an affects log, and
> nvme_init_known_nvm_effects should force this even for controllers
> not supporting the log or reporting bogus information.  So in general
> the queue should be frozen during the actual format.

That is something I missed, thanks for sharing the story.


Thanks,
Ming

Re: [PATCH] block: bio-integrity: fix potential null-ptr-deref in bio_integrity_free

[Markus Elfring]

…
> The root cause of this issue is the concurrency between the write process
> and the block size update process. However, this concurrency does not exist
> in actual production environments. To solve above issue, Verify if the
> segments of BIO are aligned with integrity intervals.

* Please improve the change description with an imperative wording.
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc2#n94

* Would you like to add the tag “Fixes” accordingly?

* How do you think about to use the summary phrase “Avoid null pointer dereference
  in bio_integrity_free()”?


Regards,
Markus