block/bio.c | 14 ++++++++++---- block/blk-crypto-fallback.c | 2 +- block/blk-merge.c | 5 +++++ drivers/md/raid0.c | 10 ++++++++++ drivers/md/raid1.c | 8 ++++++++ drivers/md/raid10.c | 18 ++++++++++++++++++ 6 files changed, 52 insertions(+), 5 deletions(-)
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. Sending as an RFC as unsure if this is the right direction. The motivator for this series was initial md RAID atomic write support in https://lore.kernel.org/linux-block/21f19b4b-4b83-4ca2-a93b-0a433741fd26@oracle.com/ 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). 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 | 5 +++++ drivers/md/raid0.c | 10 ++++++++++ drivers/md/raid1.c | 8 ++++++++ drivers/md/raid10.c | 18 ++++++++++++++++++ 6 files changed, 52 insertions(+), 5 deletions(-) -- 2.31.1
On 9/19/24 11:22, John Garry wrote: > 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. > > Sending as an RFC as unsure if this is the right direction. > > The motivator for this series was initial md RAID atomic write support in > https://lore.kernel.org/linux-block/21f19b4b-4b83-4ca2-a93b-0a433741fd26@oracle.com/ > > 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). > > 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 | 5 +++++ > drivers/md/raid0.c | 10 ++++++++++ > drivers/md/raid1.c | 8 ++++++++ > drivers/md/raid10.c | 18 ++++++++++++++++++ > 6 files changed, 52 insertions(+), 5 deletions(-) > You are missing '__bio_split_to_limits()' which looks as it would need to be modified, too. Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
On 23/09/2024 06:53, Hannes Reinecke wrote: > On 9/19/24 11:22, John Garry wrote: >> 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. >> >> Sending as an RFC as unsure if this is the right direction. >> >> The motivator for this series was initial md RAID atomic write support in >> https://lore.kernel.org/linux-block/21f19b4b-4b83-4ca2- >> a93b-0a433741fd26@oracle.com/ >> >> 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). >> >> 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 | 5 +++++ >> drivers/md/raid0.c | 10 ++++++++++ >> drivers/md/raid1.c | 8 ++++++++ >> drivers/md/raid10.c | 18 ++++++++++++++++++ >> 6 files changed, 52 insertions(+), 5 deletions(-) >> > You are missing '__bio_split_to_limits()' which looks as it would need > to be modified, too. > In __bio_split_to_limits(), for REQ_OP_DISCARD, REQ_OP_SECURE_ERASE, and REQ_OP_WRITE_ZEROES, we indirectly call bio_split(). And bio_split() might error. But functions like bio_split_discard() can return NULL for cases where a split is not required. So I suppose we need to check IS_ERR(split) for those request types mentioned. For NULL being returned, we would still have the __bio_split_to_limits() is "if (split)" check. Thanks, John
On 9/23/24 09:19, John Garry wrote:
> On 23/09/2024 06:53, Hannes Reinecke wrote:
>> On 9/19/24 11:22, John Garry wrote:
>>> 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.
>>>
>>> Sending as an RFC as unsure if this is the right direction.
>>>
>>> The motivator for this series was initial md RAID atomic write
>>> support in
>>> https://lore.kernel.org/linux-block/21f19b4b-4b83-4ca2-
>>> a93b-0a433741fd26@oracle.com/
>>>
>>> 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).
>>>
>>> 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 | 5 +++++
>>> drivers/md/raid0.c | 10 ++++++++++
>>> drivers/md/raid1.c | 8 ++++++++
>>> drivers/md/raid10.c | 18 ++++++++++++++++++
>>> 6 files changed, 52 insertions(+), 5 deletions(-)
>>>
>> You are missing '__bio_split_to_limits()' which looks as it would need
>> to be modified, too.
>>
>
> In __bio_split_to_limits(), for REQ_OP_DISCARD, REQ_OP_SECURE_ERASE, and
> REQ_OP_WRITE_ZEROES, we indirectly call bio_split(). And bio_split()
> might error. But functions like bio_split_discard() can return NULL for
> cases where a split is not required. So I suppose we need to check
> IS_ERR(split) for those request types mentioned. For NULL being
> returned, we would still have the __bio_split_to_limits() is "if
> (split)" check.
>
Indeed. And then you'll need to modify nvme:
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index f72c5a6a2d8e..c99f51e7730e 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -453,7 +453,7 @@ static void nvme_ns_head_submit_bio(struct bio *bio)
* pool from the original queue to allocate the bvecs from.
*/
bio = bio_split_to_limits(bio);
- if (!bio)
+ if (IS_ERR_OR_NULL(bio))
return;
srcu_idx = srcu_read_lock(&head->srcu);
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
On 23/09/2024 10:43, Hannes Reinecke wrote: > On 9/23/24 09:19, John Garry wrote: >> On 23/09/2024 06:53, Hannes Reinecke wrote: >>> On 9/19/24 11:22, John Garry wrote: >>>> 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. >>>> >>>> Sending as an RFC as unsure if this is the right direction. >>>> >>>> The motivator for this series was initial md RAID atomic write >>>> support in >>>> https://lore.kernel.org/linux-block/21f19b4b-4b83-4ca2- >>>> a93b-0a433741fd26@oracle.com/ >>>> >>>> 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). >>>> >>>> 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 | 5 +++++ >>>> drivers/md/raid0.c | 10 ++++++++++ >>>> drivers/md/raid1.c | 8 ++++++++ >>>> drivers/md/raid10.c | 18 ++++++++++++++++++ >>>> 6 files changed, 52 insertions(+), 5 deletions(-) >>>> >>> You are missing '__bio_split_to_limits()' which looks as it would >>> need to be modified, too. >>> >> >> In __bio_split_to_limits(), for REQ_OP_DISCARD, REQ_OP_SECURE_ERASE, >> and REQ_OP_WRITE_ZEROES, we indirectly call bio_split(). And >> bio_split() might error. But functions like bio_split_discard() can >> return NULL for cases where a split is not required. So I suppose we >> need to check IS_ERR(split) for those request types mentioned. For >> NULL being returned, we would still have the __bio_split_to_limits() >> is "if (split)" check. >> hold on a moment - were you looking at latest code on Jens' branch? There __bio_split_to_limits() will not return a ERR_PTR() (from changes in this series) - it will still just return NULL or a bio. In all cases there, __bio_split_to_limits() calls bio_submit_rw(), and still bio_submit_rw() will return NULL or a proper bio. This is because we translate a ERR_PTR() from bio_split() to NULL. Christoph changed this bio splitting in https://lore.kernel.org/linux-block/20240826173820.1690925-1-hch@lst.de/ I think that if my changes were based on v6.11, you were right. Thanks, John
© 2016 - 2024 Red Hat, Inc.