Re: [dm-devel] [PATCH v5.10] dm: fix mempool NULL pointer race when completing IO

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Apr 28, 2022 at 12:22:26PM -0400, Mikulas Patocka wrote:
> 
> 
> On Tue, 26 Apr 2022, Greg Kroah-Hartman wrote:
> 
> > On Thu, Apr 21, 2022 at 02:08:30PM -0400, Mikulas Patocka wrote:
> > > Hi
> > 
> > Not really needed in a changelog text :)
> > 
> > > This is backport of patches d208b89401e0 ("dm: fix mempool NULL pointer
> > > race when completing IO") and 9f6dc6337610 ("dm: interlock pending dm_io
> > > and dm_wait_for_bios_completion") for the kernel 5.10.
> > 
> > Can you just make these 2 different patches?
> > 
> > > 
> > > The bugs fixed by these patches can cause random crashing when reloading
> > > dm table, so it is eligible for stable backport.
> > > 
> > > This patch is different from the upstream patches because the code
> > > diverged significantly.
> > > 
> > 
> > This change is _VERY_ different.  I would need acks from the maintainers
> > of this code before I could accept this, along with a much more detailed
> > description of why the original commits will not work here as well.
> > 
> > Same for the other backports.
> 
> Regarding backporting of 9f6dc633:
> 
> My reasoning was that introducing "md->pending_io" in the backported 
> stable kernels is useless - it will just degrade performance by consuming 
> one more cache line per I/O without providing any gain.
> 
> In the upstream kernels, Mike needs that "md->pending_io" variable for 
> other reasons (the I/O accounting was reworked there in order to avoid 
> some spikes with dm-crypt), but there is no need for it in the stable 
> kernels.
> 
> In order to fix that race condition, all we need to do is to make sure 
> that dm_stats_account_io is called before bio_end_io_acct - and the patch 
> does that - it swaps them.
> 
> Do you still insist that this useless percpu variable must be added to the 
> stable kernels? If you do, I can make it, but I think it's better to just 
> swap those two functions.

I am no insisting on anything, I want the dm maintainers to agree that
this change is acceptable to take as it is not what is in Linus's tree.
Every time we take a "not upstream" commit, the odds are 90% that it
ends up being wrong, so I need extra review and assurances that it is
acceptable before I can apply it.

thanks,

greg k-h

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel