diff mbox series

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

Message ID bb8bc77e6ecc2287fc9110cfa08caa48eb2a0385.1696508228.git.limings@nvidia.com
State New
Headers
Series [v1,1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning message |

Commit Message

Liming Sun Oct. 5, 2023, 12:18 p.m. UTC 
  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@nvidia.com>
---
 drivers/platform/mellanox/mlxbf-tmfifo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Hans de Goede Oct. 6, 2023, 12:53 p.m. UTC | #1 
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@nvidia.com>
> ---
>  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
Liming Sun Oct. 6, 2023, 3:50 p.m. UTC | #2 
Thanks Hans.

Below is the logic:

IS_VRING_DROP() is ONLY set to TRUE for Rx, which is done in two places:
Line 696:  *desc = &vring->drop_desc;
Line 742: desc = &vring->drop_desc;

So line 634 below will never happen when IS_VRING_DROP() is TRUE due the checking of line 633.
633         if (!is_rx)
 634                 writeq(data, fifo->tx.data);

Please correct me if it's my misunderstanding.

Thanks,
Liming

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Friday, October 6, 2023 8:54 AM
> To: Liming Sun <limings@nvidia.com>; Vadim Pasternak
> <vadimp@nvidia.com>; David Thompson <davthompson@nvidia.com>; Mark
> Gross <markgross@kernel.org>; Dan Carpenter <dan.carpenter@linaro.org>
> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning
> message
> 
> 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@nvidia.com>
> > ---
> >  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
> 
> 
>
Hans de Goede Oct. 6, 2023, 5:06 p.m. UTC | #3 
Hi Liming,

On 10/6/23 17:50, Liming Sun wrote:
> Thanks Hans.
> 
> Below is the logic:
> 
> IS_VRING_DROP() is ONLY set to TRUE for Rx, which is done in two places:
> Line 696:  *desc = &vring->drop_desc;
> Line 742: desc = &vring->drop_desc;
> 
> So line 634 below will never happen when IS_VRING_DROP() is TRUE due the checking of line 633.
> 633         if (!is_rx)
>  634                 writeq(data, fifo->tx.data);
> 
> Please correct me if it's my misunderstanding.

If IS_VRING_DROP() is ONLY set to TRUE for Rx, then it
should simply *not* be checked *at all* in the tx paths.

Just setting data = 0 is simply papering over the warning
without actually fixing anything.

Regards,

Hans




>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Friday, October 6, 2023 8:54 AM
>> To: Liming Sun <limings@nvidia.com>; Vadim Pasternak
>> <vadimp@nvidia.com>; David Thompson <davthompson@nvidia.com>; Mark
>> Gross <markgross@kernel.org>; Dan Carpenter <dan.carpenter@linaro.org>
>> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning
>> message
>>
>> 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@nvidia.com>
>>> ---
>>>  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
>>
>>
>>
>
Liming Sun Oct. 9, 2023, 7:28 p.m. UTC | #4 
> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Friday, October 6, 2023 1:07 PM
> To: Liming Sun <limings@nvidia.com>; Vadim Pasternak
> <vadimp@nvidia.com>; David Thompson <davthompson@nvidia.com>; Mark
> Gross <markgross@kernel.org>; Dan Carpenter <dan.carpenter@linaro.org>
> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning
> message
> 
> Hi Liming,
> 
> On 10/6/23 17:50, Liming Sun wrote:
> > Thanks Hans.
> >
> > Below is the logic:
> >
> > IS_VRING_DROP() is ONLY set to TRUE for Rx, which is done in two places:
> > Line 696:  *desc = &vring->drop_desc;
> > Line 742: desc = &vring->drop_desc;
> >
> > So line 634 below will never happen when IS_VRING_DROP() is TRUE due the
> checking of line 633.
> > 633         if (!is_rx)
> >  634                 writeq(data, fifo->tx.data);
> >
> > Please correct me if it's my misunderstanding.
> 
> If IS_VRING_DROP() is ONLY set to TRUE for Rx, then it
> should simply *not* be checked *at all* in the tx paths.

IS_VRING_DROP() itself is actually not checked in the Tx path.  It is the "! IS_VRING_DROP()" that checks the Rx/Tx, something like:

if (!IS_VRING_DROP(vring)) {
	if (is_rx)
		...
	else
		...
}

The reason is that I thought we might reuse the ' IS_VRING_DROP' for Tx later.

However, if the logic looks confusing, I could revise it to something like:

if (is_rx) {
	if (!IS_VRING_DROP(vring)) 
		...
} else {
		...
}

Thanks.
> 
> Just setting data = 0 is simply papering over the warning
> without actually fixing anything.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> >> -----Original Message-----
> >> From: Hans de Goede <hdegoede@redhat.com>
> >> Sent: Friday, October 6, 2023 8:54 AM
> >> To: Liming Sun <limings@nvidia.com>; Vadim Pasternak
> >> <vadimp@nvidia.com>; David Thompson <davthompson@nvidia.com>;
> Mark
> >> Gross <markgross@kernel.org>; Dan Carpenter <dan.carpenter@linaro.org>
> >> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning
> >> message
> >>
> >> 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@nvidia.com>
> >>> ---
> >>>  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
> >>
> >>
> >>
> >
Hans de Goede Oct. 10, 2023, 8:05 a.m. UTC | #5 
Hi,

On 10/9/23 21:28, Liming Sun wrote:
> 
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Friday, October 6, 2023 1:07 PM
>> To: Liming Sun <limings@nvidia.com>; Vadim Pasternak
>> <vadimp@nvidia.com>; David Thompson <davthompson@nvidia.com>; Mark
>> Gross <markgross@kernel.org>; Dan Carpenter <dan.carpenter@linaro.org>
>> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning
>> message
>>
>> Hi Liming,
>>
>> On 10/6/23 17:50, Liming Sun wrote:
>>> Thanks Hans.
>>>
>>> Below is the logic:
>>>
>>> IS_VRING_DROP() is ONLY set to TRUE for Rx, which is done in two places:
>>> Line 696:  *desc = &vring->drop_desc;
>>> Line 742: desc = &vring->drop_desc;
>>>
>>> So line 634 below will never happen when IS_VRING_DROP() is TRUE due the
>> checking of line 633.
>>> 633         if (!is_rx)
>>>  634                 writeq(data, fifo->tx.data);
>>>
>>> Please correct me if it's my misunderstanding.
>>
>> If IS_VRING_DROP() is ONLY set to TRUE for Rx, then it
>> should simply *not* be checked *at all* in the tx paths.
> 
> IS_VRING_DROP() itself is actually not checked in the Tx path.  It is the "! IS_VRING_DROP()" that checks the Rx/Tx, something like:
> 
> if (!IS_VRING_DROP(vring)) {
> 	if (is_rx)
> 		...
> 	else
> 		...
> }
> 
> The reason is that I thought we might reuse the ' IS_VRING_DROP' for Tx later.
> 
> However, if the logic looks confusing, I could revise it to something like:
> 
> if (is_rx) {
> 	if (!IS_VRING_DROP(vring)) 
> 		...
> } else {
> 		...
> }

Yes please revise the log to look like this, this should also
fix the warning without needing to initialize data to 0.

Since now in the tx path you are guaranteed to first fill
data before sending it.

Regards,

Hans



>>>> -----Original Message-----
>>>> From: Hans de Goede <hdegoede@redhat.com>
>>>> Sent: Friday, October 6, 2023 8:54 AM
>>>> To: Liming Sun <limings@nvidia.com>; Vadim Pasternak
>>>> <vadimp@nvidia.com>; David Thompson <davthompson@nvidia.com>;
>> Mark
>>>> Gross <markgross@kernel.org>; Dan Carpenter <dan.carpenter@linaro.org>
>>>> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning
>>>> message
>>>>
>>>> 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@nvidia.com>
>>>>> ---
>>>>>  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
>>>>
>>>>
>>>>
>>>
>
Liming Sun Oct. 12, 2023, 11:03 p.m. UTC | #6 
> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Tuesday, October 10, 2023 4:06 AM
> To: Liming Sun <limings@nvidia.com>; Vadim Pasternak
> <vadimp@nvidia.com>; David Thompson <davthompson@nvidia.com>; Mark
> Gross <markgross@kernel.org>; Dan Carpenter <dan.carpenter@linaro.org>
> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning
> message
> 
> Hi,
> 
> On 10/9/23 21:28, Liming Sun wrote:
> >
> >
> >> -----Original Message-----
> >> From: Hans de Goede <hdegoede@redhat.com>
> >> Sent: Friday, October 6, 2023 1:07 PM
> >> To: Liming Sun <limings@nvidia.com>; Vadim Pasternak
> >> <vadimp@nvidia.com>; David Thompson <davthompson@nvidia.com>;
> Mark
> >> Gross <markgross@kernel.org>; Dan Carpenter <dan.carpenter@linaro.org>
> >> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a warning
> >> message
> >>
> >> Hi Liming,
> >>
> >> On 10/6/23 17:50, Liming Sun wrote:
> >>> Thanks Hans.
> >>>
> >>> Below is the logic:
> >>>
> >>> IS_VRING_DROP() is ONLY set to TRUE for Rx, which is done in two places:
> >>> Line 696:  *desc = &vring->drop_desc;
> >>> Line 742: desc = &vring->drop_desc;
> >>>
> >>> So line 634 below will never happen when IS_VRING_DROP() is TRUE due
> the
> >> checking of line 633.
> >>> 633         if (!is_rx)
> >>>  634                 writeq(data, fifo->tx.data);
> >>>
> >>> Please correct me if it's my misunderstanding.
> >>
> >> If IS_VRING_DROP() is ONLY set to TRUE for Rx, then it
> >> should simply *not* be checked *at all* in the tx paths.
> >
> > IS_VRING_DROP() itself is actually not checked in the Tx path.  It is the "!
> IS_VRING_DROP()" that checks the Rx/Tx, something like:
> >
> > if (!IS_VRING_DROP(vring)) {
> > 	if (is_rx)
> > 		...
> > 	else
> > 		...
> > }
> >
> > The reason is that I thought we might reuse the ' IS_VRING_DROP' for Tx
> later.
> >
> > However, if the logic looks confusing, I could revise it to something like:
> >
> > if (is_rx) {
> > 	if (!IS_VRING_DROP(vring))
> > 		...
> > } else {
> > 		...
> > }
> 
> Yes please revise the log to look like this, this should also
> fix the warning without needing to initialize data to 0.
> 
> Since now in the tx path you are guaranteed to first fill
> data before sending it.

Thanks, done. Updated in v2.

> 
> Regards,
> 
> Hans
> 
> 
> 
> >>>> -----Original Message-----
> >>>> From: Hans de Goede <hdegoede@redhat.com>
> >>>> Sent: Friday, October 6, 2023 8:54 AM
> >>>> To: Liming Sun <limings@nvidia.com>; Vadim Pasternak
> >>>> <vadimp@nvidia.com>; David Thompson <davthompson@nvidia.com>;
> >> Mark
> >>>> Gross <markgross@kernel.org>; Dan Carpenter
> <dan.carpenter@linaro.org>
> >>>> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> >>>> Subject: Re: [PATCH v1 1/1] platform/mellanox: mlxbf-tmfifo: Fix a
> warning
> >>>> message
> >>>>
> >>>> 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@nvidia.com>
> >>>>> ---
> >>>>>  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
> >>>>
> >>>>
> >>>>
> >>>
> >
diff mbox series

Patch

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));