linux-kernel - Re: [PATCH v3 0/6] bio_split() error handling rework

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: <dfe4b3e5-c39d-4e53-8df8-8ef53a1487f6@oracle.com>
Date: Thu, 7 Nov 2024 06:49:15 +0000
From: John Garry <john.g.garry@...cle.com>
To: axboe@...nel.dk, song@...nel.org, yukuai3@...wei.com, hch@....de
Cc: martin.petersen@...cle.com, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-raid@...r.kernel.org, hare@...e.de,
        Johannes.Thumshirn@....com
Subject: Re: [PATCH v3 0/6] bio_split() error handling rework

On 31/10/2024 09:59, John Garry wrote:

Hi Jens,

Could you kindly consider picking up this series via the block tree?

Please note that the raid 0/1/10 atomic write support in 
https://lore.kernel.org/linux-block/20241101144616.497602-1-john.g.garry@oracle.com/ 
depends on this series, so maybe you would also be willing to pick that 
one up (when it's fully reviewed). Or create a branch with all the block 
changes, like which was done for the atomic writes XFS support.

Thanks very much,
John

> bio_split() error handling could be improved as follows:
> - Instead of returning NULL for an error - which is vague - return a
>    PTR_ERR, which may hint what went wrong.
> - Remove BUG_ON() calls - which are generally not preferred - and instead
>    WARN and pass an error code back to the caller. Many callers of
>    bio_split() don't check the return code. As such, for an error we would
>    be getting a crash still from an invalid pointer dereference.
> 
> Most bio_split() callers don't check the return value. However, it could
> be argued the bio_split() calls should not fail. So far I have just
> fixed up the md RAID code to handle these errors, as that is my interest
> now.
> 
> The motivator for this series was initial md RAID atomic write support in
> https://lore.kernel.org/linux-block/20241030094912.3960234-1-john.g.garry@oracle.com/T/#m5859ee900de8e6554d5bb027c0558f0147c32df8
> 
> There I wanted to ensure that we don't split an atomic write bio, and it
> made more sense to handle this in bio_split() (instead of the bio_split()
> caller).
> 
> Based on 133008e84b99 (block/for-6.13/block) blk-integrity: remove
> seed for user mapped buffers
> 
> Changes since v2:
> - Drop "block: Use BLK_STS_OK in bio_init()" change (Christoph)
> - Use proper rdev indexing in raid10_write_request() (Kuai)
> - Decrement rdev nr_pending in raid1 read error path (Kuai)
> - Add RB tags from Christoph, Johannes, and Kuai (thanks!)
> 
> Changes since RFC:
> - proper handling to end the raid bio in all cases, and also pass back
>    proper error code (Kuai)
> - Add WARN_ON_ERROR in bio_split() (Johannes, Christoph)
> - Add small patch to use BLK_STS_OK in bio_init()
> - Change bio_submit_split() error path (Christoph)
> 
> John Garry (6):
>    block: Rework bio_split() return value
>    block: Error an attempt to split an atomic write in bio_split()
>    block: Handle bio_split() errors in bio_submit_split()
>    md/raid0: Handle bio_split() errors
>    md/raid1: Handle bio_split() errors
>    md/raid10: Handle bio_split() errors
> 
>   block/bio.c                 | 14 +++++++----
>   block/blk-crypto-fallback.c |  2 +-
>   block/blk-merge.c           | 15 ++++++++----
>   drivers/md/raid0.c          | 12 ++++++++++
>   drivers/md/raid1.c          | 33 ++++++++++++++++++++++++--
>   drivers/md/raid10.c         | 47 ++++++++++++++++++++++++++++++++++++-
>   6 files changed, 110 insertions(+), 13 deletions(-)
>