Re: [PATCH 11/11] PCI: endpoint: pci-epf-vntb: Expose packed MWs through configfs

[sashiko-bot@kernel.org]

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

New issues:
- [High] vntb_epf_mw_set_trans() does not reject calls when packed_mws is enabled, allowing legacy NTB clients to corrupt the shared BAR configuration.
- [Medium] Data race on `ntb->packed_mws` in configfs `show` methods can lead to theoretical divide-by-zero.
- [Low] Using native division and modulo operators on a 64-bit integer will cause a link-time error on 32-bit architectures.

Pre-existing issues:
- [High] The loop initializing optional BARs incorrectly starts at BAR_MW1 instead of BAR_MW2, leaving BAR_MW4 uninitialized when num_mws is 4.
--

--- Patch [11]: [PATCH 11/11] PCI: endpoint: pci-epf-vntb: Expose packed MWs through configfs ---
commit ff5e63e72a02a10c0fca1af20a09901153b50a18
Author: Koichiro Den <den@valinux.co.jp>
PCI: endpoint: pci-epf-vntb: Expose packed MWs through configfs

This patch adds support for exposing packed memory windows via configfs,
enabling up to 16 logical memory windows when packing is selected. It
utilizes mw1 as the aggregate BAR size and restricts other window sizes
and BAR attributes when packing is active.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c

[ ... ]

> @@ -1295,6 +1295,7 @@ static ssize_t epf_ntb_##_name##_show(struct config_item *item,		\
>  	struct config_group *group = to_config_group(item);		\
>  	struct epf_ntb *ntb = to_epf_ntb(group);			\
>  	struct device *dev = &ntb->epf->dev;				\
> +	u32 packed_mws;							\
>  	int win_no, idx;						\
>  									\
>  	if (sscanf(#_name, "mw%d", &win_no) != 1)			\
> @@ -1306,6 +1307,14 @@ static ssize_t epf_ntb_##_name##_show(struct config_item *item,		\
>  			win_no, ntb->num_mws);				\
>  		return -ERANGE;						\
>  	}								\
> +	packed_mws = ntb->packed_mws;					\
> +	if (packed_mws && idx > 0) {					\
> +		u64 size = ntb->mws_size[0];				\
> +									\
> +		if (size % packed_mws)					\
> +			return -EINVAL;					\
> +		return sprintf(page, "%llu\n", size / packed_mws);	\

[Severity: Medium]
Could the compiler re-fetch ntb->packed_mws from memory during the modulo
or division operations? If a concurrent configfs thread writes 0 to
packed_mws between the initial check and the division, might this result in
a divide-by-zero exception? Would using READ_ONCE() here be appropriate?

[Severity: Low]
Does using the native modulo and division operators on a 64-bit integer
(size) and a 32-bit integer (packed_mws) cause a link-time error on 32-bit
architectures due to missing symbols? Should this use div64_u64() or
do_div() instead? This same pattern also appears to be present in
epf_ntb_build_mw_layout().

> +	}								\
>  	idx = array_index_nospec(idx, ntb->num_mws);			\
>  	return sprintf(page, "%llu\n", ntb->mws_size[idx]);		\
>  }

[ ... ]

> @@ -1400,6 +1417,44 @@ static ssize_t epf_ntb_num_mws_store(struct config_item *item,
>  	return len;
>  }
>  
> +static ssize_t epf_ntb_packed_mws_store(struct config_item *item,
> +					const char *page, size_t len)
> +{
> +	struct config_group *group = to_config_group(item);
> +	struct epf_ntb *ntb = to_epf_ntb(group);
> +	u32 val;
> +	int ret;
> +	int i;
> +
> +	if (epf_ntb_epc_attached(ntb))
> +		return -EOPNOTSUPP;
> +
> +	ret = kstrtou32(page, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val > EPF_NTB_MAX_MW ||
> +	    (val && (val < 2 || !is_power_of_2(val))))
> +		return -EINVAL;
> +
> +	if (val && ntb->num_mws && val != ntb->num_mws)
> +		return -EINVAL;
> +
> +	if (val) {
> +		for (i = 1; i < MAX_MW; i++)
> +			if (ntb->mws_size[i])
> +				return -EINVAL;
> +
> +		for (i = BAR_MW2; i <= BAR_MW4; i++)
> +			if (ntb->epf_ntb_bar[i] != NO_BAR)
> +				return -EINVAL;
> +	}
> +
> +	ntb->packed_mws = val;
> +
> +	return len;
> +}

[Severity: High]
When packed_mws is enabled, multiple logical memory windows share a single
physical PCI BAR. Does vntb_epf_mw_set_trans() need a check to reject calls
if packed_mws is set? If a legacy NTB client driver calls
vntb_epf_mw_set_trans(), could it overwrite the shared epf_bar->size with a
smaller logical window size and reconfigure the physical BAR, breaking the
configuration for all shared windows?

[Severity: High]
This is a pre-existing issue, but there appears to be a logic error in
epf_ntb_init_epc_bar(). The automatic BAR assignment loop for optional
memory windows starts iterating from BAR_MW1 with i=1 instead of BAR_MW2:

drivers/pci/endpoint/functions/pci-epf-vntb.c:epf_ntb_init_epc_bar() {
    ...
    for (bar = BAR_MW1, i = 1;
         i < num_mws && bar <= BAR_MW4; bar++, i++) {
        barno = epf_ntb_find_bar(ntb, epc_features, bar, barno);
    ...
}

Since BAR_MW1 was already assigned earlier in the function, this redundant
check consumes an iteration. When num_mws is 4, doesn't the loop terminate
when bar reaches BAR_MW3, leaving BAR_MW4 uninitialized and causing
automatic BAR assignment to fail?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803180439.1963864-1-den@valinux.co.jp?part=11