Re: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning message

[Hans de Goede]

Hi Liming,

On 10/5/23 14:18, Liming Sun wrote:
> This commit fixes the smatch static checker warning in
> mlxbf_tmfifo_rxtx_word() which complains data not initialized at
> line 634 when IS_VRING_DROP() is TRUE. This is not a real bug since
> line 634 is for Tx while IS_VRING_DROP() is only set for Rx. So there
> is no case that line 634 is executed when IS_VRING_DROP() is TRUE.
> 
> This commit initializes the local data variable to avoid unnecessary
> confusion to those static analyzing tools.
> 
> Signed-off-by: Liming Sun <limings@xxxxxxxxxx>
> ---
>  drivers/platform/mellanox/mlxbf-tmfifo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c b/drivers/platform/mellanox/mlxbf-tmfifo.c
> index f3696a54a2bd..ccc4b51d3379 100644
> --- a/drivers/platform/mellanox/mlxbf-tmfifo.c
> +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
> @@ -595,8 +595,8 @@ static void mlxbf_tmfifo_rxtx_word(struct mlxbf_tmfifo_vring *vring,
>  {
>  	struct virtio_device *vdev = vring->vq->vdev;
>  	struct mlxbf_tmfifo *fifo = vring->fifo;
> +	u64 data = 0;
>  	void *addr;
> -	u64 data;
>  
>  	/* Get the buffer address of this desc. */
>  	addr = phys_to_virt(virtio64_to_cpu(vdev, desc->addr));


This will fix the warning but not the issue at hand. As Dan pointed
out in his original bug report, the issue is that after:

78034cbece79 ("platform/mellanox: mlxbf-tmfifo: Drop the Rx packet if no descriptors")

We now have this IS_VRING_DROP() check in the path, which despite
the subject writeq(data, fifo->tx.data);is currently being applied to both rx and tx vring-s
and when this returns true the memcpy from the ring to &data
will not happen, but the code will still do:

writeq(data, fifo->tx.data);

So you may have silenced the warning now, but you will still write
data not coming from the vring to transmit. The only difference
is you are now guaranteed to write all zeroes.

Note another older issue is that if you hit the not enough space
path:

       } else {
                /* Leftover bytes. */
                if (!IS_VRING_DROP(vring)) {
                        if (is_rx)
                                memcpy(addr + vring->cur_len, &data,
                                       len - vring->cur_len);
                        else
                                memcpy(&data, addr + vring->cur_len,
                                       len - vring->cur_len);
                }
                vring->cur_len = len;
        }

Then even if IS_VRING_DROP() returns true you are only initializing some bytes of the 8 bytes data variable and the other bytes will stay at whatever random value they had before and you end up writing this random bytes when doing:

writeq(data, fifo->tx.data);

Regards,

Hans