Re: [PATCH 3/3] s390/dasd: Make FBA discard support conditional per device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Physical FBA devices can falsely inherit discard support if they bind to a bus_id previously used by a virtual FBA device.

Pre-existing issues:
- [High] Processing a discard or write zeroes request via `dasd_fba_free_cp()` causes a NULL pointer dereference when accessing `bio->bi_io_vec`.
--

--- Patch [3]: [PATCH 3/3] s390/dasd: Make FBA discard support conditional per device ---
commit 87109ad16dbff96168935ae6a9f15a26e6baaf51
Author: Volkan Unal <vunal@linux.ibm.com>

s390/dasd: Make FBA discard support conditional per device
    
This commit replaces the static discipline->has_discard flag with dynamic
per-device feature checks. Discard support is conditionally enabled only for
virtual FBA devices (6310/9336), while physical FBA devices (3880/3370) will
report no discard support.

> diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
> index 3181c06d91ce3..3e009217c130d 100644
> --- a/drivers/s390/block/dasd.c
> +++ b/drivers/s390/block/dasd.c
> @@ -327,7 +327,7 @@ static int dasd_state_basic_to_ready(struct dasd_device *device)
>  	 */
>  	lim.dma_alignment = lim.logical_block_size - 1;
>  
> -	if (device->discipline->has_discard) {
> +	if (dasd_get_feature(device->cdev, DASD_FEATURE_DISCARD) > 0) {
>  		unsigned int max_bytes;
>  
>  		lim.discard_granularity = block->bp_block;
> diff --git a/drivers/s390/block/dasd_fba.c b/drivers/s390/block/dasd_fba.c
> index 029acb83ecab6..b28661b7c0446 100644
> --- a/drivers/s390/block/dasd_fba.c
> +++ b/drivers/s390/block/dasd_fba.c
[ ... ]
> @@ -162,8 +172,9 @@ dasd_fba_check_characteristics(struct dasd_device *device)
>  	if (readonly)
>  		set_bit(DASD_FLAG_DEVICE_RO, &device->flags);
>  
> -	/* FBA supports discard, set the according feature bit */
> -	dasd_set_feature(cdev, DASD_FEATURE_DISCARD, 1);
> +	/* Virtual and EDEV FBA supports discard, set the according feature bit */
> +	if (cdev_is_virtual_or_edev(cdev))
> +		dasd_set_feature(cdev, DASD_FEATURE_DISCARD, 1);

[Severity: Medium]
Will physical FBA devices inherit discard support if they bind to a bus_id
previously used by a virtual FBA device?

If a virtual device is unbound, the devmap persists in memory with the
DASD_FEATURE_DISCARD bit set. If a physical FBA device is subsequently bound
to the same subchannel, this code doesn't explicitly clear the bit, which
might cause the physical device to incorrectly advertise discard support.

Should there be an else branch to clear the feature bit for physical devices?

[ ... ]
> @@ -763,7 +774,6 @@ static struct dasd_discipline dasd_fba_discipline = {
>  	.owner = THIS_MODULE,
>  	.name = "FBA ",
>  	.ebcname = "FBA ",
> -	.has_discard = true,
>  	.check_device = dasd_fba_check_characteristics,

[Severity: High]
This is a pre-existing issue, but does dasd_fba_free_cp() crash when
processing discard or write zeroes requests?

If the kernel is booted with the fixedbuffers parameter (initializing
dasd_page_cache), dasd_fba_free_cp() unconditionally enters the segment loop:

drivers/s390/block/dasd_fba.c:dasd_fba_free_cp() {
    ...
	if (!dasd_page_cache)
		goto out;
    ...
	rq_for_each_segment(bv, req, iter) {
    ...
}

Since discard and write zeroes requests have no data payload, bio->bi_io_vec
is NULL, which will cause a NULL pointer dereference.

Does this iteration need a blk_rq_bytes() or bio_has_data() check before
traversing the segments?

>  	.do_analysis = dasd_fba_do_analysis,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722151651.292621-1-vunal@linux.ibm.com?part=3