linux-kernel - Re: [PATCH v2 05/14] block: blk-merge: fix to add the number of integrity segments to the request twice

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 12 May 2023 15:51:36 +0200
From:   "hch@....de" <hch@....de>
To:     Jinyoung CHOI <j-young.choi@...sung.com>
Cc:     "axboe@...nel.dk" <axboe@...nel.dk>,
        "kbusch@...nel.org" <kbusch@...nel.org>, "hch@....de" <hch@....de>,
        "sagi@...mberg.me" <sagi@...mberg.me>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "johannes.thumshirn@....com" <johannes.thumshirn@....com>,
        "kch@...dia.com" <kch@...dia.com>,
        "willy@...radead.org" <willy@...radead.org>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 05/14] block: blk-merge: fix to add the number of
 integrity segments to the request twice

The subject looks a bit odd, I think you're trying to say:

"do not add the number of integrity segments to the request twice"

based on the actual patch, is this correct?

> blk_integrity_merge_bio() not only performs conditional tests, but also
> updates the integrity segment information of request.
> It can be called twice when merging the bio into an existing request.
> 
> bio_attempt_bio_merge() or blk_mq_sched_try_merge()
>   blk_rq_merge_ok()
>     blk_integrity_merge_bio()  -  1
>   bio_attemp_{back|front}_merge()
>     ll_{back|front}_merge_fn()
>       ll_new_hw_segments()
>         blk_integrity_merge_bio()  -  2
> 
> The part of checking the conditions and the code to update the
> information of the actual request were separated. At this time, the
> ll_back_merge_fn was called by passth-path, so the condition check was
> called by all the separated functions.
> 
> And after success in blk_integrity_merge_bio(), the information of the
> request may be wrong if it is impossible to merge due to other
> conditional tests. Thus, it was changed to be called immediately before
> merging the bio's segments.


> +static inline bool blk_integrity_bypass_check(struct request *req,
> +					      struct bio *bio)
> +{
> +	return blk_integrity_rq(req) == 0 && bio_integrity(bio) == NULL;
> +}

No need for the explicit comparisms, this could just be:

	return !blk_integrity_rq(req) && !bio_integrity(bio);

and given that it just has two callers I'm not sure the helper is
all that useful to start with.

> +static bool __blk_integrity_mergeable(struct request_queue *q,
> +				      struct request *req, struct bio *bio)
> +{
> +	if (blk_integrity_rq(req) == 0 || bio_integrity(bio) == NULL)
> +		return false;
> +
> +	if (bio_integrity(req->bio)->bip_flags != bio_integrity(bio)->bip_flags)
> +		return false;
> +
> +	return true;
> +}
> +
> +bool blk_integrity_mergeable(struct request_queue *q, struct request *req,
> +			     struct bio *bio)
> +{
> +	if (blk_integrity_bypass_check(req, bio))
> +		return true;
> +
> +	return __blk_integrity_mergeable(q, req, bio);
> +}

Similarly here, I'm not even sure we need all these helpers.  I supect
the code would become more readable by dropping these helpers and just
making the checks explicitlẏ