Re: [PATCH v3 2/3] s390/diag204: Replace diag204 users with new implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] s390/diag204: diag204_store() calls fail with -EINVAL without BIF support
--

commit e6a66aa1f5ad19fef893eed0590d4b6f880aaa42
Author: Mete Durlu <meted@linux.ibm.com>

s390/diag204: Replace diag204 users with new implementation

This commit converts existing kernelspace diagnose 204 users in hypfs and
sthyi to use the new centralized diag204 interface.

> diff --git a/arch/s390/hypfs/hypfs_diag.c b/arch/s390/hypfs/hypfs_diag.c
> index 08777f57bd244..c3eebe4667faf 100644
> --- a/arch/s390/hypfs/hypfs_diag.c
> +++ b/arch/s390/hypfs/hypfs_diag.c
> @@ -22,139 +22,24 @@

[ ... ]

> -int diag204_store(void *buf, int pages)
> -{
> -	unsigned long subcode;
> -	int rc;
> -
> -	subcode = diag204_get_info_type();
> -	subcode |= diag204_store_sc;
> -	if (diag204_has_bif())
> -		subcode |= DIAG204_BIF_BIT;
> -	while (1) {
> -		rc = diag204(subcode, pages, buf);
> -		if (rc != -EBUSY)
> -			break;
> -		if (signal_pending(current))
> -			return -ERESTARTSYS;
> -		schedule_timeout_interruptible(DIAG204_BUSY_WAIT);
> -	}
> -	return rc < 0 ? rc : 0;
> -}
> -
>  struct dbfs_d204_hdr {
>  	u64	len;		/* Length of d204 buffer without header */
>  	u16	version;	/* Version of header */
> @@ -173,19 +58,20 @@ static int dbfs_d204_create(void **data, void **data_free_ptr, size_t *size)
>  	int rc, buf_size;
>  	void *base;
>  
> -	buf_size = PAGE_SIZE * (diag204_buf_pages + 1) + sizeof(d204->hdr);
> +	buf_size = diag204_get_buf_size();
> +	buf_size += PAGE_SIZE + sizeof(d204->hdr);
>  	base = vzalloc(buf_size);
>  	if (!base)
>  		return -ENOMEM;
>  	d204 = PTR_ALIGN(base + sizeof(d204->hdr), PAGE_SIZE) - sizeof(d204->hdr);
> -	rc = diag204_store(d204->buf, diag204_buf_pages);
> -	if (rc) {
> +	rc = diag204_store(d204->buf, 1, 1);
> +	if (rc < 0) {
>  		vfree(base);
>  		return rc;
>  	}

[ ... ]

> diff --git a/arch/s390/hypfs/hypfs_diag_fs.c b/arch/s390/hypfs/hypfs_diag_fs.c
> index 39621c3260103..2ce9119abe990 100644
> --- a/arch/s390/hypfs/hypfs_diag_fs.c
> +++ b/arch/s390/hypfs/hypfs_diag_fs.c
> @@ -303,15 +303,14 @@ int hypfs_diag_create_files(struct dentry *root)
>  	struct dentry *systems_dir, *hyp_dir;
>  	void *time_hdr, *part_hdr;
>  	void *buffer, *ptr;
> -	int i, rc, pages;
> +	int i, rc;
>  
> -	buffer = diag204_get_buffer(diag204_get_info_type(), &pages);
> +	buffer = hypfs_diag204_get_buffer();
>  	if (IS_ERR(buffer))
>  		return PTR_ERR(buffer);
> -	rc = diag204_store(buffer, pages);
> -	if (rc)
> +	rc = diag204_store(buffer, 1, 1);
> +	if (rc < 0)
>  		return rc;
> -
>  	systems_dir = hypfs_mkdir(root, "systems");
>  	if (IS_ERR(systems_dir))
>  		return PTR_ERR(systems_dir);

[ ... ]

> diff --git a/arch/s390/kernel/sthyi.c b/arch/s390/kernel/sthyi.c
> index 5eae2e25997ae..9adb26588d5b5 100644
> --- a/arch/s390/kernel/sthyi.c
> +++ b/arch/s390/kernel/sthyi.c
> @@ -305,29 +305,19 @@ static struct diag204_x_part_block *lpar_cpu_inf(struct lpar_cpu_inf *part_inf,
>  
>  static void *diag204_get_data(bool diag204_allow_busy)
>  {
> -	unsigned long subcode;
>  	void *diag204_buf;
> -	int pages, rc;
> -
> -	subcode = DIAG204_SUBC_RSI;
> -	subcode |= DIAG204_INFO_EXT;
> -	pages = diag204(subcode, 0, NULL);
> -	if (pages < 0)
> -		return ERR_PTR(pages);
> -	if (pages == 0)
> -		return ERR_PTR(-ENODATA);
> -	diag204_buf = __vmalloc_node(array_size(pages, PAGE_SIZE),
> -				     PAGE_SIZE, GFP_KERNEL, NUMA_NO_NODE,
> -				     __builtin_return_address(0));
> +	int rc;
> +
> +	if (!diag204_is_available())
> +		return ERR_PTR(-EOPNOTSUPP);
> +	if (diag204_get_subcode() != DIAG204_SUBC_STIB7)
> +		return ERR_PTR(-EOPNOTSUPP);
> +	diag204_buf = diag204_allocate_buffer();
>  	if (!diag204_buf)
>  		return ERR_PTR(-ENOMEM);
> -	subcode = DIAG204_SUBC_STIB7;
> -	subcode |= DIAG204_INFO_EXT;
> -	if (diag204_has_bif() && diag204_allow_busy)
> -		subcode |= DIAG204_BIF_BIT;
> -	rc = diag204(subcode, pages, diag204_buf);
> +	rc = diag204_store(diag204_buf, diag204_allow_busy, 0);
>  	if (rc < 0) {
> -		vfree(diag204_buf);
> +		diag204_free_buffer(diag204_buf);
>  		return ERR_PTR(rc);
>  	}
>  	return diag204_buf;

[Severity: High]
Does this cause diag204_store() calls in hypfs and sthyi to fail with
-EINVAL on systems without the Diagnose 204 Busy Indication Facility
(BIF)?

In arch/s390/kernel/diag/diag204.c, diag204_store() rejects any call
where allow_busy is set if g_info.has_bif is false:

    if (!allow_busy && allow_busy_wait)
        return -EOPNOTSUPP;
    if (allow_busy && !g_info.has_bif)
        return -EINVAL;

Previously, diag204_store() in arch/s390/hypfs/hypfs_diag.c checked
diag204_has_bif() before setting DIAG204_BIF_BIT, and diag204_get_data()
in arch/s390/kernel/sthyi.c checked both diag204_has_bif() and
diag204_allow_busy. When BIF was not supported, both callers issued
diagnose 204 without DIAG204_BIF_BIT.

With this change, dbfs_d204_create() in arch/s390/hypfs/hypfs_diag.c and
hypfs_diag_create_files() in arch/s390/hypfs/hypfs_diag_fs.c both pass
allow_busy = 1 and allow_busy_wait = 1 unconditionally:

    rc = diag204_store(d204->buf, 1, 1);

and:

    rc = diag204_store(buffer, 1, 1);

When g_info.has_bif is false, reading the debugfs diag_204 file or
mounting and updating s390_hypfs will immediately return -EINVAL.

Similarly, in arch/s390/kernel/sthyi.c, diag204_get_data() now passes
diag204_allow_busy directly to diag204_store():

    sthyi_fill()
      -> sthyi_update_cache()
        -> fill_dst()
          -> diag204_get_data(is_diag204_cached(sctns))
            -> diag204_store(diag204_buf, diag204_allow_busy, 0)

On systems without hardware STHYI (!test_facility(74)) and without BIF,
the first sthyi_fill() call succeeds because sthyi_cache.info is zeroed
and is_diag204_cached(sctns) is false. Once fill_diag() populates the
validity bits in sthyi_cache.info and the cache expires after
CACHE_VALID_JIFFIES, subsequent calls to sthyi_fill() pass
diag204_allow_busy = true and fail with -EINVAL because
sthyi_update_cache() only handles -EBUSY.

Should these callers gate allow_busy and allow_busy_wait on BIF support,
or should diag204_store() fall back to a non-BIF store when
!g_info.has_bif?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-diag204ioctl-v3-0-c25e7147736e@linux.ibm.com?part=2