linux-kernel - Re: [PATCH v3 5/6] vdpa/mlx5: conditionally show MTU and STATUS in config space

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <d6265719-a423-2798-4cd0-b4b57a34878b@nvidia.com>
Date:   Sun, 5 Feb 2023 11:36:30 +0200
From:   Eli Cohen <elic@...dia.com>
To:     Si-Wei Liu <si-wei.liu@...cle.com>, mst@...hat.com,
        jasowang@...hat.com, parav@...dia.com
Cc:     virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/6] vdpa/mlx5: conditionally show MTU and STATUS in
 config space


On 03/02/2023 7:02, Si-Wei Liu wrote:
> The spec says:
>      mtu only exists if VIRTIO_NET_F_MTU is set
>      status only exists if VIRTIO_NET_F_STATUS is set
>
> We should only show MTU and STATUS conditionally depending on
> the feature bits.
>
> Signed-off-by: Si-Wei Liu <si-wei.liu@...cle.com>
> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 22 ++++++++++++++--------
>   1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 3a6dbbc6..867ac18 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -3009,6 +3009,8 @@ static int event_handler(struct notifier_block *nb, unsigned long event, void *p
>   	struct mlx5_vdpa_wq_ent *wqent;
>   
>   	if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
> +		if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS)))
> +			return NOTIFY_DONE;
>   		switch (eqe->sub_type) {
>   		case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
>   		case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
> @@ -3118,16 +3120,20 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>   			goto err_alloc;
>   	}
>   
> -	err = query_mtu(mdev, &mtu);
> -	if (err)
> -		goto err_alloc;
> +	if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_MTU)) {

VIRTIO_NET_F_MTU is offered by the device. So conditional is always true. We are not done with feature negotiation at this stage so you may still set a value to device mtu if MTU won't be negotiated eventually. But that is not a problem because the spec says:

  VIRTIO_NET_F_MTU(3) Device maximum MTU reporting is supported. If 
offered by the device, device
advises driver about the value of its maximum MTU. If negotiated, the 
driver uses mtu as the maximum

MTU value.

So the driver will use whatever value is there only if negotiated.

> +		err = query_mtu(mdev, &mtu);
> +		if (err)
> +			goto err_alloc;
>   
> -	ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
> +		ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
> +	}
>   
> -	if (get_link_state(mvdev))
> -		ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
> -	else
> -		ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
> +	if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
> +		if (get_link_state(mvdev))
> +			ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
> +		else
> +			ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
> +	}
Same thing here. Feature negotiation is not complete yet and if

VIRTIO_NET_F_STATUS ends up not being negotiated, the driver will ignore this value.

>   
>   	if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
>   		memcpy(ndev->config.mac, add_config->net.mac, ETH_ALEN);